Add flag to MetadataDiff to make noop diffs cheaper#90101

Merged

original-brownbear merged 3 commits intoelastic:mainfrom

original-brownbear:skip-noop-metadata-diff

Sep 19, 2022

Contributor

original-brownbear commented Sep 15, 2022 •

edited

Loading

No need to rebuild a new metadata or send needless bytes if we know before == after. This should save quite a bit of work on data nodes during non-metadata state updates.

relates #77466


          Add flag to MetadataDiff to make noop diffs cheaper

No need to rebuild a new metadata or send needless bytes if we know
`before == after`. This should save quite a bit of work on data
nodes during non-metadata state updates.

original-brownbear added >non-issue :Distributed/Cluster Coordination v8.5.0 labels

elasticsearchmachine added the Team:Distributed label

Collaborator

elasticsearchmachine commented Sep 15, 2022

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

idegtiarenko reviewed

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java Outdated

+                                  return;
+                              }
+                          } else {
+                              noop = false;

Contributor

idegtiarenko Sep 19, 2022

Nit: noop is already assigned by this point, no need for the else branch

idegtiarenko reviewed

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java Outdated

+                                  indices = DiffableUtils.emptyDiff();
+                                  templates = DiffableUtils.emptyDiff();
+                                  customs = DiffableUtils.emptyDiff();
+                                  reservedStateMetadata = DiffableUtils.emptyDiff();

Contributor

idegtiarenko Sep 19, 2022

I wonder if it is possible or worth replacing constructor with a static factory? This way we could have a constant static empty diff

Contributor Author

original-brownbear Sep 19, 2022

Good point, we don't need a specific instance when reading. We even have a static factory already. I made use of that and reused the existing empty instance in SimpleDiffable now + made the constructor private to avoid abuse.

idegtiarenko reviewed

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java Outdated Show resolved Hide resolved

idegtiarenko reviewed

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java Outdated

                       private final Diff<Map<String, ReservedStateMetadata>> reservedStateMetadata;
+                      // true if this diff is a noop because before and after were the same instance
+                      private final boolean noop;

Contributor

idegtiarenko Sep 19, 2022

I think it is better to name this empty in a context of a diff

idegtiarenko reviewed

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java

                       @Override
                       public Metadata apply(Metadata part) {
+                          if (noop) {
+                              return part;

Contributor

idegtiarenko Sep 19, 2022

👍

idegtiarenko reviewed

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java Outdated

                           new DiffableUtils.DiffableValueReader<>(ReservedStateMetadata::readFrom, ReservedStateMetadata::readDiffFrom);
                       MetadataDiff(StreamInput in) throws IOException {
+                          if (in.getVersion().onOrAfter(NOOP_METADATA_DIFF_VERSION)) {

Contributor

idegtiarenko Sep 19, 2022

Do we have a unit tests that checks serialization/deserialization of the MetadataDiff? DO we need one for an empty case?

Contributor Author

original-brownbear Sep 19, 2022

Turns out we don't, it's massively covered by existing tests but it's nice to have a test that makes sure we get instance equality for the empty diff to lock down this optimization. I added one now.

original-brownbear added 2 commits

September 19, 2022 09:34


          Merge remote-tracking branch 'elastic/main' into skip-noop-metadata-diff

cf46ca5


          CR comments

d0e0bb3

original-brownbear requested a review from idegtiarenko

September 19, 2022 08:08

Contributor Author

original-brownbear commented Sep 19, 2022

Thanks Ievgen, all points addressed now I think.

idegtiarenko approved these changes

View reviewed changes

Contributor Author

original-brownbear commented Sep 19, 2022

Thanks Ievgen!

original-brownbear merged commit 0bbe17a into elastic:main

original-brownbear deleted the skip-noop-metadata-diff branch

September 19, 2022 13:28

original-brownbear mentioned this pull request

Optimize unchanged metadata handling in PersistedClusterStateService #90164

Merged

original-brownbear added a commit that referenced this pull request


          Optimize unchanged metadata handling in PersistedClusterStateService (#…

c462e50

…90164)

With #90101 both follower and leader nodes will get instance equality
on the metadata on non-metadata changes here reliably.
We can leverage that and skip the expensive loop over all indices
in this method on instance equality.

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request


          Preserve last-committed data at accept time

3ff4c76

The cluster coordination consistency layer relies on a couple of fields
within `Metadata` which record the last _committed_ values on each node.
In contrast, the rest of the cluster state can only be changed at
_accept_ time.

In the past we would copy these fields over from the master on every
publication, but since elastic#90101 we don't copy anything at all if the
`Metadata` is unchanged on the master. However, the master computes the
diff against the last _committed_ state whereas the receiving nodes
apply the diff to the last _accepted_ state, and this means if the
master sends a no-op `Metadata` diff then the receiving node will revert
its last-committed values to the ones included in the state it last
accepted.

With this commit we adjust `CoordinationState` to ignore changes to the
last-committed fields at accept time.

DaveCTurner mentioned this pull request

Preserve last-committed data at accept time #92258

Closed

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request


          Include last-committed data in publication

173d526

The cluster coordination consistency layer relies on a couple of fields
within `Metadata` which record the last _committed_ values on each node.
In contrast, the rest of the cluster state can only be changed at
_accept_ time.

In the past we would copy these fields over from the master on every
publication, but since elastic#90101 we don't copy anything at all if the
`Metadata` is unchanged on the master. However, the master computes the
diff against the last _committed_ state whereas the receiving nodes
apply the diff to the last _accepted_ state, and this means if the
master sends a no-op `Metadata` diff then the receiving node will revert
its last-committed values to the ones included in the state it last
accepted.

With this commit we include the last-committed values alongside the
cluster state diff so that they are always copied properly.

DaveCTurner mentioned this pull request

Include last-committed data in publication #92259

Merged

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request


          Include last-committed data in publication

ad1574f

The cluster coordination consistency layer relies on a couple of fields
within `Metadata` which record the last _committed_ values on each node.
In contrast, the rest of the cluster state can only be changed at
_accept_ time.

In the past we would copy these fields over from the master on every
publication, but since elastic#90101 we don't copy anything at all if the
`Metadata` is unchanged on the master. However, the master computes the
diff against the last _committed_ state whereas the receiving nodes
apply the diff to the last _accepted_ state, and this means if the
master sends a no-op `Metadata` diff then the receiving node will revert
its last-committed values to the ones included in the state it last
accepted.

With this commit we include the last-committed values alongside the
cluster state diff so that they are always copied properly.

elasticsearchmachine pushed a commit that referenced this pull request


          Include last-committed data in publication (#92259)

fb756a9

The cluster coordination consistency layer relies on a couple of fields
within `Metadata` which record the last _committed_ values on each node.
In contrast, the rest of the cluster state can only be changed at
_accept_ time.

In the past we would copy these fields over from the master on every
publication, but since #90101 we don't copy anything at all if the
`Metadata` is unchanged on the master. However, the master computes the
diff against the last _committed_ state whereas the receiving nodes
apply the diff to the last _accepted_ state, and this means if the
master sends a no-op `Metadata` diff then the receiving node will revert
its last-committed values to the ones included in the state it last
accepted.

With this commit we include the last-committed values alongside the
cluster state diff so that they are always copied properly.

Closes #90158

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request


          Include last-committed data in publication

The cluster coordination consistency layer relies on a couple of fields
within `Metadata` which record the last _committed_ values on each node.
In contrast, the rest of the cluster state can only be changed at
_accept_ time.

In the past we would copy these fields over from the master on every
publication, but since elastic#90101 we don't copy anything at all if the
`Metadata` is unchanged on the master. However, the master computes the
diff against the last _committed_ state whereas the receiving nodes
apply the diff to the last _accepted_ state, and this means if the
master sends a no-op `Metadata` diff then the receiving node will revert
its last-committed values to the ones included in the state it last
accepted.

With this commit we include the last-committed values alongside the
cluster state diff so that they are always copied properly.

Closes elastic#90158
Backport of elastic#92259 to 8.6

DaveCTurner mentioned this pull request

Include last-committed data in publication #92277

Merged

DaveCTurner added a commit that referenced this pull request


          Include last-committed data in publication (#92277)

890f010

The cluster coordination consistency layer relies on a couple of fields
within `Metadata` which record the last _committed_ values on each node.
In contrast, the rest of the cluster state can only be changed at
_accept_ time.

In the past we would copy these fields over from the master on every
publication, but since #90101 we don't copy anything at all if the
`Metadata` is unchanged on the master. However, the master computes the
diff against the last _committed_ state whereas the receiving nodes
apply the diff to the last _accepted_ state, and this means if the
master sends a no-op `Metadata` diff then the receiving node will revert
its last-committed values to the ones included in the state it last
accepted.

With this commit we include the last-committed values alongside the
cluster state diff so that they are always copied properly.

Closes #90158
Backport of #92259 to 8.6

original-brownbear restored the skip-noop-metadata-diff branch

April 18, 2023 20:40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Cluster Coordination >non-issue Team:Distributed v8.5.0