Skip to content
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

storage: release preemptive snapshot after remote apply #10491

Conversation

petermattis
Copy link
Collaborator

@petermattis petermattis commented Nov 6, 2016

Previously we were releasing the snapshot (i.e. calling
Replica.CloseOutSnap()) when the ChangeReplicas operation
completed. Now we release the snapshot as soon as the remote node has
applied it. This is important to allow other ranges to make progress
which might be required for the current ChangeReplicas operation to
complete.

Fixes #10483
Fixes #10306
See #10409


This change is Reviewable

@petermattis
Copy link
Collaborator Author

Perhaps we don't need #10482 as this PR also fixes #10409.

Not quite sure how to test this. Thoughts?

@petermattis petermattis force-pushed the pmattis/release-snapshot-after-remote-apply branch from 8fdadf0 to 1710cfd Compare November 6, 2016 16:35
@tamird
Copy link
Contributor

tamird commented Nov 6, 2016

:lgtm:

Perhaps you could simulate exactly the problem in #10409? We'll need a know to inject a raft log truncation after the first range on the second node receives its pre-emptive snapshot and before the ensuing conf change.


Reviewed 1 of 1 files at r1.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@petermattis
Copy link
Collaborator Author

Perhaps you could simulate exactly the problem in #10409?

I should have been clearer that I made several attempts to test this change, specifically trying to simulate the problem in #10409. I'll give it another go tomorrow.

@petermattis petermattis force-pushed the pmattis/release-snapshot-after-remote-apply branch from 1710cfd to 54e28cc Compare November 7, 2016 01:09
@petermattis petermattis force-pushed the pmattis/release-snapshot-after-remote-apply branch from 54e28cc to 287e4fe Compare November 7, 2016 01:45
@petermattis
Copy link
Collaborator Author

Added a test that directly tests that the preemptive snapshot is released. The test fails on master and passes on this PR.

Previously we were releasing the snapshot (i.e. calling
`Replica.CloseOutSnap()`) when the ChangeReplicas operation
completed. Now we release the snapshot as soon as the remote node has
applied it. This is important to allow other ranges to make progress
which might be required for the current ChangeReplicas operation to
complete.

Fixes cockroachdb#10483
Fixes cockroachdb#10306
See cockroachdb#10409
@petermattis petermattis force-pushed the pmattis/release-snapshot-after-remote-apply branch from 287e4fe to efa4624 Compare November 7, 2016 01:54
@bdarnell
Copy link
Contributor

bdarnell commented Nov 7, 2016

:lgtm:

I think we should keep #10482; the discrepancy between the Commit index and the current membership is a concern even when it doesn't lead to snapshots getting stuck.


Review status: 1 of 2 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Nov 7, 2016

Reviewed 1 of 1 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@petermattis
Copy link
Collaborator Author

I think we should keep #10482; the discrepancy between the Commit index and the current membership is a concern even when it doesn't lead to snapshots getting stuck.

I agree. I've already merged #10482.

@petermattis petermattis merged commit 80bef08 into cockroachdb:master Nov 7, 2016
@petermattis petermattis deleted the pmattis/release-snapshot-after-remote-apply branch November 7, 2016 13:25
pav-kv pushed a commit to pav-kv/cockroach that referenced this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants