diff --git a/CHANGES.txt b/CHANGES.txt index 6a23d410e3..2a7162fb4c 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 5.1 + * Optimize DataPlacement lookup by ReplicationParams (CASSANDRA-20804) * Fix ShortPaxosSimulationTest and AccordSimulationRunner do not execute from the cli (CASSANDRA-20805) * Allow overriding arbitrary settings via environment variables (CASSANDRA-20749) * Optimize MessagingService.getVersionOrdinal (CASSANDRA-20816) diff --git a/src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java b/src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java index 882117f218..ed448f6842 100644 --- a/src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java @@ -81,7 +81,13 @@ public final class CreateKeyspaceStatement extends AlterSchemaStatement throw new AlreadyExistsException(keyspaceName); } - KeyspaceMetadata keyspaceMetadata = KeyspaceMetadata.create(keyspaceName, attrs.asNewKeyspaceParams()); + // we deduplicate ReplicationParams here to use the same objects in KeyspaceMetadata + // as we have as keys in metadata.placements to have a fast map lookup + // ReplicationParams are immutable, so it is a safe optimization + KeyspaceParams keyspaceParams = attrs.asNewKeyspaceParams(); + ReplicationParams replicationParams = metadata.placements.deduplicateReplicationParams(keyspaceParams.replication); + keyspaceParams = keyspaceParams.withSwapped(replicationParams); + KeyspaceMetadata keyspaceMetadata = KeyspaceMetadata.create(keyspaceName, keyspaceParams); if (keyspaceMetadata.params.replication.klass.equals(LocalStrategy.class)) throw ire("Unable to use given strategy class: LocalStrategy is reserved for internal use."); diff --git a/src/java/org/apache/cassandra/locator/ReplicaLayout.java b/src/java/org/apache/cassandra/locator/ReplicaLayout.java index f961b4051d..0cd3ccf257 100644 --- a/src/java/org/apache/cassandra/locator/ReplicaLayout.java +++ b/src/java/org/apache/cassandra/locator/ReplicaLayout.java @@ -31,6 +31,7 @@ import org.apache.cassandra.schema.KeyspaceMetadata; import org.apache.cassandra.schema.TableId; import org.apache.cassandra.service.reads.ReadCoordinator; import org.apache.cassandra.tcm.ClusterMetadata; +import org.apache.cassandra.tcm.ownership.DataPlacement; import org.apache.cassandra.utils.FBUtilities; import java.util.Set; @@ -238,8 +239,14 @@ public abstract class ReplicaLayout> { // todo deduplicate so that "pending" contains "read - write", // which is a hack until we revisit how consistency level handles pending - natural = forNonLocalStrategyTokenRead(metadata, ks, token); - pending = forNonLocalStrategyTokenWrite(metadata, ks, token).without(natural.endpoints()); + DataPlacement dataPlacement = metadata.placements.get(ks.params.replication); + natural = forNonLocalStrategyTokenRead(dataPlacement, token); + // perf optimization to avoid double endpoints search and filtering for a typical case + // DataPlacement constructor does a deduplication of reads/writes, so we can use cheap == comparision here + if (dataPlacement.reads == dataPlacement.writes) + pending = EndpointsForToken.empty(token); + else + pending = forNonLocalStrategyTokenWrite(dataPlacement, token).without(natural.endpoints()); } return forTokenWrite(replicationStrategy, natural, pending); } @@ -392,14 +399,25 @@ public abstract class ReplicaLayout> public static EndpointsForToken forNonLocalStrategyTokenRead(ClusterMetadata metadata, KeyspaceMetadata keyspace, Token token) { - return metadata.placements.get(keyspace.params.replication).reads.forToken(token).get(); + return forNonLocalStrategyTokenRead(metadata.placements.get(keyspace.params.replication), token); + } + + public static EndpointsForToken forNonLocalStrategyTokenRead(DataPlacement dataPlacement, Token token) + { + return dataPlacement.reads.forToken(token).get(); } static EndpointsForToken forNonLocalStrategyTokenWrite(ClusterMetadata metadata, KeyspaceMetadata keyspace, Token token) { - return metadata.placements.get(keyspace.params.replication).writes.forToken(token).get(); + return forNonLocalStrategyTokenWrite(metadata.placements.get(keyspace.params.replication), token); } + static EndpointsForToken forNonLocalStrategyTokenWrite(DataPlacement dataPlacement, Token token) + { + return dataPlacement.writes.forToken(token).get(); + } + + static EndpointsForRange forLocalStrategyRange(ClusterMetadata metadata, AbstractReplicationStrategy replicationStrategy, AbstractBounds range) { return replicationStrategy.calculateNaturalReplicas(range.right.getToken(), metadata); diff --git a/src/java/org/apache/cassandra/schema/KeyspaceParams.java b/src/java/org/apache/cassandra/schema/KeyspaceParams.java index 09afed84ba..f683fabf49 100644 --- a/src/java/org/apache/cassandra/schema/KeyspaceParams.java +++ b/src/java/org/apache/cassandra/schema/KeyspaceParams.java @@ -117,6 +117,11 @@ public final class KeyspaceParams return new KeyspaceParams(true, ReplicationParams.nts(args), FastPathStrategy.simple()); } + public KeyspaceParams withSwapped(ReplicationParams params) + { + return new KeyspaceParams(durableWrites, params, fastPath); + } + public void validate(String name, ClientState state, ClusterMetadata metadata) { replication.validate(name, state, metadata); diff --git a/src/java/org/apache/cassandra/schema/ReplicationParams.java b/src/java/org/apache/cassandra/schema/ReplicationParams.java index 40a92c803f..c1e2643897 100644 --- a/src/java/org/apache/cassandra/schema/ReplicationParams.java +++ b/src/java/org/apache/cassandra/schema/ReplicationParams.java @@ -57,11 +57,13 @@ public final class ReplicationParams public final Class klass; public final ImmutableMap options; + private final int hashCode; private ReplicationParams(Class klass, Map options) { this.klass = klass; this.options = ImmutableMap.copyOf(options); + this.hashCode = Objects.hashCode(this.klass, this.options); } @VisibleForTesting @@ -234,7 +236,7 @@ public final class ReplicationParams @Override public int hashCode() { - return Objects.hashCode(klass, options); + return hashCode; } @Override diff --git a/src/java/org/apache/cassandra/tcm/ClusterMetadata.java b/src/java/org/apache/cassandra/tcm/ClusterMetadata.java index d57df4de5f..aa8c238941 100644 --- a/src/java/org/apache/cassandra/tcm/ClusterMetadata.java +++ b/src/java/org/apache/cassandra/tcm/ClusterMetadata.java @@ -55,6 +55,7 @@ import org.apache.cassandra.locator.Replica; import org.apache.cassandra.net.CMSIdentifierMismatchException; import org.apache.cassandra.schema.DistributedSchema; import org.apache.cassandra.schema.KeyspaceMetadata; +import org.apache.cassandra.schema.KeyspaceParams; import org.apache.cassandra.schema.Keyspaces; import org.apache.cassandra.schema.ReplicationParams; import org.apache.cassandra.schema.TableId; @@ -1064,6 +1065,8 @@ public class ClusterMetadata TokenMap tokenMap = TokenMap.serializer.deserialize(in, version); DataPlacements placements = DataPlacements.serializer.deserialize(in, version); + schema = deduplicateReplicationParams(schema, placements); + AccordFastPath accordFastPath; ConsensusMigrationState consensusMigrationState; AccordStaleReplicas staleReplicas; @@ -1107,6 +1110,23 @@ public class ClusterMetadata staleReplicas); } + private DistributedSchema deduplicateReplicationParams(DistributedSchema schema, DataPlacements placements) + { + Keyspaces newKeyspaces = schema.getKeyspaces(); + for (KeyspaceMetadata keyspaceMetadata : schema.getKeyspaces()) + { + KeyspaceParams params = keyspaceMetadata.params; + ReplicationParams newReplicationParams = placements.deduplicateReplicationParams(params.replication); + if (newReplicationParams != params.replication) + { + KeyspaceParams newKeyspaceParams = params.withSwapped(newReplicationParams); + KeyspaceMetadata newKeyspaceMetadata = keyspaceMetadata.withSwapped(newKeyspaceParams); + newKeyspaces = newKeyspaces.withAddedOrUpdated(newKeyspaceMetadata); + } + } + return new DistributedSchema(newKeyspaces, schema.lastModified()); + } + @Override public long serializedSize(ClusterMetadata metadata, Version version) { diff --git a/src/java/org/apache/cassandra/tcm/ownership/DataPlacement.java b/src/java/org/apache/cassandra/tcm/ownership/DataPlacement.java index 12920d6862..fd1a2618db 100644 --- a/src/java/org/apache/cassandra/tcm/ownership/DataPlacement.java +++ b/src/java/org/apache/cassandra/tcm/ownership/DataPlacement.java @@ -58,7 +58,10 @@ public class DataPlacement ReplicaGroups writes) { this.reads = reads; - this.writes = writes; + if (reads.equals(writes)) + this.writes = reads; // performance optimization for a typical case, to not search for endpoints twice + else + this.writes = writes; } /** diff --git a/src/java/org/apache/cassandra/tcm/ownership/DataPlacements.java b/src/java/org/apache/cassandra/tcm/ownership/DataPlacements.java index b89ecde7d3..a4dc95eef4 100644 --- a/src/java/org/apache/cassandra/tcm/ownership/DataPlacements.java +++ b/src/java/org/apache/cassandra/tcm/ownership/DataPlacements.java @@ -159,6 +159,17 @@ public class DataPlacements extends ReplicationMap implements Met .allMatch(e -> e.getValue().equivalentTo(other.get(e.getKey()))); } + public ReplicationParams deduplicateReplicationParams(ReplicationParams replicationParams) + { + if (this.get(replicationParams) == null) + return replicationParams; + + for (ReplicationParams placementReplicationParams : keys()) + if (placementReplicationParams.equals(replicationParams)) + return placementReplicationParams; + return replicationParams; + } + public static DataPlacements sortReplicaGroups(DataPlacements placements, Comparator comparator) { Builder builder = DataPlacements.builder(placements.size());