tlasica commented on a change in pull request #870:
URL: https://github.com/apache/cassandra/pull/870#discussion_r571853059
##########
File path:
src/java/org/apache/cassandra/cql3/statements/schema/AlterKeyspaceStatement.java
##########
@@ -62,6 +63,9 @@ public AlterKeyspaceStatement(String keyspaceName,
KeyspaceAttributes attrs)
public Keyspaces apply(Keyspaces schema)
{
+ if (ClientWarn.instance.get() == null)
+ ClientWarn.instance.captureWarnings();
+ int previousWarningsSize = ClientWarn.instance.getWarnings() == null ?
0 : ClientWarn.instance.getWarnings().size();
Review comment:
maybe we can introduce `ClientWarn::numWarnings()` method that would
hide this null check and return 0 or size?
##########
File path:
src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java
##########
@@ -56,6 +62,10 @@ public CreateKeyspaceStatement(String keyspaceName,
KeyspaceAttributes attrs, bo
public Keyspaces apply(Keyspaces schema)
{
+ if (ClientWarn.instance.get() == null)
+ ClientWarn.instance.captureWarnings();
+ int previousWarningsSize = ClientWarn.instance.getWarnings() == null ?
0 : ClientWarn.instance.getWarnings().size();
Review comment:
`numWarnings()` could be used here
##########
File path:
src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java
##########
@@ -75,8 +85,16 @@ public Keyspaces apply(Keyspaces schema)
throw ire("Unable to use given strategy class: LocalStrategy is
reserved for internal use.");
keyspace.params.validate(keyspaceName);
+ Keyspaces ksps = schema.withAddedOrUpdated(keyspace);
- return schema.withAddedOrUpdated(keyspace);
+ if (ClientWarn.instance.getWarnings() != null)
Review comment:
and here same pattern:
`clientWanrings.addAll(ClientWarn.instance.getWarningsAfter(previousWarningsSize));`
##########
File path:
test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java
##########
@@ -269,6 +276,59 @@ public void testCreateAlterKeyspaces() throws Throwable
"max_threshold", "32")));
}
+ @Test
+ public void testCreateAlterKeyspacesRFWarnings() throws Throwable
+ {
+ requireNetwork();
+ Thread.sleep(1000); // Hack: requireNetwork() will clear warnings
async so give some time to complete
+
+ // NTS
+ ClientWarn.instance.captureWarnings();
+ String ks = createKeyspace("CREATE KEYSPACE %s WITH replication =
{'class' : 'NetworkTopologyStrategy', '" + DATA_CENTER + "' : 3 }");
+ List<String> warnings = ClientWarn.instance.getWarnings();
+ assertEquals(1, warnings.size());
+ Assertions.assertThat(warnings.get(0)).contains("Your replication
factor 3 for keyspace " + ks + " is higher than the number of nodes 1 for
datacenter " + DATA_CENTER);
+
+ ClientWarn.instance.captureWarnings();
+ execute("CREATE TABLE " + ks + ".t (k int PRIMARY KEY, v int)");
+ warnings = ClientWarn.instance.getWarnings();
+ assertNull(warnings);
+
+ ClientWarn.instance.captureWarnings();
Review comment:
with so many cases maybe we can extract some methods
```
private void expectCreateKeyspaceWarning(ddl, warning)
{
List<String> warnings = ClientWarn.instance.getWarnings();
createKeyspace(ddl);
assertEquals(1, warnings.size());
Assertions.assertThat(warnings.get(0)).contains(warning);
}
```
and
```
private void expectNoWarning(...)
```
so that test would become a list of checks?
```
expectWarning(...)
expectNoWarning(....)
```
##########
File path: src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java
##########
@@ -302,6 +310,34 @@ public void validateOptions() throws ConfigurationException
}
}
+ @Override
+ public void maybeWarnOnOptions()
+ {
+ ImmutableMultimap<String, InetAddressAndPort> dcsNodes =
StorageService.instance.getTokenMetadata().getDC2AllEndpoints(snitch);
+ for (Entry<String, String> e : this.configOptions.entrySet())
+ {
+ if (!SchemaConstants.isSystemKeyspace(keyspaceName))
Review comment:
I think this system keyspace check should be outside of the loop, as int
he loop it is causing reader to think why.
##########
File path: src/java/org/apache/cassandra/dht/RangeStreamer.java
##########
@@ -353,9 +361,27 @@ public void addRanges(String keyspaceName,
ReplicaCollection<?> replicas)
*/
private boolean useStrictSourcesForRanges(AbstractReplicationStrategy
strat)
{
- return useStrictConsistency
- && tokens != null
- && metadata.getSizeOfAllEndpoints() !=
strat.getReplicationFactor().allReplicas;
+ boolean res = useStrictConsistency && tokens != null;
+
+ if (res)
+ {
+ int nodes = 0;
+
+ if (strat instanceof NetworkTopologyStrategy)
Review comment:
Sorry, I am not sure what the conditions should be, just to make sure:
DC1:3, DC2:3 with DC1 having 2 and DC2 having 6 nodes => res = True?
Also the condition before change was `!=` and now it is `>`?
##########
File path:
src/java/org/apache/cassandra/cql3/statements/schema/AlterKeyspaceStatement.java
##########
@@ -78,7 +82,14 @@ public Keyspaces apply(Keyspaces schema)
validateNoRangeMovements();
validateTransientReplication(keyspace.createReplicationStrategy(),
newKeyspace.createReplicationStrategy());
- return schema.withAddedOrUpdated(newKeyspace);
+ Keyspaces res = schema.withAddedOrUpdated(newKeyspace);
+ if (ClientWarn.instance.getWarnings() != null)
Review comment:
with proposed earlier method this will be easier:
```
int newSize = ClientWarn.instance.numWarnings();
if (newSize > previousWarningSize)
{
clientWarnings.addAll(...)
}
```
but even better it can all be probably extraceted into some new `ClientWarn`
method,
that would return "getWarningsAfter(previousWarningSize)" and return some
list or empty list
depending on the conditions.
Then this code would become simply:
```
clientWarnings.addAll(ClientWarn.instance.getWarningsAfter(previousWarningSize));
```
##########
File path:
test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java
##########
@@ -269,6 +276,63 @@ public void testCreateAlterKeyspaces() throws Throwable
"max_threshold", "32")));
}
+ @Test
+ public void testCreateAlterKeyspacesRFWarnings() throws Throwable
+ {
+ requireNetwork();
+ Thread.sleep(1000); // Hack: requireNetwork() will clear warnings
async so give some time to complete
Review comment:
so in this case maybe we should
- poll until warnings are cleared, or
- assert that warnings are cleared after this sleep.
----------------------------------------------------------------
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]
