Add the ability to fetch the latest successful shard snapshot by fcofdez · Pull Request #75080 · elastic/elasticsearch

fcofdez · 2021-07-07T16:27:22Z

This commit adds a new master transport action TransportGetShardSnapshotAction
that allows getting the last successful snapshot for a particular
shard in a set of repositories. It deals with the different
implementation details around BwC for repositories.

Relates #73496

fcofdez · 2021-07-08T09:26:44Z

server/src/main/java/org/elasticsearch/repositories/IndexSnapshotsService.java

We could take into account PARTIAL snapshots too, the implementation would be a little less straightforward but we could consider it.

I agree to defer this to a follow-up after we have done other tasks, this refinement can be done while benchmarking.

This commit adds a new master transport action TransportGetShardSnapshotAction that allows getting the last successful snapshot for a particular shard in a set of repositories. It deals with the different implementation details around BwC for repositories.

elasticmachine · 2021-07-08T10:42:06Z

Pinging @elastic/es-distributed (Team:Distributed)

henningandersen

Had a quick look only, added a couple of comments.

I would like to see added wire serialization tests too for the new serializable classes. See: AbstractWireSerializingTestCase

henningandersen · 2021-07-09T12:02:22Z

.../elasticsearch/action/admin/cluster/snapshots/get/shard/TransportGetShardSnapshotAction.java

+        );
+
+        for (String repository : repositories) {
+            indexSnapshotsService.getLatestSuccessfulSnapshotForShard(


I think this could run in parallel on all the repos. I am guessing we will only need one repo in reality so it might not matter. But I would prefer to go serially through the repos and early terminate when a good snapshot is found. Or to just remove the multi-repo support, we can always add that if another use comes along.

The point of the multi-repo support is to avoid needing to implement a notion of a "default repo", we can simply look at all available repos. Moreover I don't think we can early-terminate since we're searching for the latest snapshot.

However I do agree that we shouldn't fan out to all the repos at once like this, working one-at-a-time would be better.

henningandersen · 2021-07-09T12:05:37Z

server/src/main/java/org/elasticsearch/repositories/IndexSnapshotsService.java

I agree to defer this to a follow-up after we have done other tasks, this refinement can be done while benchmarking.

DaveCTurner

Looks good, I left mostly small comments.

.../java/org/elasticsearch/action/admin/cluster/snapshots/get/shard/GetShardSnapshotAction.java

DaveCTurner · 2021-07-09T14:50:30Z

...r/src/main/java/org/elasticsearch/index/snapshots/blobstore/BlobStoreIndexShardSnapshot.java

     * Information about snapshotted file
     */
-    public static class FileInfo {
+    public static class FileInfo implements Writeable {


Are we really going to need the details of the files in the response? I think it would make more sense for the master to find a good snapshot and pass its ID onto the data node is, but then have the data node read these details itself.

Addressed in d0e1ce0. I was wondering if we should parse the latest commit in the snapshot to get the max seqNo and local checkpoint and include that in the response.

Not sure, but I think we can leave that to the data nodes.

server/src/internalClusterTest/java/org/elasticsearch/repositories/IndexSnapshotsServiceIT.java

...java/org/elasticsearch/action/admin/cluster/snapshots/get/shard/GetShardSnapshotRequest.java

.../elasticsearch/action/admin/cluster/snapshots/get/shard/TransportGetShardSnapshotAction.java

DaveCTurner · 2021-07-09T15:01:57Z

.../elasticsearch/action/admin/cluster/snapshots/get/shard/TransportGetShardSnapshotAction.java

+        );
+
+        for (String repository : repositories) {
+            indexSnapshotsService.getLatestSuccessfulSnapshotForShard(


The point of the multi-repo support is to avoid needing to implement a notion of a "default repo", we can simply look at all available repos. Moreover I don't think we can early-terminate since we're searching for the latest snapshot.

However I do agree that we shouldn't fan out to all the repos at once like this, working one-at-a-time would be better.

server/src/main/java/org/elasticsearch/repositories/ShardSnapshotInfo.java

server/src/main/java/org/elasticsearch/repositories/IndexSnapshotsService.java

fcofdez · 2021-07-12T17:51:50Z

Thanks for the review @DaveCTurner ! I've tackled your comments. Let me know if you think we should change something else 👍

original-brownbear

Just a few random points, mainly around testing. Looking just fine otherwise :)

server/src/internalClusterTest/java/org/elasticsearch/repositories/IndexSnapshotsServiceIT.java

...java/org/elasticsearch/action/admin/cluster/snapshots/get/shard/GetShardSnapshotRequest.java

.../elasticsearch/action/admin/cluster/snapshots/get/shard/TransportGetShardSnapshotAction.java

...java/org/elasticsearch/action/admin/cluster/snapshots/get/shard/GetShardSnapshotRequest.java

DaveCTurner · 2021-07-20T14:12:43Z

server/src/main/java/org/elasticsearch/repositories/ShardSnapshotInfo.java

+
+        this.indexId = indexId;
+        this.shardId = shardId;
+        this.snapshotInfo = snapshotInfo;


AFAICT this is the complete SnapshotInfo, including all indices and shard failures (including stack traces) and so on. That might be pretty sizeable, and mostly not relevant to the shard in question. Can we just pick out the bits we need?

...arch/action/admin/cluster/snapshots/get/shard/GetShardSnapshotRequestSerializationTests.java

DaveCTurner · 2021-07-20T14:40:11Z

...r/src/main/java/org/elasticsearch/index/snapshots/blobstore/BlobStoreIndexShardSnapshot.java

     * Information about snapshotted file
     */
-    public static class FileInfo {
+    public static class FileInfo implements Writeable {


Not sure, but I think we can leave that to the data nodes.

DaveCTurner

I left three pretty small comments; apart from that this all looks good.

original-brownbear

+1 to David's open points, LGTM otherwise no need for another round from me I think :)

fcofdez · 2021-07-21T15:57:53Z

@elasticmachine run elasticsearch-ci/part-1
(Unrelated failure)

fcofdez · 2021-07-21T17:38:44Z

@elasticmachine run elasticsearch-ci/part-1
(Unrelated failure)

DaveCTurner

LGTM (one more tiny suggestion). Great work @fcofdez 👍

DaveCTurner · 2021-07-22T07:23:10Z

...java/org/elasticsearch/action/admin/cluster/snapshots/get/shard/GetShardSnapshotRequest.java

+    GetShardSnapshotRequest(List<String> repositories, ShardId shardId) {
+        assert repositories.isEmpty() == false;
+        assert repositories.stream().noneMatch(Objects::isNull);
+        assert repositories.size() == 1 && repositories.get(0).equals(ALL_REPOSITORIES)


nit:

Suggested change

assert repositories.size() == 1 && repositories.get(0).equals(ALL_REPOSITORIES)

assert repositories.size() == 1

(if not, I'd prefer having clarifying parentheses, I find it trappy to rely on && binding more tightly than ||)

fcofdez · 2021-07-22T11:55:49Z

Thanks all for the review!

This commit adds a new master transport action TransportGetShardSnapshotAction that allows getting the last successful snapshot for a particular shard in a set of repositories. It deals with the different implementation details around BwC for repositories. Relates elastic#73496 Backport of elastic#75080

This commit adds a new master transport action TransportGetShardSnapshotAction that allows getting the last successful snapshot for a particular shard in a set of repositories. It deals with the different implementation details around BwC for repositories. Relates #73496 Backport of #75080

…c#75080) This commit adds a new master transport action TransportGetShardSnapshotAction that allows getting the last successful snapshot for a particular shard in a set of repositories. It deals with the different implementation details around BwC for repositories. Relates elastic#73496

elasticsearchmachine added the v8.0.0 label Jul 7, 2021

fcofdez force-pushed the fetch-info-recovery branch 2 times, most recently from 33fb9db to e39267f Compare July 8, 2021 08:47

fcofdez commented Jul 8, 2021

View reviewed changes

fcofdez force-pushed the fetch-info-recovery branch from e39267f to c244436 Compare July 8, 2021 09:33

fcofdez added v7.15.0 :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed Meta label for distributed team. labels Jul 8, 2021

fcofdez marked this pull request as ready for review July 8, 2021 10:42

fcofdez requested review from DaveCTurner and original-brownbear July 8, 2021 10:42

henningandersen reviewed Jul 9, 2021

View reviewed changes

DaveCTurner reviewed Jul 9, 2021

View reviewed changes

fcofdez added 10 commits July 12, 2021 16:17

Add wire serialization tests

73eaa9c

Merge remote-tracking branch 'origin/master' into fetch-info-recovery

306a733

Fetch shard snapshot info sequentially

2b538db

Use Optional.empty() instead of null

6363c6b

Don't serialize SnapshotFiles

d0e1ce0

More review comments

e8fd26b

Clarify comment

73cccf9

Merge remote-tracking branch 'origin/master' into fetch-info-recovery

5f1e053

Add extra test

8b7a1f6

Missing constant

afeea5e

fcofdez requested a review from DaveCTurner July 12, 2021 17:51

Merge remote-tracking branch 'origin/master' into fetch-info-recovery

c756833

original-brownbear reviewed Jul 19, 2021

View reviewed changes

Address review comments

aa13abb

Merge remote-tracking branch 'origin/master' into fetch-info-recovery

25a7b70

fcofdez requested a review from original-brownbear July 20, 2021 14:06

DaveCTurner reviewed Jul 20, 2021

View reviewed changes

original-brownbear approved these changes Jul 21, 2021

View reviewed changes

fcofdez added 2 commits July 21, 2021 17:22

Address review comments

c661878

Merge remote-tracking branch 'origin/master' into fetch-info-recovery

8c300f1

DaveCTurner approved these changes Jul 22, 2021

View reviewed changes

fcofdez added 2 commits July 22, 2021 11:31

Adjust assertion

eb762b5

Merge remote-tracking branch 'origin/master' into fetch-info-recovery

aa58a89

fcofdez merged commit 90148fe into elastic:master Jul 22, 2021

fcofdez mentioned this pull request Jul 22, 2021

[7.x] Add the ability to fetch the latest successful shard snapshot #75627

Merged

jakelandis added v8.0.0-alpha1 v8.0.0 and removed v8.0.0 v8.0.0-alpha1 labels Jul 26, 2021

fcofdez mentioned this pull request Jul 29, 2021

Replicate or relocate data via snapshot #73496

Closed

8 tasks

joegallo added the >enhancement label Sep 22, 2021

	assert repositories.size() == 1 && repositories.get(0).equals(ALL_REPOSITORIES)
	assert repositories.size() == 1

Conversation

fcofdez commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 8, 2021

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fcofdez commented Jul 12, 2021

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

fcofdez commented Jul 21, 2021

Uh oh!

fcofdez commented Jul 21, 2021

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fcofdez commented Jul 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

fcofdez commented Jul 7, 2021 •

edited

Loading