yifan-c commented on code in PR #226:
URL:
https://github.com/apache/cassandra-analytics/pull/226#discussion_r4235453998
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/data/CassandraDataLayer.java:
##########
@@ -1189,7 +1189,9 @@ protected Sizing
getSizing(CompletableFuture<RingResponse> ringFuture,
ReplicationFactor replicationFactor,
ClientConfig options)
{
- return SizingFactory.create(replicationFactor, options,
consistencyLevel, keyspace, table, datacenter, sidecar, sidecarPort,
ringFuture);
+ return SizingFactory.create(replicationFactor, options,
consistencyLevel,
+ maybeQuotedKeyspace, maybeQuotedTable,
datacenter,
+ sidecar, sidecarPort, ringFuture);
Review Comment:
Please drop this change. It is unrelated to the fix for the restore path.
Sizing is backed by TableStatsHandler and JMX internally in Sidecar. Quoting
the name could regress.
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/data/QualifiedTableName.java:
##########
@@ -65,6 +65,14 @@ public String keyspace()
return keyspace;
}
+ /**
+ * @return the keyspace name, quoted with double quotes when {@code
quoteIdentifiers} is set
+ */
+ public String maybeQuotedKeyspace()
+ {
+ return maybeQuote(keyspace);
+ }
+
Review Comment:
Instead of adding the `maybeQuotedX" methods, could you please use the
helper
`org.apache.cassandra.bridge.CassandraBridgeFactory#maybeQuotedIdentifier` for
code consistency? You can find code example in
`org.apache.cassandra.spark.bulkwriter.SidecarDataTransferApi`.
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/CassandraClusterInfo.java:
##########
@@ -289,11 +289,12 @@ protected String getCurrentKeyspaceSchema() throws
Exception
private TokenRangeReplicasResponse getTokenRangesAndReplicaSets()
{
CassandraContext context = getCassandraContext();
+ String maybeQuotedKeyspace = maybeQuotedIdentifier(bridge(),
conf.quoteIdentifiers, conf.keyspace);
try
{
long start = System.nanoTime();
TokenRangeReplicasResponse response = context.getSidecarClient()
-
.tokenRangeReplicas(new ArrayList<>(context.getCluster()), conf.keyspace)
+
.tokenRangeReplicas(new ArrayList<>(context.getCluster()), maybeQuotedKeyspace)
Review Comment:
Please drop this change.
Mixed-case is already proven working in
`org.apache.cassandra.analytics.QuoteIdentifiersWriteTest`. Quoting the name
could cause regression.
In Sidecar, `TokenRangeReplicaMapHandler` retrieves range info from JMX
endpoints, which take case-sensitive string and require no quotes.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
