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

Better Incrementality for Snapshots of Unchanged Shards #52182

Conversation

original-brownbear
Copy link
Member

@original-brownbear original-brownbear commented Feb 11, 2020

Use sequence numbers to determine whether a shard has changed or not instead before falling back to comparing files to get incremental snapshots on primary fail-over.

@original-brownbear
Copy link
Member Author

@ywelsch @dnhatn Thanks for all the help here so far:

I incorporated feedback and the work in #52694 into this PR now. Instead of dealing with all the details of the global- and local-checkpoints being equal etc. in the repository this is now all happening upstream in the SnapshotShardsService.
The repository now simply accepts a string identifier for a shard's state that is then checked against existing file sets. The identifier is then either null if we don't have local == global checkpoint or else a string made up from the history- and forcemerge uuids and the current checkpoint (that should be unique in all cases I think).

=> I think this one is good for another round of reviews whenever you have some time :)

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

I've left one ask for a test, o.w. looking good.

import static org.hamcrest.Matchers.is;

@ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, numDataNodes = 0)
public class BlobStoreIncrementalityIT extends AbstractSnapshotIntegTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add a test that checks that force-merging is leading to another full snapshot?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing, how about this one b508fb3

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM

@original-brownbear
Copy link
Member Author

Thanks Yannick!

@original-brownbear original-brownbear merged commit 87c910b into elastic:master Mar 23, 2020
@original-brownbear original-brownbear deleted the better-incrementality-snapshot branch March 23, 2020 13:25
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Mar 23, 2020
Use sequence numbers and force merge UUID to determine whether a shard has changed or not instead before falling back to comparing files to get incremental snapshots on primary fail-over.
original-brownbear added a commit that referenced this pull request Mar 23, 2020
)

Use sequence numbers and force merge UUID to determine whether a shard has changed or not instead before falling back to comparing files to get incremental snapshots on primary fail-over.
@original-brownbear original-brownbear restored the better-incrementality-snapshot branch August 6, 2020 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement v7.7.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants