Skip to content

Speed up Routing Nodes Priority Comparator #78609

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 2 commits into from
Oct 4, 2021
Merged

Speed up Routing Nodes Priority Comparator #78609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df1a9e9
Speed up Routing Nodes Priority Comparator
original-brownbear Oct 4, 2021
c1cf040
final fields :)
original-brownbear Oct 4, 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
28 changes: 20 additions & 8 deletions server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,10 @@ public static APIBlock readFrom(StreamInput input) throws IOException {

private final IndexLongFieldRange timestampRange;

private final int priority;

private final long creationDate;

private IndexMetadata(
final Index index,
final long version,
Expand All @@ -417,7 +421,9 @@ private IndexMetadata(
final ActiveShardCount waitForActiveShards,
final ImmutableOpenMap<String, RolloverInfo> rolloverInfos,
final boolean isSystem,
final IndexLongFieldRange timestampRange) {
final IndexLongFieldRange timestampRange,
final int priority,
final long creationDate) {

this.index = index;
this.version = version;
Expand Down Expand Up @@ -450,6 +456,8 @@ private IndexMetadata(
this.rolloverInfos = rolloverInfos;
this.isSystem = isSystem;
this.timestampRange = timestampRange;
this.priority = priority;
this.creationDate = creationDate;
assert numberOfShards * routingFactor == routingNumShards : routingNumShards + " must be a multiple of " + numberOfShards;
}

Expand Down Expand Up @@ -509,7 +517,7 @@ public Version getCreationVersion() {
}

public long getCreationDate() {
return settings.getAsLong(SETTING_CREATION_DATE, -1L);
return creationDate;
}

public State getState() {
Expand Down Expand Up @@ -930,6 +938,10 @@ public boolean isSystem() {
return isSystem;
}

public int priority() {
return priority;
}

public static Builder builder(String index) {
return new Builder(index);
}
Expand Down Expand Up @@ -1206,9 +1218,6 @@ public IndexLongFieldRange getTimestampRange() {
}

public IndexMetadata build() {
ImmutableOpenMap.Builder<String, AliasMetadata> tmpAliases = aliases;
Settings tmpSettings = settings;

/*
* We expect that the metadata has been properly built to set the number of shards and the number of replicas, and do not rely
* on the default values here. Those must have been set upstream.
Expand Down Expand Up @@ -1294,9 +1303,9 @@ public IndexMetadata build() {
state,
numberOfShards,
numberOfReplicas,
tmpSettings,
settings,
mappings.build(),
tmpAliases.build(),
aliases.build(),
customMetadata.build(),
filledInSyncAllocationIds.build(),
requireFilters,
Expand All @@ -1309,7 +1318,10 @@ public IndexMetadata build() {
waitForActiveShards,
rolloverInfos.build(),
isSystem,
timestampRange);
timestampRange,
IndexMetadata.INDEX_PRIORITY_SETTING.get(settings),
settings.getAsLong(SETTING_CREATION_DATE, -1L)
);
}

@SuppressWarnings("unchecked")
Expand Down
4 changes: 0 additions & 4 deletions 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 @@ -16,7 +16,6 @@
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.cluster.metadata.Metadata;
import org.elasticsearch.cluster.metadata.SingleNodeShutdownMetadata;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.cluster.routing.UnassignedInfo.AllocationStatus;
import org.elasticsearch.cluster.routing.allocation.ExistingShardsAllocator;
Expand Down Expand Up @@ -67,8 +66,6 @@ public class RoutingNodes implements Iterable<RoutingNode> {

private final Map<ShardId, List<ShardRouting>> assignedShards = new HashMap<>();

private final Map<String, SingleNodeShutdownMetadata> nodeShutdowns;

private final boolean readOnly;

private int inactivePrimaryCount = 0;
Expand All @@ -87,7 +84,6 @@ public RoutingNodes(ClusterState clusterState) {
public RoutingNodes(ClusterState clusterState, boolean readOnly) {
this.readOnly = readOnly;
final RoutingTable routingTable = clusterState.routingTable();
nodeShutdowns = clusterState.metadata().nodeShutdowns();

Map<String, LinkedHashMap<ShardId, ShardRouting>> nodesToShards = new HashMap<>();
// fill in the nodeToShards with the "live" nodes
Expand Down
25 changes: 7 additions & 18 deletions server/src/main/java/org/elasticsearch/gateway/PriorityComparator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.cluster.routing.ShardRouting;
import org.elasticsearch.cluster.routing.allocation.RoutingAllocation;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.Index;

import java.util.Comparator;
Expand All @@ -32,38 +31,28 @@ public abstract class PriorityComparator implements Comparator<ShardRouting> {

@Override
public final int compare(ShardRouting o1, ShardRouting o2) {
final String o1Index = o1.getIndexName();
final String o2Index = o2.getIndexName();
final Index o1Index = o1.index();
final Index o2Index = o2.index();
Copy link
Contributor Author

@original-brownbear original-brownbear Oct 4, 2021

Choose a reason for hiding this comment

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

Better compare the Index here than just their names. If we have instance equality it's the same performance as comparing names. If we have different indices, the UUID strings will compare quicker than index names because there's no common prefixes.

int cmp = 0;
if (o1Index.equals(o2Index) == false) {
final IndexMetadata metadata01 = getMetadata(o1.index());
final IndexMetadata metadata02 = getMetadata(o2.index());
final IndexMetadata metadata01 = getMetadata(o1Index);
final IndexMetadata metadata02 = getMetadata(o2Index);
cmp = Boolean.compare(metadata02.isSystem(), metadata01.isSystem());

if (cmp == 0) {
final Settings settingsO1 = metadata01.getSettings();
final Settings settingsO2 = metadata02.getSettings();
cmp = Long.compare(priority(settingsO2), priority(settingsO1));
cmp = Long.compare(metadata02.priority(), metadata01.priority());

if (cmp == 0) {
cmp = Long.compare(timeCreated(settingsO2), timeCreated(settingsO1));
cmp = Long.compare(metadata02.getCreationDate(), metadata01.getCreationDate());
if (cmp == 0) {
cmp = o2Index.compareTo(o1Index);
cmp = o2Index.getName().compareTo(o1Index.getName());
}
}
}
}
return cmp;
}

private static int priority(Settings settings) {
return IndexMetadata.INDEX_PRIORITY_SETTING.get(settings);
}

private static long timeCreated(Settings settings) {
return settings.getAsLong(IndexMetadata.SETTING_CREATION_DATE, -1L);
}

protected abstract IndexMetadata getMetadata(Index index);

/**
Expand Down