You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2021/01/29 16:06:50 UTC
[GitHub] [cassandra] adelapena commented on a change in pull request #870: CASSANDRA-16296 Warn on KS creation when RF > nodes
adelapena commented on a change in pull request #870:
URL: https://github.com/apache/cassandra/pull/870#discussion_r566899298
##########
File path: src/java/org/apache/cassandra/dht/tokenallocator/TokenAllocation.java
##########
@@ -134,7 +134,7 @@ SummaryStatistics getAllocationRingOwnership(InetAddressAndPort endpoint)
abstract class StrategyAdapter implements ReplicationStrategy<InetAddressAndPort>
{
- // return true iff the provided endpoint occurs in the same virtual token-ring we are allocating for
+ // return true if the provided endpoint occurs in the same virtual token-ring we are allocating for
Review comment:
I think `iff` stands for "if and only if", is it not correct in the context?
##########
File path: test/unit/org/apache/cassandra/cql3/validation/operations/CreateTest.java
##########
@@ -365,6 +372,31 @@ public void testKeyspace() throws Throwable
execute("DROP KEYSPACE testXYZ");
}
+ /**
+ * Test a warning is thrown on create keyspace with a RF > number of nodes.
+ */
+ @Test
+ public void testCreateKeyspaceRFgtNodesWarns() throws Throwable
+ {
+ // NTS
+ ClientWarn.instance.captureWarnings();
+ execute("CREATE KEYSPACE testABC WITH replication = {'class' : 'NetworkTopologyStrategy', '" + DATA_CENTER + "' : 2 }");
+ List<String> warnings = ClientWarn.instance.getWarnings();
+ warnings.removeIf(s -> !s.equals("Your replication factor 2 for keyspace testabc is higher than the number of nodes 1 for datacenter datacenter1"));
Review comment:
```suggestion
warnings.removeIf(s -> !s.equals("Your replication factor 2 for keyspace testabc is higher than the number of nodes 1 for datacenter " + DATA_CENTER));
```
##########
File path: src/java/org/apache/cassandra/locator/SimpleStrategy.java
##########
@@ -88,6 +92,18 @@ public void validateOptions() throws ConfigurationException
{
validateOptionsInternal(configOptions);
validateReplicationFactor(configOptions.get(REPLICATION_FACTOR));
+
+ int nodeCount = StorageService.instance.getHostIdToEndpoint().size();
+ // nodeCount==0 on many tests
+ if (rf.fullReplicas > nodeCount && nodeCount!=0)
Review comment:
```suggestion
if (rf.fullReplicas > nodeCount && nodeCount != 0)
```
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java
##########
@@ -46,6 +48,7 @@
private final KeyspaceAttributes attrs;
private final boolean ifNotExists;
+ private HashSet<String> clientWarnings = new HashSet<>();
Review comment:
Nit: could be final
```suggestion
private final HashSet<String> clientWarnings = new HashSet<>();
```
##########
File path: src/java/org/apache/cassandra/locator/TokenMetadata.java
##########
@@ -1331,6 +1331,23 @@ public EndpointsForToken getWriteEndpoints(Token token, String keyspaceName, End
}
}
+ /**
+ * @return a (stable copy, won't be modified) datacenter to Endpoint map for all the nodes in the cluster.
+ */
+ public ImmutableMultimap<String, InetAddressAndPort> getDC2AllEndpoints(IEndpointSnitch snitch)
+ {
+ ImmutableMultimap.Builder<String, InetAddressAndPort> dc2Nodesbuilder = ImmutableMultimap.builder();
+ HashSet<InetAddressAndPort> nodes = new HashSet<InetAddressAndPort>(getAllEndpoints());
Review comment:
Why is the `Set` returned by `getAllEndpoints` encapsulated in another `Set`? Can we just iterate `getAllEndpoints`? Also wondering whether we could replace the entire method body by `Multimaps.index(getAllEndpoints(), snitch::getDatacenter)`.
##########
File path: src/java/org/apache/cassandra/dht/RangeStreamer.java
##########
@@ -353,9 +361,20 @@ public void addRanges(String keyspaceName, ReplicaCollection<?> replicas)
*/
private boolean useStrictSourcesForRanges(AbstractReplicationStrategy strat)
{
- return useStrictConsistency
- && tokens != null
- && metadata.getSizeOfAllEndpoints() != strat.getReplicationFactor().allReplicas;
+ int nodes = 0;
+
+ if (strat instanceof NetworkTopologyStrategy)
Review comment:
We could evaluate the `useStrictConsistency && tokens != null` part of the boolean expression at the beginning of the method, so in some cases wouldn't need to compute `nodes`. Alternatively, we could encapsulate the current calculation of `nodes` in a function, so it would be lazily called from the boolean expression.
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java
##########
@@ -56,6 +59,9 @@ public CreateKeyspaceStatement(String keyspaceName, KeyspaceAttributes attrs, bo
public Keyspaces apply(Keyspaces schema)
{
+ if (ClientWarn.instance.get() == null)
+ ClientWarn.instance.captureWarnings();
Review comment:
Nit: misaligned
```suggestion
if (ClientWarn.instance.get() == null)
ClientWarn.instance.captureWarnings();
```
##########
File path: test/unit/org/apache/cassandra/cql3/validation/operations/CreateTest.java
##########
@@ -365,6 +372,31 @@ public void testKeyspace() throws Throwable
execute("DROP KEYSPACE testXYZ");
}
+ /**
+ * Test a warning is thrown on create keyspace with a RF > number of nodes.
+ */
+ @Test
+ public void testCreateKeyspaceRFgtNodesWarns() throws Throwable
Review comment:
Nice test :). Don't we need a call to `requireNetwork()` to make the client warnings work when the test is run isolated?
##########
File path: src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java
##########
@@ -51,7 +51,6 @@
{
private static final Logger logger = LoggerFactory.getLogger(AbstractReplicationStrategy.class);
- @VisibleForTesting
Review comment:
I think `keyspaceName` could be protected
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org