-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Automatic verification of all files that are being snapshotted with Snapshot/Restore #7159
Automatic verification of all files that are being snapshotted with Snapshot/Restore #7159
Conversation
@@ -396,8 +392,10 @@ protected BlobStoreIndexShardSnapshots buildBlobStoreIndexShardSnapshots(Immutab | |||
*/ | |||
public SnapshotContext(SnapshotId snapshotId, ShardId shardId, IndexShardSnapshotStatus snapshotStatus) { | |||
super(snapshotId, shardId); | |||
store = indicesService.indexServiceSafe(shardId.getIndex()).shardInjectorSafe(shardId.id()).getInstance(Store.class); | |||
IndexService indexService = indicesService.indexServiceSafe(shardId.getIndex()); | |||
store = indexService.shardInjectorSafe(shardId.id()).getInstance(Store.class); |
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.
can we cast to InternalIndexShard
and call #store()
on it? I think its cleaner....
it looks good to me, maybe @rmuir can have a look at that verifying index input? |
* mechanism that is used in some repository plugins (S3 for example). However, the checksum is only calculated on | ||
* the first read. All consecutive reads of the same data are not used to calculate the checksum. | ||
*/ | ||
static class VerifyingIndexInput extends ChecksumIndexInput { |
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.
This guy is fairly tricky: I know there are explicit tests for this indexinput, but do they test that seeking/re-reading does not impact the checksum computation?
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.
@rmuir I added a test to verify that, but I should probably rename readIndexInputFully into something like readIndexInputFullyWithRandomeSeeks to reflect that.
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.
Yes, that test takes care of my concerns. Maybe we can just add a comment to readBytes that our casts to 'int' are safe because seek catches us up? I stared hard at this thing, I dont see any problems, but it might help someone else. Thanks for doing this, I think it may be useful in the future for other purposes too (e.g. fold logic into ChecksumIndexInput).
looks good here. sorry for the delay in review! |
Adds automatic verification of all files that are being snapshotted. Closes elastic#5593
9ac7519
to
45ca777
Compare
Adds automatic verification of all files that are being snapshotted. Closes #5593