[CASSANDRA-19272] Adds integration testing for blocklist handling #32
Conversation
frankgh
left a comment
frankgh
left a comment
There was a problem hiding this comment.
Looks good, I've left a few comments in the PR
| * support mixed case and reserved keyword names for these fields. | ||
| */ | ||
| QUOTE_IDENTIFIERS, | ||
| BLOCKED_INSTANCES, |
There was a problem hiding this comment.
how about calling them BLOCKED_SIDECAR_INSTANCES to be explicit about which instances are blocked. Also, I think we should add javadocs to all these properties (in a different jira); but in the context of this PR, we should add javadocs to the property that is being added.
There was a problem hiding this comment.
on second thought these are actually Cassandra instances, correct? I think we should maybe explicitly call them BLOCKED_CASSANDRA_INSTANCES maybe? I don't think Sidecar itself is blocked, but rather specific Cassandra instances, right?
There was a problem hiding this comment.
Sure, doesn't hurt to make it more explicit.
...ics-integration-tests/src/test/java/org/apache/cassandra/analytics/BlockedInstancesTest.java
Outdated
Show resolved
Hide resolved
| { | ||
| if (method.isAnnotationPresent(Test.class)) | ||
| { | ||
| createTestTable(new QualifiedName(TEST_KEYSPACE, method.getName().toLowerCase()), CREATE_TEST_TABLE_STATEMENT); |
There was a problem hiding this comment.
there are some limitations on valid keyspace/table names in Cassandra, including length, so we need to be a bit careful there. but this looks like a clever approach to setup schemas
There was a problem hiding this comment.
good point. Added a class level javadoc describing what we're doing for now.
...ics-integration-tests/src/test/java/org/apache/cassandra/analytics/BlockedInstancesTest.java
Outdated
Show resolved
Hide resolved
...ics-integration-tests/src/test/java/org/apache/cassandra/analytics/BlockedInstancesTest.java
Outdated
Show resolved
Hide resolved
...ics-integration-tests/src/test/java/org/apache/cassandra/analytics/BlockedInstancesTest.java
Outdated
Show resolved
Hide resolved
...ics-integration-tests/src/test/java/org/apache/cassandra/analytics/BlockedInstancesTest.java
Outdated
Show resolved
Hide resolved
...ics-integration-tests/src/test/java/org/apache/cassandra/analytics/BlockedInstancesTest.java
Outdated
Show resolved
Hide resolved
arjunashok
changed the title
arjunashok
changed the title
|
Closed via fa6df8e |
2 participants
Changes
blocked_instances