-
Notifications
You must be signed in to change notification settings - Fork 466
Add mcs admin trace api #82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewing 👀
.......
review completed 👍
Please update your branch with the latest from master |
done |
|
||
// Set number of goroutines to wait. wg.Wait() | ||
// waitsuntil counter is zero (all are done) | ||
wg.Add(3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the PR description regarding 3 goroutines but I don't have all the context, is this need it for some type of quorum? would someone want to increase this to 5 or 7 in the future? If the answer is yes an env variable should be added for user to configure this value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll explain:
- The goroutines are fixed for this (
trace
) functionality. A goroutine [1] for listen on client heartbeat is needed. Second goroutine is needed for Writing to the socket (send the trace info), and a Third one is needed for checking if it crashed on reading (on [1]), for instance, the client disconnected or the ping Deadline happend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could instead wait on their respective channels to close up instead of a wait group to ensure there's no deadlock due to a go routine never calling wg.Done
, since each goroutine already returns a channel, I think essentially you aonly need sendTraceInfo
routine to close to mark the process as finished, and you could signal the other 2 routines to close via context, elimitating the need for a wait group
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dvaldivia No, because I need any of (wsReadCheck or sendTraceInfo) to trigger a cancel context, not just sendTraceInfo. If ReadCheck fails that should cancel sendTraceInfo and viceversa. Else either the Read or the Trace would remain open. There is no deadlock since all return if context is cancelled.
Trace Api uses websocket to send trace information, a valid jwt token needs to be sent either on the header or as a cookie of the ws request to start. Three goroutines are needed to ensure communication if read hearbeat fails all trace should stop by cancelling the context. WaitGroups are needed to ensure all goroutines finish gracefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Uh oh!
There was an error while loading. Please reload this page.