smiklosovic commented on code in PR #4282:
URL: https://github.com/apache/cassandra/pull/4282#discussion_r2250704072
##########
src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java:
##########
@@ -82,6 +82,13 @@ public Keyspaces apply(ClusterMetadata metadata)
}
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 = keyspaceMetadata.params;
+ ReplicationParams replicationParams =
metadata.placements.deduplicateReplicationParams(keyspaceParams.replication);
Review Comment:
Is this logic really necessary? What we started to do is `withSwapped`
which is creating new object `new KeyspaceParams` and `new KeyspaceMetadata`
etc ...
So one step forward but also back as we allocate ...
What if "the deduplication" was done directly upon
`KeyspaceMetadata.create`? We do `attrs.asNewKeyspaceParams` just so we do
further transformations / deduplications on that. Why would not we put
`KeyspaceParams` into `KeyspaceMetadata.create` deduplicated already?
##########
src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java:
##########
@@ -82,6 +82,13 @@ public Keyspaces apply(ClusterMetadata metadata)
}
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 = keyspaceMetadata.params;
+ ReplicationParams replicationParams =
metadata.placements.deduplicateReplicationParams(keyspaceParams.replication);
Review Comment:
Is this logic really necessary? What we started to do is `withSwapped`
which is creating new object `new KeyspaceParams` and `new KeyspaceMetadata`
etc ...
So one step forward but also some step back as we allocate ...
What if "the deduplication" was done directly upon
`KeyspaceMetadata.create`? We do `attrs.asNewKeyspaceParams` just so we do
further transformations / deduplications on that. Why would not we put
`KeyspaceParams` into `KeyspaceMetadata.create` deduplicated already?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
