Maintain block order after peer selection or exhaustion during peer streaming #258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ben-lerner
reviewed
Feb 13, 2017
| opts := newSessionTestAdminOptions(). | ||
| SetFetchSeriesBlocksMaxBlockRetries(2) | ||
| s, err := newSession(opts) | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
Let me update this file and these changes to reflect this.
xichen2020
added a commit
that referenced
this pull request
Feb 13, 2017
…treaming (#258) * Maintain block order after peer selection or exhaustion during peer streaming * Use require.NoError for segments of tests that must succeed to proceed
xichen2020
added a commit
that referenced
this pull request
Feb 13, 2017
* Format session errors with m3x/errors/.Errors (#257) * Maintain block order after peer selection or exhaustion during peer streaming (#258) * Maintain block order after peer selection or exhaustion during peer streaming * Use require.NoError for segments of tests that must succeed to proceed * Peer streaming only fails when unable to read data from any peer during merging reads (#259) * Peer streaming only fails when unable to read data from any peer during merging reads * Update comments * Fix tests * Disable integration tests for native pool testing * Remove blocks from peers' block lists if they are not eligible * Better comments * Add metrics to report final errors during peer streaming (#260) * Peer streaming only fails when unable to read data from any peer during merging reads * Update comments * Fix tests * Disable integration tests for native pool testing * Remove blocks from peers' block lists if they are not eligible * Better comments * Add metrics to report final errors during peer streaming * Update m3x dependency (#261)
xichen2020
added a commit
that referenced
this pull request
Feb 14, 2017
* Format session errors with m3x/errors/.Errors (#257) * Maintain block order after peer selection or exhaustion during peer streaming (#258) * Maintain block order after peer selection or exhaustion during peer streaming * Use require.NoError for segments of tests that must succeed to proceed * Peer streaming only fails when unable to read data from any peer during merging reads (#259) * Peer streaming only fails when unable to read data from any peer during merging reads * Update comments * Fix tests * Disable integration tests for native pool testing * Remove blocks from peers' block lists if they are not eligible * Better comments * Add metrics to report final errors during peer streaming (#260) * Peer streaming only fails when unable to read data from any peer during merging reads * Update comments * Fix tests * Disable integration tests for native pool testing * Remove blocks from peers' block lists if they are not eligible * Better comments * Add metrics to report final errors during peer streaming * Update m3x dependency (#261) * Flush last writes on host queue close * Close any times * Longer timeout
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc @robskillington @prateek @ben-lerner
This PR fixes an issue when removing blocks from peers after we select a block from multiple peer candidate or the peer list has been exhausted. In particular when a block is removed, we used to swap it with the last block and shrink the block list, thereby destroying invariant that the blocks are sorted by their start times. This in turn causes issues because we use the block start times to determine which peers can provide the block and relies on the block order to align the peers. When the block ordering is disrupted, it leads to the error case where a block is only selected from one peer, even though multiple peers are eligible to provide this block.
Also added two test cases to catch the error cases. Both cases are failing against HEAD and passing against this PR.