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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
