Skip to content

Make MasterService.patchVersions not Rebuild the Full CS #79860

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 27, 2021
Merged

Make MasterService.patchVersions not Rebuild the Full CS #79860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8d3059a
Make MasterService.patchVersions not Rebuild the Full CS
original-brownbear Oct 26, 2021
9b90629
Merge remote-tracking branch 'elastic/master' into free-version-incre…
original-brownbear Oct 27, 2021
350e579
Merge remote-tracking branch 'elastic/master' into free-version-incre…
original-brownbear Oct 27, 2021
10f16c2
CR: add assertion
original-brownbear Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions server/src/main/java/org/elasticsearch/cluster/ClusterState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.io.stream.VersionedNamedWriteable;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.xcontent.ToXContentFragment;
import org.elasticsearch.xcontent.XContentBuilder;

Expand Down Expand Up @@ -121,12 +122,21 @@ default boolean isPrivate() {

public ClusterState(long version, String stateUUID, ClusterState state) {
this(state.clusterName, version, stateUUID, state.metadata(), state.routingTable(), state.nodes(), state.blocks(),
state.customs(), false);
}

public ClusterState(ClusterName clusterName, long version, String stateUUID, Metadata metadata, RoutingTable routingTable,
DiscoveryNodes nodes, ClusterBlocks blocks, ImmutableOpenMap<String, Custom> customs,
boolean wasReadFromDiff) {
state.customs(), false, state.routingNodes);
}

public ClusterState(
ClusterName clusterName,
long version,
String stateUUID,
Metadata metadata,
RoutingTable routingTable,
DiscoveryNodes nodes,
ClusterBlocks blocks,
ImmutableOpenMap<String, Custom> customs,
boolean wasReadFromDiff,
@Nullable RoutingNodes routingNodes
) {
this.version = version;
this.stateUUID = stateUUID;
this.clusterName = clusterName;
Expand All @@ -136,6 +146,9 @@ public ClusterState(ClusterName clusterName, long version, String stateUUID, Met
this.blocks = blocks;
this.customs = customs;
this.wasReadFromDiff = wasReadFromDiff;
this.routingNodes = routingNodes;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we assert that this is the routingNodes we would have got from a fresh rebuild here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not easily. Neither RoutingNodes nor ShardRouting have any equals methods.
I could try to add those here, but that would make it a much larger change.

For now it's also not entirely trivial to test the gains made here in isolation I think. I was thinking of just taking this chunk in isolation, then doing a follow-up with the remaining chunks that eliminate the remainder of the unnecessary routing node rebuilds from the big PR and just adding an instance equality assertion in ClusterChangedEvent that makes sure that if the routing table doesn't change, then the nodes instance didn't change.

This change seemed safe enough to not require additional tests in isolation (to me that is:)).

Copy link
Contributor

@DaveCTurner DaveCTurner Oct 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ehh I really don't like having this invariant not be enforced. This is a public constructor, there's a risk that a future caller gets this wrong in future. They might just pass in a RoutingNodes that they happen to have on hand (especially since the argument isn't marked as @Nullable).

Instance equality on ShardRouting should be enough right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instance equality on ShardRouting should be enough right?

Right ... I think. You convinced me :) I'll do it right. Working out a proper equals now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, added the assertion and all the necessary equals methods I did exploit some obvious invariants to not have to compare all fields, but I erred on the side of caution here and there might be possible optimizations to these methods, but since they're only used for the assertion it's probably irrelevant.

As for ShardRouting, we unfortunately needed an equals there as well because of the case where we re-create the instance with a null DiscoveryNode when building RoutingNodes (hence the fix to its toString here).
Seems to work fine now though :)

assert routingNodes == null || routingNodes.equals(new RoutingNodes(this)) :
"RoutingNodes [" + routingNodes + "] are not consistent with this cluster state [" + new RoutingNodes(this) + "]";
}

public long term() {
Expand Down Expand Up @@ -476,6 +489,8 @@ public static Builder builder(ClusterState state) {

public static class Builder {

private ClusterState previous;

private final ClusterName clusterName;
private long version = 0;
private String uuid = UNKNOWN_UUID;
Expand All @@ -487,6 +502,7 @@ public static class Builder {
private boolean fromDiff;

public Builder(ClusterState state) {
this.previous = state;
this.clusterName = state.clusterName;
this.version = state.version();
this.uuid = state.stateUUID();
Expand Down Expand Up @@ -580,7 +596,16 @@ public ClusterState build() {
if (UNKNOWN_UUID.equals(uuid)) {
uuid = UUIDs.randomBase64UUID();
}
return new ClusterState(clusterName, version, uuid, metadata, routingTable, nodes, blocks, customs.build(), fromDiff);
final RoutingNodes routingNodes;
if (previous != null && routingTable.indicesRouting() == previous.routingTable.indicesRouting() && nodes == previous.nodes) {
// routing table contents and nodes haven't changed so we can try to reuse the previous state's routing nodes which are
// expensive to compute
routingNodes = previous.routingNodes;
} else {
routingNodes = null;
}
return new ClusterState(clusterName, version, uuid, metadata, routingTable, nodes, blocks, customs.build(), fromDiff,
routingNodes);
}

public static byte[] toBytes(ClusterState state) throws IOException {
Expand Down
98 changes: 79 additions & 19 deletions server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,34 +192,41 @@ public interface NonRestorableCustom extends Custom {

private SortedMap<String, IndexAbstraction> indicesLookup;

Metadata(String clusterUUID, boolean clusterUUIDCommitted, long version, CoordinationMetadata coordinationMetadata,
Settings transientSettings, Settings persistentSettings, DiffableStringMap hashesOfConsistentSettings,
ImmutableOpenMap<String, IndexMetadata> indices, ImmutableOpenMap<String, IndexTemplateMetadata> templates,
ImmutableOpenMap<String, Custom> customs, String[] allIndices, String[] visibleIndices, String[] allOpenIndices,
String[] visibleOpenIndices, String[] allClosedIndices, String[] visibleClosedIndices,
SortedMap<String, IndexAbstraction> indicesLookup) {
private Metadata(
String clusterUUID,
boolean clusterUUIDCommitted,
long version,
CoordinationMetadata coordinationMetadata,
Settings transientSettings,
Settings persistentSettings,
Settings settings,
DiffableStringMap hashesOfConsistentSettings,
int totalNumberOfShards,
int totalOpenIndexShards,
ImmutableOpenMap<String, IndexMetadata> indices,
ImmutableOpenMap<String, IndexTemplateMetadata> templates,
ImmutableOpenMap<String, Custom> customs,
String[] allIndices,
String[] visibleIndices,
String[] allOpenIndices,
String[] visibleOpenIndices,
String[] allClosedIndices,
String[] visibleClosedIndices,
SortedMap<String, IndexAbstraction> indicesLookup
) {
this.clusterUUID = clusterUUID;
this.clusterUUIDCommitted = clusterUUIDCommitted;
this.version = version;
this.coordinationMetadata = coordinationMetadata;
this.transientSettings = transientSettings;
this.persistentSettings = persistentSettings;
this.settings = Settings.builder().put(persistentSettings).put(transientSettings).build();
this.settings = settings;
this.hashesOfConsistentSettings = hashesOfConsistentSettings;
this.indices = indices;
this.customs = customs;
this.templates = templates;
int totalNumberOfShards = 0;
int totalOpenIndexShards = 0;
for (IndexMetadata indexMetadata : indices.values()) {
totalNumberOfShards += indexMetadata.getTotalNumberOfShards();
if (IndexMetadata.State.OPEN.equals(indexMetadata.getState())) {
totalOpenIndexShards += indexMetadata.getTotalNumberOfShards();
}
}
this.totalNumberOfShards = totalNumberOfShards;
this.totalOpenIndexShards = totalOpenIndexShards;

this.allIndices = allIndices;
this.visibleIndices = visibleIndices;
this.allOpenIndices = allOpenIndices;
Expand All @@ -229,6 +236,31 @@ public interface NonRestorableCustom extends Custom {
this.indicesLookup = indicesLookup;
}

public Metadata withIncrementedVersion() {
return new Metadata(
clusterUUID,
clusterUUIDCommitted,
version + 1,
coordinationMetadata,
transientSettings,
persistentSettings,
settings,
hashesOfConsistentSettings,
totalNumberOfShards,
totalOpenIndexShards,
indices,
templates,
customs,
allIndices,
visibleIndices,
allOpenIndices,
visibleOpenIndices,
allClosedIndices,
visibleClosedIndices,
indicesLookup
);
}

public long version() {
return this.version;
}
Expand Down Expand Up @@ -1613,9 +1645,37 @@ public Metadata build(boolean builtIndicesLookupEagerly) {
String[] allClosedIndicesArray = allClosedIndices.toArray(Strings.EMPTY_ARRAY);
String[] visibleClosedIndicesArray = visibleClosedIndices.toArray(Strings.EMPTY_ARRAY);

return new Metadata(clusterUUID, clusterUUIDCommitted, version, coordinationMetadata, transientSettings, persistentSettings,
hashesOfConsistentSettings, indices, templates.build(), customs.build(), allIndicesArray, visibleIndicesArray,
allOpenIndicesArray, visibleOpenIndicesArray, allClosedIndicesArray, visibleClosedIndicesArray, indicesLookup);
int totalNumberOfShards = 0;
int totalOpenIndexShards = 0;
for (IndexMetadata indexMetadata : indices.values()) {
totalNumberOfShards += indexMetadata.getTotalNumberOfShards();
if (IndexMetadata.State.OPEN.equals(indexMetadata.getState())) {
totalOpenIndexShards += indexMetadata.getTotalNumberOfShards();
}
}

return new Metadata(
clusterUUID,
clusterUUIDCommitted,
version,
coordinationMetadata,
transientSettings,
persistentSettings,
Settings.builder().put(persistentSettings).put(transientSettings).build(),
hashesOfConsistentSettings,
totalNumberOfShards,
totalOpenIndexShards,
indices,
templates.build(),
customs.build(),
allIndicesArray,
visibleIndicesArray,
allOpenIndicesArray,
visibleOpenIndicesArray,
allClosedIndicesArray,
visibleClosedIndicesArray,
indicesLookup
);
}

static SortedMap<String, IndexAbstraction> buildIndicesLookup(DataStreamMetadata dataStreamMetadata,
Expand Down
41 changes: 32 additions & 9 deletions server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.function.Predicate;
import java.util.stream.Collectors;
Expand All @@ -34,6 +35,7 @@ public class RoutingNode implements Iterable<ShardRouting> {

private final String nodeId;

@Nullable
private final DiscoveryNode node;

private final LinkedHashMap<ShardId, ShardRouting> shards; // LinkedHashMap to preserve order
Expand All @@ -48,7 +50,7 @@ public RoutingNode(String nodeId, DiscoveryNode node, ShardRouting... shards) {
this(nodeId, node, buildShardRoutingMap(shards));
}

RoutingNode(String nodeId, DiscoveryNode node, LinkedHashMap<ShardId, ShardRouting> shards) {
RoutingNode(String nodeId, @Nullable DiscoveryNode node, LinkedHashMap<ShardId, ShardRouting> shards) {
this.nodeId = nodeId;
this.node = node;
this.shards = shards;
Expand Down Expand Up @@ -88,6 +90,7 @@ public Iterator<ShardRouting> iterator() {
*
* @return discoveryNode of this node
*/
@Nullable
public DiscoveryNode node() {
return this.node;
}
Expand Down Expand Up @@ -298,13 +301,17 @@ public String prettyPrint() {
public String toString() {
StringBuilder sb = new StringBuilder();
sb.append("routingNode ([");
sb.append(node.getName());
sb.append("][");
sb.append(node.getId());
sb.append("][");
sb.append(node.getHostName());
sb.append("][");
sb.append(node.getHostAddress());
if (node != null) {
sb.append(node.getName());
sb.append("][");
sb.append(node.getId());
sb.append("][");
sb.append(node.getHostName());
sb.append("][");
sb.append(node.getHostAddress());
} else {
sb.append("null");
}
sb.append("], [");
sb.append(shards.size());
sb.append(" assigned shards])");
Expand All @@ -320,7 +327,6 @@ public boolean isEmpty() {
}

private boolean invariant() {

// initializingShards must consistent with that in shards
Collection<ShardRouting> shardRoutingsInitializing =
shards.values().stream().filter(ShardRouting::initializing).collect(Collectors.toList());
Expand All @@ -339,4 +345,21 @@ private boolean invariant() {

return true;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
RoutingNode that = (RoutingNode) o;
return nodeId.equals(that.nodeId) && Objects.equals(node, that.node) && shards.equals(that.shards);
}

@Override
public int hashCode() {
return Objects.hash(nodeId, node, shards);
}
}
79 changes: 78 additions & 1 deletion server/src/main/java/org/elasticsearch/cluster/routing/RoutingNodes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,46 @@ public int size() {
return nodesToShards.size();
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
RoutingNodes that = (RoutingNodes) o;
return readOnly == that.readOnly
&& inactivePrimaryCount == that.inactivePrimaryCount
&& inactiveShardCount == that.inactiveShardCount
&& relocatingShards == that.relocatingShards
&& activeShardCount == that.activeShardCount
&& totalShardCount == that.totalShardCount
&& nodesToShards.equals(that.nodesToShards)
&& unassignedShards.equals(that.unassignedShards)
&& assignedShards.equals(that.assignedShards)
&& attributeValuesByAttribute.equals(that.attributeValuesByAttribute)
&& recoveriesPerNode.equals(that.recoveriesPerNode
);
}

@Override
public int hashCode() {
return Objects.hash(
nodesToShards,
unassignedShards,
assignedShards,
readOnly,
inactivePrimaryCount,
inactiveShardCount,
relocatingShards,
activeShardCount,
totalShardCount,
attributeValuesByAttribute,
recoveriesPerNode
);
}

public static final class UnassignedShards implements Iterable<ShardRouting> {

private final RoutingNodes nodes;
Expand Down Expand Up @@ -990,6 +1030,26 @@ public ShardRouting[] drain() {
primaries = 0;
return mutableShardRoutings;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
UnassignedShards that = (UnassignedShards) o;
return primaries == that.primaries
&& ignoredPrimaries == that.ignoredPrimaries
&& unassigned.equals(that.unassigned)
&& ignored.equals(that.ignored);
}

@Override
public int hashCode() {
return Objects.hash(unassigned, ignored, primaries, ignoredPrimaries);
}
}


Expand Down Expand Up @@ -1183,5 +1243,22 @@ public static Recoveries getOrAdd(Map<String, Recoveries> map, String key) {
}
return recoveries;
}
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
Recoveries that = (Recoveries) o;
return incoming == that.incoming && outgoing == that.outgoing;
}

@Override
public int hashCode() {
return Objects.hash(incoming, outgoing);
}
}
}
Loading