Track Snapshot Version in RepositoryData #50930

original-brownbear · 2020-01-13T17:02:00Z

Add tracking of snapshot versions to RepositoryData to make BwC logic more efficient.
Follow up to #50853

Add tracking of snapshot versions to `RepositoryData` to make BwC logic more efficient. Follow up to elastic#50853

elasticmachine · 2020-01-13T17:02:03Z

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

original-brownbear · 2020-01-13T19:49:18Z

Jenkins run elasticsearch-ci/1

original-brownbear · 2020-01-13T22:18:13Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

        // Step 2: Write new index-N blob to repository and update index.latest
        setPendingStep.whenComplete(newGen -> threadPool().executor(ThreadPool.Names.SNAPSHOT).execute(ActionRunnable.wrap(listener, l -> {
+            // BwC logic: Load snapshot version information if any snapshot is missing a version in RepositoryData so that the new
+            // RepositoryData contains a version for every snapshot
+            final List<SnapshotId> snapshotIdsWithoutVersion = repositoryData.getSnapshotIds().stream().filter(


It is somewhat annoying that we have to load all SnapshotInfo here one-off but I think it's still the best solution available.
Ideally, I was hoping to be able to just work from the assumption that if no version is set for a snapshot then it must be from before 8.0/7.6. But I gave up on that idea since it breaks as soon as some older version cluster (for whatever reason), takes a snapshot and removes all the version fields when writing out a new index-N (maybe that's the wrong trade off though, glad to hear opinions on this :) ... obviously you could argue that loading all the SnapshotInfo is too high a price to pay just so the repo can move to the new metadata version earlier, but IMO even on S3 etc. loading 100 snapshots or so won't take all that long or cost any meaningful amount of $$$).

original-brownbear · 2020-01-13T22:20:05Z

server/src/test/java/org/elasticsearch/snapshots/CorruptedBlobStoreRepositoryIT.java

@@ -263,11 +271,24 @@ public void testHandlingMissingRootLevelSnapshotMetadata() throws Exception {
        logger.info("--> delete root level snapshot metadata blob for snapshot [{}]", snapshotToCorrupt);
        Files.delete(repo.resolve(String.format(Locale.ROOT, BlobStoreRepository.SNAPSHOT_NAME_FORMAT, snapshotToCorrupt.getUUID())));

+        logger.info("--> strip version information from index-N blob");
+        final RepositoryData withoutVersions = new RepositoryData(repositoryData.getGenId(),


Obviously now that we're writing the version field to RepositoryData using shard generations to identify having any pre-7.6 snapshots or not goes away and we have to fake an old-style snapshot by stripping out the version fields manually.

original-brownbear · 2020-01-13T22:31:38Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

+                        new ActionListener<>() {
+                            @Override
+                            public void onResponse(Collection<Void> voids) {
+                                logger.info("Successfully loaded all snapshot's version information for {} from snapshot metadata",


info IMO since it's a one time thing and it would be nice to have some marker of the "upgrade" in the logs.

Given that snapshotIdsWithoutVersion can be very long. I was wondering if we should just display the size of snapshotIdsWithoutVersion here in the info logging.

I think it would be nice to have the concrete ids of the snapshots to help debug situations where users might be writing to the repo from various ES versions (that's my main motivation to have this). I figured the list isn't going to be so long that it would create real issues due to the log line length so worst case it's a bit of an annoyance right?

I think it would be nice to have the concrete ids of the snapshots to help debug situations where users might be writing to the repo from various ES versions (that's my main motivation to have this)

Why do the exact names and versions matter in that case? Isn't it sufficient to know that some snapshots got version-tagged that previously were not?

Versions I don't care about much and the exact names aren't important either, but it would be nice to see if the list of snapshots changed if this gets logged repeatedly.
Especially on Cloud just having just the size which might be effectively constant over time seems like it could create an annoying situation when debugging :)

Perhaps use firstListElementsToCommaDelimitedString then (see AllocationService)?

Ah nice thanks => reused in 02e8c4e (can probably move that method to a more appropriate place in a follow up)

original-brownbear · 2020-01-13T22:32:37Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

+                            public void onFailure(Exception e) {
+                                logger.warn("Failure when trying to load missing version information from snapshot metadata", e);
+                            }
+                        }, () -> filterRepositoryDataStep.onResponse(repositoryData.withVersions(updatedVersionMap))),


Not great but just like in last week's discussion of SnapshotsService not introducing any new breakage here so we take whatever version information we can get and run with it even on exceptions.

…sion-in-index-N

original-brownbear · 2020-01-14T08:14:08Z

@ywelsch the test failure in a previous run here seems like it might be interesting to you:

  1> [2020-01-13T22:49:34,516][INFO ][o.e.s.DedicatedClusterSnapshotRestoreIT] [testMasterShutdownDuringSnapshot] -->  creating repository
  1> [2020-01-13T22:49:34,517][INFO ][o.e.r.RepositoriesService] [node_tm0] put repository [test-repo]
  1> [2020-01-13T22:49:34,528][ERROR][o.e.g.G.AsyncLucenePersistedState] [node_td2] Exception occurred when storing new meta data
  1> org.elasticsearch.common.util.concurrent.EsRejectedExecutionException: rejected execution of org.elasticsearch.gateway.GatewayMetaState$AsyncLucenePersistedState$1@13966630 on EsThreadPoolExecutor[name = node_td2/AsyncLucenePersistedState#updateTask, queue capacity = 1, org.elasticsearch.common.util.concurrent.EsThreadPoolExecutor@17a9f09a[Running, pool size = 1, active threads = 1, queued tasks = 1, completed tasks = 0]]
  1> 	at org.elasticsearch.common.util.concurrent.EsAbortPolicy.rejectedExecution(EsAbortPolicy.java:48) ~[main/:?]
  1> 	at java.util.concurrent.ThreadPoolExecutor.reject(ThreadPoolExecutor.java:825) ~[?:?]
  1> 	at java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1355) ~[?:?]
  1> 	at org.elasticsearch.common.util.concurrent.EsThreadPoolExecutor.execute(EsThreadPoolExecutor.java:84) [main/:?]
  1> 	at org.elasticsearch.gateway.GatewayMetaState$AsyncLucenePersistedState.scheduleUpdate(GatewayMetaState.java:316) [main/:?]
  1> 	at org.elasticsearch.gateway.GatewayMetaState$AsyncLucenePersistedState.setLastAcceptedState(GatewayMetaState.java:308) [main/:?]
  1> 	at org.elasticsearch.cluster.coordination.CoordinationState.handlePublishRequest(CoordinationState.java:345) [main/:?]
  1> 	at org.elasticsearch.cluster.coordination.Coordinator.handlePublishRequest(Coordinator.java:316) [main/:?]
  1> 	at org.elasticsearch.cluster.coordination.PublicationTransportHandler.acceptState(PublicationTransportHandler.java:405) [main/:?]
  1> 	at org.elasticsearch.cluster.coordination.PublicationTransportHandler.handleIncomingPublishRequest(PublicationTransportHandler.java:385) [main/:?]
  1> 	at org.elasticsearch.cluster.coordination.PublicationTransportHandler.lambda$new$0(PublicationTransportHandler.java:96) [main/:?]
  1> 	at org.elasticsearch.transport.RequestHandlerRegistry.processMessageReceived(RequestHandlerRegistry.java:63) [main/:?]
  1> 	at org.elasticsearch.transport.InboundHandler$RequestHandler.doRun(InboundHandler.java:262) [main/:?]
  1> 	at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:688) [main/:?]
  1> 	at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37) [main/:?]
  1> 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
  1> 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
  1> 	at java.lang.Thread.run(Thread.java:834) [?:?]

even though a snapshot test failed here it looks like this is related to the new CS persistence layer instead of my changes.

ywelsch

Left one comment, looking good o.w.

ywelsch · 2020-01-14T13:53:07Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

+                        new ActionListener<>() {
+                            @Override
+                            public void onResponse(Collection<Void> voids) {
+                                logger.info("Successfully loaded all snapshot's version information for {} from snapshot metadata",


Given that snapshotIdsWithoutVersion can be very long. I was wondering if we should just display the size of snapshotIdsWithoutVersion here in the info logging.

…sion-in-index-N

original-brownbear · 2020-01-14T16:14:44Z

Thanks Yannick!

Add tracking of snapshot versions to RepositoryData to make BwC logic more efficient. Follow up to elastic#50853

* Track Snapshot Version in RepositoryData (#50930) Add tracking of snapshot versions to RepositoryData to make BwC logic more efficient. Follow up to #50853

Add tracking of snapshot versions to RepositoryData to make BwC logic more efficient. Follow up to elastic#50853

Track Snapshot Version in RepositoryData

e99eaea

Add tracking of snapshot versions to `RepositoryData` to make BwC logic more efficient. Follow up to elastic#50853

original-brownbear added >non-issue :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.6.0 labels Jan 13, 2020

shorter diff

6f45d07

original-brownbear added the WIP label Jan 13, 2020

fix test

386f0c3

original-brownbear removed the WIP label Jan 13, 2020

original-brownbear commented Jan 13, 2020

View reviewed changes

original-brownbear added 2 commits January 13, 2020 23:29

nicer

4ca7e7c

nicer

86ef75f

original-brownbear commented Jan 13, 2020

View reviewed changes

Merge remote-tracking branch 'elastic/master' into store-snapshot-ver…

eb22ebb

…sion-in-index-N

original-brownbear requested review from ywelsch and tlrx January 14, 2020 12:05

ywelsch approved these changes Jan 14, 2020

View reviewed changes

original-brownbear added 2 commits January 14, 2020 15:56

Merge remote-tracking branch 'elastic/master' into store-snapshot-ver…

99872cb

…sion-in-index-N

nicer logging

02e8c4e

original-brownbear merged commit e349c5e into elastic:master Jan 14, 2020

original-brownbear deleted the store-snapshot-version-in-index-N branch January 14, 2020 16:15

original-brownbear mentioned this pull request Jan 14, 2020

Track Snapshot Version in RepositoryData (#50930) #50989

Merged

SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020

Track Snapshot Version in RepositoryData (elastic#50930)

94e071d

Add tracking of snapshot versions to RepositoryData to make BwC logic more efficient. Follow up to elastic#50853

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track Snapshot Version in RepositoryData #50930

Track Snapshot Version in RepositoryData #50930

original-brownbear commented Jan 13, 2020

elasticmachine commented Jan 13, 2020

original-brownbear commented Jan 13, 2020

original-brownbear Jan 13, 2020

original-brownbear Jan 13, 2020

original-brownbear Jan 13, 2020

ywelsch Jan 14, 2020

original-brownbear Jan 14, 2020

ywelsch Jan 14, 2020

original-brownbear Jan 14, 2020

ywelsch Jan 14, 2020

original-brownbear Jan 14, 2020

original-brownbear Jan 13, 2020

original-brownbear commented Jan 14, 2020

ywelsch left a comment

ywelsch Jan 14, 2020

original-brownbear commented Jan 14, 2020

Track Snapshot Version in RepositoryData #50930

Track Snapshot Version in RepositoryData #50930

Conversation

original-brownbear commented Jan 13, 2020

elasticmachine commented Jan 13, 2020

original-brownbear commented Jan 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Jan 14, 2020

ywelsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Jan 14, 2020