Make SnapshotsInProgress Diffable #89619

original-brownbear · 2022-08-25T13:50:30Z

This is very important for #77466. Profiling showed that serializing snapshots-in-progress when there's a few snapshots with high shard count running takes a significant amount of CPU and heap for sending the full data structure over an over.
This PR adds diffing in the simplest way I could think of on top of the existing data structure.

closes #88732

…-sn-in-progress

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

original-brownbear · 2022-09-05T07:07:17Z

server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java

+         */
+        private record ByRepoDiff(
+            DiffableUtils.MapDiff<String, Entry, Map<String, Entry>> diffBySnapshotUUID,
+            DiffableUtils.MapDiff<String, Integer, Map<String, Integer>> positionDiff


Not the nicest solution ever to diffing a list that can see both entries change and move about in the list but the best I could come up with without refactoring things in this class.

Likewise, could we do that prep work first and avoid having to do this? If not, could we at least sprinkle a bunch of assertions around here to validate our assumptions about the diff (no gaps, no duplicates, etc)?

I think a big refactoring that avoids the current structure of a map of lists will be hard to pull off in the short-term. It's a massive change-set that would be required here and I'd much rather do it in increments.
The beauty of the code here is that at least for the positions we need no such assertions since List.of(...) will be unforgiving to gaps. So if we take the size of the array from the uuid->snaphot map and then use the positions for the iteration we can't really run into trouble ever can we?

Ok - I'll suggest a couple of extra assertions in some following comments.

server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java

elasticsearchmachine · 2022-09-05T11:58:23Z

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

elasticsearchmachine · 2022-09-05T11:59:00Z

Hi @original-brownbear, I've created a changelog YAML for you.

idegtiarenko · 2022-09-05T14:48:33Z

server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java

+
+        @Override
+        public Version getMinimalSupportedVersion() {
+            return Version.CURRENT.minimumCompatibilityVersion();


Should it be DIFFABLE_VERSION?

I don't think so because we were always allegedly "diffable" but used a non-diff. That's why I had to add the BwC new SimpleDiffable.CompleteDiff<>(after).writeTo(out); below. So in a sense this is just a wire format change from the perspective of the checks on this I think.

DaveCTurner

Tests look ok now, I left a couple of other ideas.

server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java

DaveCTurner · 2022-09-05T14:46:53Z

server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java

+         */
+        private record ByRepoDiff(
+            DiffableUtils.MapDiff<String, Entry, Map<String, Entry>> diffBySnapshotUUID,
+            DiffableUtils.MapDiff<String, Integer, Map<String, Integer>> positionDiff


Likewise, could we do that prep work first and avoid having to do this? If not, could we at least sprinkle a bunch of assertions around here to validate our assumptions about the diff (no gaps, no duplicates, etc)?

…-sn-in-progress

original-brownbear · 2022-09-12T16:51:27Z

@DaveCTurner ping in case you have a second, this would be quite nice to have in for our snapshot benchmarking. It's a surprisingly large speedup :)

DaveCTurner

A few more small comments

server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java

DaveCTurner · 2022-09-13T08:05:06Z

server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java

+        }
+
+        @SuppressWarnings("unchecked")
+        EntryDiff(Entry before, Entry after) {


I think we should be explicit about the fields that must not change between before and after here, at least asserting that they're the same but maybe also protecting against that kind of bug in production too.

Makes sense, I added actual exception throwing in production. If we ever run into a bug here, it's still better to stall everything and (maybe) for a full cluster restart to fix things than to corrupt the repo IMO :)

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

DaveCTurner · 2022-09-13T08:13:41Z

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

+                        entries.set(i, mutateEntryWithLegalChange(entry));
                    }
                }
                updatedInstance = updatedInstance.withUpdatedEntriesForRepo(perRepoEntries.get(0).repository(), entries);


Can we also reorder the entries for one or more repositories? Really just to give the ByRepoDiff stuff a proper workout.

Jup added shuffling the list. Had to change the way we generate the repo name for that. It was a random string per snapshot and we'd effectively never see collisions there. Now we actually have multiple snapshots per repo here and the position reordering is covered by tests.

👍 sounds good

DaveCTurner · 2022-09-13T08:14:41Z

server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java

+         */
+        private record ByRepoDiff(
+            DiffableUtils.MapDiff<String, Entry, Map<String, Entry>> diffBySnapshotUUID,
+            DiffableUtils.MapDiff<String, Integer, Map<String, Integer>> positionDiff


Ok - I'll suggest a couple of extra assertions in some following comments.

server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java

original-brownbear · 2022-09-13T11:02:30Z

Thanks @DaveCTurner all points addressed I think :)

DaveCTurner

LGTM

original-brownbear · 2022-09-13T11:33:53Z

Thanks David!

original-brownbear added 13 commits August 23, 2022 16:13

lets try this

1fae874

faster

bb926e6

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

e16e333

back out unrelated

a13707b

bck

8bd7689

fixed up

1a5e668

cheaper map

84845c7

less redundant diffing

1c83e07

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

9148ee4

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

85c128f

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

e78fd53

tet fix

e4360d2

smaller serialization

0eaef8a

original-brownbear added WIP :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Aug 25, 2022

elasticsearchmachine added the v8.5.0 label Aug 25, 2022

original-brownbear added 11 commits August 26, 2022 13:23

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

3a77f18

bck

c73975b

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

15056f9

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

03876a3

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

c44f8ae

fix BwC

a5bf01c

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

55eb691

some docs

107c6e3

Merge remote-tracking branch 'origin/nicer-sn-in-progress' into nicer…

2ba10fe

…-sn-in-progress

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

f2750e6

nicer

d482623

original-brownbear commented Sep 5, 2022

View reviewed changes

server/src/test/java/org/elasticsearch/snapshots/SnapshotsInProgressSerializationTests.java Show resolved Hide resolved

original-brownbear commented Sep 5, 2022

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java Show resolved Hide resolved

original-brownbear added 2 commits September 5, 2022 13:26

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

67d2efa

CR: split out legal mutations

852cf9d

original-brownbear removed the WIP label Sep 5, 2022

elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Sep 5, 2022

original-brownbear added the >enhancement label Sep 5, 2022

Update docs/changelog/89619.yaml

9f9995c

idegtiarenko reviewed Sep 5, 2022

View reviewed changes

DaveCTurner reviewed Sep 5, 2022

View reviewed changes

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

9197d3e

original-brownbear mentioned this pull request Sep 6, 2022

Refactor SnapshotsInProgress to simplified constructors #89813

Merged

original-brownbear added 2 commits September 6, 2022 13:49

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

bb91620

Merge remote-tracking branch 'origin/nicer-sn-in-progress' into nicer…

c370bd8

…-sn-in-progress

original-brownbear requested review from DaveCTurner and idegtiarenko September 6, 2022 12:34

original-brownbear mentioned this pull request Sep 7, 2022

Limit the number of concurrent shard snapshots #89826

Open

DaveCTurner reviewed Sep 13, 2022

View reviewed changes

original-brownbear added 3 commits September 13, 2022 10:17

Merge remote-tracking branch 'elastic/main' into nicer-sn-in-progress

4bbe6a2

CR: comments

107eeac

verify diffable

97c71b1

original-brownbear requested a review from DaveCTurner September 13, 2022 11:02

DaveCTurner approved these changes Sep 13, 2022

View reviewed changes

original-brownbear merged commit b69d1bd into elastic:main Sep 13, 2022

original-brownbear deleted the nicer-sn-in-progress branch September 13, 2022 11:34

DaveCTurner mentioned this pull request Dec 9, 2022

Avoid capturing cluster state in TBbNA #92255

Merged

original-brownbear restored the nicer-sn-in-progress branch April 18, 2023 20:51

Make SnapshotsInProgress Diffable #89619

Make SnapshotsInProgress Diffable #89619

Uh oh!

Conversation

original-brownbear commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Sep 5, 2022

Uh oh!

elasticsearchmachine commented Sep 5, 2022

Uh oh!

idegtiarenko Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Sep 12, 2022

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

original-brownbear commented Sep 13, 2022

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Sep 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

original-brownbear commented Aug 25, 2022 •

edited

Loading

idegtiarenko Sep 5, 2022 •

edited

Loading