-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Throw back replica local checkpoint on new primary #25452
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
Merged
jasontedor
merged 18 commits into
elastic:master
from
jasontedor:local-checkpoint-throwback
Jul 5, 2017
Merged
Changes from 8 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
2115f4a
Throw back replica local checkpoint on new primary
jasontedor 8f74e92
Checkstyle
jasontedor 5e9d79f
Fix order and beef up test
jasontedor 1e7cee9
Do not reset max seq no
jasontedor f33925d
Fix logging statement
jasontedor cbe568b
Revert accidental format changes
jasontedor 0174af4
Remove import
jasontedor 813032a
Merge branch 'master' into local-checkpoint-throwback
jasontedor 385c948
Add assertion
jasontedor ab8eeb3
Merge branch 'master' into local-checkpoint-throwback
jasontedor 93e751f
Fix test
jasontedor e8e4544
Fix tests
jasontedor 49742d4
Revert "Fix tests"
jasontedor 5064e8c
Revert "Fix test"
jasontedor 5640e2a
Revert "Add assertion"
jasontedor d1e0ec2
Special case
jasontedor e1bc5c7
Fix test
jasontedor 6d67289
Cleanup
jasontedor File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,7 @@ | |
| import java.util.stream.Collectors; | ||
| import java.util.stream.IntStream; | ||
|
|
||
| import static org.hamcrest.Matchers.empty; | ||
| import static org.hamcrest.Matchers.equalTo; | ||
| import static org.hamcrest.Matchers.isOneOf; | ||
|
|
||
|
|
@@ -236,4 +237,23 @@ public void testWaitForOpsToComplete() throws BrokenBarrierException, Interrupte | |
|
|
||
| thread.join(); | ||
| } | ||
|
|
||
| public void testResetCheckpoint() { | ||
| final int operations = 1024 - scaledRandomIntBetween(0, 1024); | ||
| int maxSeqNo = Math.toIntExact(SequenceNumbersService.NO_OPS_PERFORMED); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. neat, I did not know about this method |
||
| for (int i = 0; i < operations; i++) { | ||
| if (!rarely()) { | ||
| tracker.markSeqNoAsCompleted(i); | ||
| maxSeqNo = i; | ||
| } | ||
| } | ||
|
|
||
| final int localCheckpoint = | ||
| randomIntBetween(Math.toIntExact(SequenceNumbersService.NO_OPS_PERFORMED), Math.toIntExact(tracker.getCheckpoint())); | ||
| tracker.resetCheckpoint(localCheckpoint); | ||
| assertThat(tracker.getCheckpoint(), equalTo((long) localCheckpoint)); | ||
| assertThat(tracker.getMaxSeqNo(), equalTo((long) maxSeqNo)); | ||
| assertThat(tracker.processedSeqNo, empty()); | ||
| assertThat(tracker.generateSeqNo(), equalTo((long) (maxSeqNo + 1))); | ||
| } | ||
| } | ||
This file contains hidden or 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
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.
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.
As we are feeding this method the current global checkpoint, which could be still unknown, is it possible that we call
resetLocalCheckpointwithSequenceNumbersService.UNASSIGNED_SEQ_NO? If so, I think that that would be bad. The methodresetLocalCheckpointshould have an assertion, similar to the constructor. Also we need to make sure to special-case this.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 do not think this is possible after we address #25415. A newly created primary will update its local checkpoint to -1 and calculate a global checkpoint of -1. Replicas that recover from this primary will receive a global checkpoint of -1 that they would maintain if promoted. Similarly for relocation. Thus I think that we will never see -2 here.
I think we should only add an assertion here.
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.
To recap a discussion we had via another channel, we do have to worry about -2 here in the case when a primary on 5.x dies and a replica on 6.x is promoted and initiates are re-sync to another 6.x replica. I pushed a d1e0ec2.