feat: Implemented the Retry and logging changes #49
Conversation
AkbaraliShaikh
commented
Oct 3, 2022
•
AkbaraliShaikh
commented
- Added simple retry logic on error
- Added the websocket client implementation
- Added implementation for standard logging output
- Providing the interface to the user to implement the choice of logging
- Refactored the names
- Updated the README document
| // Do retries the function f, waits time between retries until a successful call is made. | ||
| // wait is time to wait for the next call | ||
| // maxAttempts is the maximum number if calls to the function. | ||
| func Do(wait time.Duration, maxAttempts uint, f func() error) error { |
There was a problem hiding this comment.
@AkbaraliShaikh How will it handle OS interrupts? Shall we accept context and handle it?
There was a problem hiding this comment.
@ravisuhag Just trying to figure out why we need to handle OS signals here?
This is just a simple retry on any error.
There was a problem hiding this comment.
I mean any service which uses this client how do they propagate context to cancel retry etc on the client users receive OS signal or other interrupts?
There was a problem hiding this comment.
If we want to give the client the option to cancel the ongoing request. Then, we must give the cancel context to the client APIs; this will require modification for all clients; should we address this in the current PR?
may take that up in the following because the PR is already too big to review now.
There was a problem hiding this comment.
Once an event is in flight, I don't see how we could stop sending it. Even if there was a way to kill the connection it still doesn't the purpose.
IMO: Having a cancel context is complex for the client and may be an overkill too for this case.
There was a problem hiding this comment.
@chakravarthyvp I agree to keep it simple.
There was a problem hiding this comment.
@chakravarthyvp It is not just about in-flight msg but we should also gracefully close the connections with the raccoon server and wait for ack etc on sent msgs.
Why do you think it will be hard for clients to pass context? They will simply pass the context while initiating the connection.
It is ok to enhance it in another PR but I think it will be important to do it.
There was a problem hiding this comment.
@ravisuhag
For graceful shutdown, I suppose handling interrupts or context may not be needed. We should let the client have fine grained control on when to close than with the raccoon-client taking this call. For this, We could provide a close() method at the interface where we can close the connection internally and the client can call this close() as part of their shutdown process. wdyt?
Sure, we can take this up in the next PR for further discussions on this.
