Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Peer query speculative fixes and improvements #1430

Merged
merged 2 commits into from
Aug 14, 2019

Conversation

woodsaj
Copy link
Member

@woodsaj woodsaj commented Aug 14, 2019

  • fixup context handling for intra-cluster requests. The function for handling requests was using a depreciated approach for cancelling requests.

  • update peerQuerySpeculative to retry other peers in a shardGroup when a request fails.

When using peerQuerySpeculative queries, if a request to a peer fails,
instead of failing and aborting all requests, simply retry the request
on another peer for the shard.
@@ -546,6 +549,19 @@ func (s *Server) peerQuerySpeculativeChan(ctx context.Context, data cluster.Trac
}

if resp.err != nil {
// check if there is another peer for this shardGroup. If so try it.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I originally didn't retry errors, is that many errors aren't effective to retry. Things like bad requests will still fail with the peers. I would be interested to see how often this is actually helpful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Between when cluster.MembersForSpeculativeQuery() returned the list of peers and when the requests where actually run, nodes could go offline. We have been seeing bursts of failures in clusters that handle a high volume of requests because of this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no instance should issue a request that is considered a bad request by a peer. if we see that, it is a bug to be fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussion on this topic lives in #985

@woodsaj woodsaj merged commit 6113e0f into master Aug 14, 2019
@woodsaj woodsaj deleted the peerQuerySpeculativeRetryOnError branch August 14, 2019 12:49
Dieterbe added a commit that referenced this pull request Sep 2, 2019
Dieterbe added a commit that referenced this pull request Sep 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants