adelapena commented on a change in pull request #870:
URL: https://github.com/apache/cassandra/pull/870#discussion_r570300307
##########
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:
I haven't been able to reproduce this. Is it the problem that the
warnings are `null` if `requireNetwork` hasn't finished, or that there are
other previous contents in them?
##########
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 previousSize = ClientWarn.instance.getWarnings() == null ? 0 :
ClientWarn.instance.getWarnings().size();
Review comment:
Nit: maybe we could name this using the `warnings` word, something like
`previousNumWarnings`, or `previousWarningsSize`? Feel free to ignore if you
don't agree.
##########
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
+
+ // NTS
+ ClientWarn.instance.captureWarnings();
+ execute("CREATE KEYSPACE testABC WITH replication = {'class' :
'NetworkTopologyStrategy', '" + DATA_CENTER + "' : 3 }");
Review comment:
Not a big deal, but we could use
[`CQLTester#createKeyspace`](https://github.com/apache/cassandra/blob/trunk/test/unit/org/apache/cassandra/cql3/CQLTester.java#L692)
instead of `execute`, as it's done
[here](https://github.com/adelapena/cassandra/commit/a42317a0e3ccf5d291b6629ee0c318298dcb994c)
as an example.
The advantage of `createKeyspace` is that it reduces the likelihood of
collisions with other tests using the same keyspace names (IIRC we have had
problems with this in the past). Also, `CQLTester` conditionally [drops the
keyspaces](https://github.com/apache/cassandra/blob/trunk/test/unit/org/apache/cassandra/cql3/CQLTester.java#L344)
created with `createKeyspace` after each test, so we wouldn't need the `DROP
KEYSPACE` queries at the end of this tests. These drops will be executed even
if the test fails, as if they were in a `finally` block, which is also an
advantage.
##########
File path:
src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java
##########
@@ -299,6 +298,8 @@ public RangesAtEndpoint
getPendingAddressRanges(TokenMetadata metadata, Collecti
public abstract void validateOptions() throws ConfigurationException;
+ public abstract void maybeWarnOnOptions() throws ConfigurationException;
Review comment:
Since the purpose of the method is just warn about stuff I guess it
won't raise `ConfigurationException`
----------------------------------------------------------------
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]
