Respect generational files in recoveryDiff #77695

fcofdez · 2021-09-14T12:58:50Z

Today MetadataSnapshot#recoveryDiff considers the .liv file as per-commit
rather than per-segment and often transfers them during peer recoveries and
snapshot restores. It also considers differences in .fnm, .dvd and .dvm
files as indicating a difference in the whole segment, even though these files
may be adjusted without changing the segment itself.

This commit adjusts this logic to attach these generational files to the
segments themselves, allowing Elasticsearch only to transfer them if they are
genuinely needed.

Closes #55142

This is basically the same as #55239 but updated

Today `MetadataSnapshot#recoveryDiff` considers the `.liv` file as per-commit rather than per-segment and often transfers them during peer recoveries and snapshot restores. It also considers differences in `.fnm`, `.dvd` and `.dvm` files as indicating a difference in the whole segment, even though these files may be adjusted without changing the segment itself. This commit adjusts this logic to attach these generational files to the segments themselves, allowing Elasticsearch only to transfer them if they are genuinely needed. Closes elastic#55142 Resolves an outstanding `//NORELEASE` action related to elastic#50999.

…y-liv-file

elasticmachine · 2021-09-14T12:58:52Z

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

fcofdez · 2021-09-14T15:16:42Z

@elasticmachine update branch

fcofdez · 2021-09-16T08:30:57Z

This is blocked by #77842 unless we find a workaround for source-only snapshots that share the segment id but whose content can be different.

…y-liv-file

fcofdez · 2021-09-27T11:53:42Z

@elasticmachine update branch

…y-liv-file

fcofdez · 2021-09-29T08:00:31Z

@elasticmachine update branch

fcofdez · 2021-09-29T15:45:23Z

@original-brownbear would you mind reviewing the bits around snapshot FileInfo serialization when you have the chance? 7f2b8bc I had to introduce some conditional serialization based on the repo version to account mixed clusters.

original-brownbear

LGTM as far as the snapshot related changes go :)

tlrx

LGTM - I left only minor comments that you can choose to address or not. Sorry for the delay

tlrx · 2021-10-04T08:44:45Z

server/src/main/java/org/elasticsearch/index/store/Store.java

+            for (StoreFileMetadata sourceFile : this) {
+                if (sourceFile.name().startsWith("_")) {
+                    final String segmentId = IndexFileNames.parseSegmentName(sourceFile.name());
+                    final long generation = IndexFileNames.parseGeneration(sourceFile.name());


Suggested change

final long generation = IndexFileNames.parseGeneration(sourceFile.name());

final boolean isGenerationalFile = IndexFileNames.parseGeneration(sourceFile.name()) > 0L;

tlrx · 2021-10-04T08:55:18Z

server/src/main/java/org/elasticsearch/index/store/Store.java

         */
-        public RecoveryDiff recoveryDiff(MetadataSnapshot recoveryTargetSnapshot) {
+        public RecoveryDiff recoveryDiff(final MetadataSnapshot targetSnapshot) {
+            final List<StoreFileMetadata> perCommitSourceFiles = new ArrayList<>();


I'd move the computation of perCommitSourceFiles and perSegmentSourceFiles just before the loop where it is used.

tlrx · 2021-10-04T09:12:48Z

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


            final ShardGeneration indexGeneration;
            final boolean writeShardGens = SnapshotsService.useShardGenerations(context.getRepositoryMetaVersion());
+            final boolean writeFileInfoWriterUUID = SnapshotsService.includeFileInfoWriterUUID(context.getRepositoryMetaVersion());


This one is always used as a String so maybe worth to declare it a String

tlrx · 2021-10-04T09:14:55Z

server/src/main/java/org/elasticsearch/index/store/StoreFileMetadata.java

+        // If we have the file contents, we directly compare the contents. This is useful to compare segment info
+        // files of source-only snapshots where the original segment info file shares the same id as the source-only
+        // segment info file but its contents are different.
+        if (hashEqualsContents()) {


Should we compute hashEqualsContents once is the constructor and stores it as a class member? It looks like every time a StoreFileMetadata is instanciated we use it.

tlrx · 2021-10-04T09:15:32Z

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

+    /**
+     * Writes blob with resolving the blob name using {@link #blobName} method.
+     * <p>
+     * The blob will optionally by compressed.


Suggested change

* The blob will optionally by compressed.

* The blob will optionally be compressed.

tlrx · 2021-10-04T09:16:31Z

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

+        final String blobName,
+        final boolean compress,
+        final Map<String, String> extraParams,
+        OutputStream outputStream


let's make this one final too

tlrx · 2021-10-04T09:18:23Z

server/src/test/java/org/elasticsearch/index/snapshots/blobstore/FileInfoTests.java

            XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON).prettyPrint();
-            BlobStoreIndexShardSnapshot.FileInfo.toXContent(info, builder);
+            boolean serializeWriterUUID = randomBoolean();
+            ToXContent.Params params = new ToXContent.MapParams(


Maybe also test the default behavior with an empty map

tlrx · 2021-10-04T09:27:37Z

server/src/test/java/org/elasticsearch/index/store/StoreTests.java

+            iwc.setMergePolicy(NoMergePolicy.INSTANCE);
+            iwc.setUseCompoundFile(random.nextBoolean());
+            iwc.setOpenMode(IndexWriterConfig.OpenMode.APPEND);
+            IndexWriter writer = new IndexWriter(store.directory(), iwc);


Not very important but IndexWriter implements AutoCloseable and can be used in try-with-resources blocks. IndexWriterConfig also commits on close so you can save few lines (but it's not used like this in other tests so 🤷).

tlrx · 2021-10-04T09:40:32Z

server/src/main/java/org/elasticsearch/index/store/StoreFileMetadata.java


+    private final BytesRef writerUuid;
+
    public StoreFileMetadata(String name, long length, String checksum, String writtenBy) {


Should we get rid of this ctor somehow? It's used in RecoveryFileChunkRequest as a way to carry the name/length/etc but those are serialized separately there and I wonder if that could introduce some bugs later if someone rely on writerUuid in recovery but it's never available there.

Good catch! Maybe we should serialize the writerUuid there too? It's a bit hacky but that's where we are today 🤔

Instead of adding the serialization of the writerUuid we could maybe just serialize a StoreFileMetadata. This can be done in a follow up though

…y-liv-file

This reverts commit c6f7a45.

fcofdez · 2021-10-05T07:51:27Z

@elasticmachine update branch

fcofdez · 2021-10-05T09:04:38Z

@elasticmachine run elasticsearch-ci/part-1
It was a known failure #78675

fcofdez · 2021-10-05T09:41:45Z

Thanks Armin and Tanguy!

Today `MetadataSnapshot#recoveryDiff` considers the `.liv` file as per-commit rather than per-segment and often transfers them during peer recoveries and snapshot restores. It also considers differences in `.fnm`, `.dvd` and `.dvm` files as indicating a difference in the whole segment, even though these files may be adjusted without changing the segment itself. This commit adjusts this logic to attach these generational files to the segments themselves, allowing Elasticsearch only to transfer them if they are genuinely needed. Closes elastic#55142 Backport of elastic#77695 Co-authored-by: David Turner <[email protected]>

Today `MetadataSnapshot#recoveryDiff` considers the `.liv` file as per-commit rather than per-segment and often transfers them during peer recoveries and snapshot restores. It also considers differences in `.fnm`, `.dvd` and `.dvm` files as indicating a difference in the whole segment, even though these files may be adjusted without changing the segment itself. This commit adjusts this logic to attach these generational files to the segments themselves, allowing Elasticsearch only to transfer them if they are genuinely needed. Closes #55142 Backport of #77695 Co-authored-by: David Turner <[email protected]>

…to 7.x

…78753)

DaveCTurner and others added 7 commits April 15, 2020 12:49

Imports

c159fc8

Merge branch 'master' into 2020-04-15-dont-copy-liv-file

14f0a3f

Merge branch 'master' into 2020-04-15-dont-copy-liv-file

b68c5d7

WIP add support for writer-assigned UUIDs introduced in Lucene 8.6

85e85ad

Merge remote-tracking branch 'origin/master' into 2020-04-15-dont-cop…

7f453b3

…y-liv-file

Remove dated comment

0edde82

fcofdez added >enhancement :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. v8.0.0 Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.16.0 labels Sep 14, 2021

Fix SnapshotsRecoveryPlannerServiceTests

968a8d5

fcofdez mentioned this pull request Sep 16, 2021

Source-only snapshots create a modified segment info file with the same id as the original segment #77842

Open

fcofdez added 2 commits September 23, 2021 17:17

Merge remote-tracking branch 'origin/master' into 2020-04-15-dont-cop…

a46ed7a

…y-liv-file

Compare contents directly when possible

da417ce

fcofdez force-pushed the 2020-04-15-dont-copy-liv-file branch from 115680b to da417ce Compare September 23, 2021 15:19

fcofdez added 3 commits September 28, 2021 13:47

Merge remote-tracking branch 'origin/master' into 2020-04-15-dont-cop…

9d6d0ac

…y-liv-file

Take into account version for FileInfo serialization

7f2b8bc

Merge remote-tracking branch 'origin/master' into 2020-04-15-dont-cop…

57af839

…y-liv-file

fcofdez force-pushed the 2020-04-15-dont-copy-liv-file branch from 12f94bb to 57af839 Compare September 28, 2021 15:42

Merge branch 'master' into 2020-04-15-dont-copy-liv-file

5cb099e

original-brownbear self-requested a review September 30, 2021 06:01

original-brownbear approved these changes Sep 30, 2021

View reviewed changes

fcofdez requested a review from tlrx October 1, 2021 11:00

tlrx approved these changes Oct 4, 2021

View reviewed changes

tlrx reviewed Oct 4, 2021

View reviewed changes

fcofdez added 4 commits October 4, 2021 15:20

Merge remote-tracking branch 'origin/master' into 2020-04-15-dont-cop…

90b8940

…y-liv-file

Review comments

3148a53

Compute hashEqualContents eagerly

c6f7a45

Revert "Compute hashEqualContents eagerly"

8529206

This reverts commit c6f7a45.

Merge branch 'master' into 2020-04-15-dont-copy-liv-file

2f86d08

fcofdez merged commit 310b4ac into elastic:master Oct 5, 2021

fcofdez added the auto-backport Automatically create backport pull requests when merged label Oct 5, 2021

fcofdez mentioned this pull request Oct 5, 2021

Respect generational files in recoveryDiff #55239

Closed

fcofdez added auto-backport Automatically create backport pull requests when merged and removed auto-backport Automatically create backport pull requests when merged labels Oct 5, 2021

fcofdez mentioned this pull request Oct 5, 2021

[7.x] Respect generational files in recoveryDiff #78707

Merged

fcofdez added a commit to fcofdez/elasticsearch that referenced this pull request Oct 6, 2021

Mute bwc test for backporting elastic#77695 to 7.x

dfc77ff

fcofdez added a commit that referenced this pull request Oct 6, 2021

Mute bwc test for backporting #77695 to 7.x (#78752)

253c53d

fcofdez added a commit to fcofdez/elasticsearch that referenced this pull request Oct 6, 2021

Re-enable BWC tests and adjust versions after elastic#77695 backport …

684a54a

…to 7.x

fcofdez added a commit that referenced this pull request Oct 6, 2021

Re-enable BWC tests and adjust versions after #77695 backport to 7.x (#…

ea27dd4

…78753)

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

	final long generation = IndexFileNames.parseGeneration(sourceFile.name());
	final boolean isGenerationalFile = IndexFileNames.parseGeneration(sourceFile.name()) > 0L;

	* The blob will optionally by compressed.
	* The blob will optionally be compressed.


		private final BytesRef writerUuid;

		public StoreFileMetadata(String name, long length, String checksum, String writtenBy) {

Respect generational files in recoveryDiff #77695

Respect generational files in recoveryDiff #77695

Uh oh!

Conversation

fcofdez commented Sep 14, 2021

Uh oh!

elasticmachine commented Sep 14, 2021

Uh oh!

fcofdez commented Sep 14, 2021

Uh oh!

fcofdez commented Sep 16, 2021

Uh oh!

fcofdez commented Sep 27, 2021

Uh oh!

fcofdez commented Sep 29, 2021

Uh oh!

fcofdez commented Sep 29, 2021

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

tlrx 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!

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!

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!

fcofdez commented Oct 5, 2021

Uh oh!

fcofdez commented Oct 5, 2021

Uh oh!

fcofdez commented Oct 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants