[WIP] Announcements and Monitoring of DST #269
Conversation
vaibhawvipul
commented
Apr 2, 2024
vaibhawvipul
commented
- draft PR
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #269 +/- ##
==========================================
+ Coverage 58.46% 58.62% +0.16%
==========================================
Files 113 115 +2
Lines 9775 9845 +70
==========================================
+ Hits 5715 5772 +57
- Misses 3693 3706 +13
Partials 367 367 ☔ View full report in Codecov by Sentry. |
|
A few suggestions: MonitorsMake
Source internal/announcements/monitor.go AnnouncementExtend the
register is a Noop for Noop and adds a monitor to DstAnnouncement. On announce, iterate over all monitors an apply the event Note I believe if this is not true, please discuss, then my mental model is incomplete |
| defer tm.mutex.Unlock() | ||
|
|
||
| // Increment event type count | ||
| tm.eventTypeCount[event.Type]++ |
There was a problem hiding this comment.
This is the right idea: When an event happens, modify a counter. When we check the monitors at the end of the run, here we want taskCreated == taskCompleted. If that check is too aggressive, at least we want to check that taskCreated >= taskCompleted
if event.Type == "TaskCreated" {
tm.taskCreated ++
}
if event.Type == "TaskCompleted" {
tm.taskCompleted ++
}
|
|
||
| package announcements | ||
|
|
||
| type Monitors interface { |
There was a problem hiding this comment.
I would rename the interface to Monitor instead of Monitors (to the object not the verb)
| } else { | ||
| res = &http.Response{ | ||
| StatusCode: http.StatusInternalServerError, | ||
| } | ||
|
|
||
| event := announcements.NewEvent("HTTPResponse") |
| "github.com/resonatehq/resonate/internal/announcements" | ||
| ) | ||
|
|
||
| type taskMonitor struct { |
There was a problem hiding this comment.
TaskMonitor must not part of the core package for announcements and monitors, but part of the test/dst -- it's application specific
| type DstAnnouncement struct { | ||
| announcements []Event | ||
| monitors []Monitors | ||
| mutex sync.Mutex // Mutex for thread safety |
There was a problem hiding this comment.
Usually a better practice to have the mutex at the start of the struct.
As the struct need not calculate the offset every time to find the mutex variable inside the struct.
