sync -- client cleanup
#1680
sync -- client cleanup
#1680
Conversation
| lastErr = err | ||
| time.Sleep(failedRequestSleepInterval) |
There was a problem hiding this comment.
Removed to avoid sleeping. It's not the end of the world to wait 10 ms to wake up in the event of context cancellation, but still seems like good practice to not sleep.
| // or [ctx] is canceled. | ||
| // Returns the peer's NodeID and response. | ||
| // It's safe to call this method multiple times concurrently. | ||
| func (c *client) get(ctx context.Context, request []byte) (ids.NodeID, []byte, error) { |
There was a problem hiding this comment.
In other places like this we return the node ID first so I did that here too
| func (c *networkClient) AppRequestFailed(_ context.Context, nodeID ids.NodeID, requestID uint32) error { | ||
| // Always returns nil because the engine considers errors | ||
| // returned from this function as fatal. | ||
| func (c *networkClient) AppRequestFailed( |
There was a problem hiding this comment.
I don't think it's necessary to rewrite the semantics of AppRequestFailed here
| if err := c.activeRequests.Acquire(ctx, 1); err != nil { | ||
| return nil, ErrAcquiringSemaphore | ||
| } | ||
| defer c.activeRequests.Release(1) |
There was a problem hiding this comment.
Moved releases for this semaphore to be in the two places we acquire from it
| handler := newResponseHandler() | ||
| c.outstandingRequestHandlers[requestID] = handler |
There was a problem hiding this comment.
can wait to register this until we've successfully sent the request
x/sync/client.go
Outdated
| // Return error at top of loop. | ||
| // Don't do it here to avoid duplicate code. | ||
| case <-time.After(failedRequestSleepInterval): | ||
| // TODO should we randomize this? |
There was a problem hiding this comment.
What benefit would randomization have?
We could consider adding exponential backoff.
There was a problem hiding this comment.
Just trying to prevent retry storms. I'll add exponential backoff in a follow up PR.
There was a problem hiding this comment.
Why this should be merged
Cleanup
How this works
Comments, nits and other small changes to improve readability and make the code easier to understand.
How this was tested
Existing UT.