Repository navigation
CASSANALYTICS-183: Pass quoted identifiers to sidecar API calls for mixed-case keyspace/table restore - #226
bianca-stanciu29 wants to merge 7 commits into
Conversation
… does not exist when quoteIdentifiers is set
| private TokenRangeReplicasResponse getTokenRangesAndReplicaSets() | ||
| { | ||
| CassandraContext context = getCassandraContext(); | ||
| String quotedKeyspace = maybeQuotedIdentifier(bridge(), conf.quoteIdentifiers, conf.keyspace); |
There was a problem hiding this comment.
nit: change name to maybeQuotedKeyspace?
There was a problem hiding this comment.
Done, renamed the local variable to maybeQuotedKeyspace in CassandraClusterInfo.
| } | ||
|
|
||
| @Test | ||
| void testCreateRestoreJobUsesQuotedIdentifiers() throws Exception |
There was a problem hiding this comment.
What do you think about adding quoted identifiers tests to the other modified APIs?
There was a problem hiding this comment.
Done, added tests for quoted identifiers in the direct and coordinated restore-slice requests, coordinated restore-job progress, cluster schema and token-range requests, and dynamic sizing/table-stats calls.
| /** | ||
| * @return the keyspace name, quoted with double quotes when {@code quoteIdentifiers} is set | ||
| */ | ||
| public String maybeQuotedKeyspace() |
There was a problem hiding this comment.
@bbotella @yifan-c @frankgh I think we should keep keyspace() and table() as raw-name accessors. They’re also used for SSTable metadata, local data-layer setup, and job statistics, so returning quoted names would require checking those callers too. The separate maybeQuoted accessors let us apply quoting when building Sidecar requests without changing other uses. CASSSIDECAR-475 fixes the metadata lookup inside Sidecar, but that’s separate from what these analytics accessors return. What do you think?
Rename the conditional keyspace value and verify quoted names in restore, coordinated, topology, and sizing requests.
yifan-c
changed the title
yifan-c
left a comment
yifan-c
left a comment
There was a problem hiding this comment.
First of all, thanks for sending the fix! It is real. I'd apologies for the belated review as well.
Please see my comments inline.
| /** | ||
| * @return the keyspace name, quoted with double quotes when {@code quoteIdentifiers} is set | ||
| */ | ||
| public String maybeQuotedKeyspace() | ||
| { | ||
| return maybeQuote(keyspace); | ||
| } | ||
|
|
There was a problem hiding this 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.
| .tokenRangeReplicas(new ArrayList<>(context.getCluster()), conf.keyspace) | ||
| .tokenRangeReplicas(new ArrayList<>(context.getCluster()), maybeQuotedKeyspace) |
There was a problem hiding this 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.
| return SizingFactory.create(replicationFactor, options, consistencyLevel, keyspace, table, datacenter, sidecar, sidecarPort, ringFuture); | ||
| return SizingFactory.create(replicationFactor, options, consistencyLevel, | ||
| maybeQuotedKeyspace, maybeQuotedTable, datacenter, | ||
| sidecar, sidecarPort, ringFuture); |
There was a problem hiding this 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.
3 participants
Problem
When quoteIdentifiers=true, Analytics must preserve case-sensitive
keyspace and table names when constructing Sidecar requests.
Some request paths were passing raw names instead of quoted identifiers.
For example, a keyspace created as "MyKeyspace" could be looked up as
mykeyspace, causing restore operations to fail with:
Solution
Add maybeQuotedKeyspace() and maybeQuotedTable() to QualifiedTableName,
reusing its existing maybeQuote() helper.
Use these accessors when constructing cloud-storage restore requests:
Also apply identifier quoting to bulk-writer token-range requests and
pass the existing quoted identifiers into reader sizing/table-statistics
requests.
Compatibility
Keep keyspace() and table() as raw-name accessors so existing callers
outside the Sidecar request boundaries retain their current behavior.
The new accessors apply quoting only when quoteIdentifiers=true.
When quoteIdentifiers=false, names remain unchanged.
Related Sidecar change
CASSSIDECAR-475 addresses identifier resolution during metadata lookup
inside Sidecar. This PR addresses how Analytics supplies identifiers
to the affected Sidecar APIs.
https://issues.apache.org/jira/browse/CASSSIDECAR-475