Add table-aware GetReplicas API and deprecate legacy overloads#207
Add table-aware GetReplicas API and deprecate legacy overloads#207sylwiaszunejko wants to merge 1 commit into
Conversation
1fa4346 to
6117c06
Compare
There was a problem hiding this comment.
Pull request overview
This PR centralizes “tablet-aware” replica resolution behind a new GetReplicas(keyspace, table, partitionKey) API, and updates token-aware query planning and tests to use it.
Changes:
- Added
Metadata.GetReplicas(string keyspaceName, string tableName, byte[] partitionKey)with tablet-map-first resolution and token-map fallback; marked older overloads as[Obsolete]. - Extended
ICluster/Clusterwith a newGetReplicas(string keyspace, string table, byte[] partitionKey)overload and deprecated older overloads. - Updated
TokenAwarePolicyand related unit/integration tests to call the new overload.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Cassandra/Policies/TokenAwarePolicy.cs | Uses the new cluster replica API (keyspace + table) during query planning. |
| src/Cassandra/Metadata.cs | Implements tablet-aware replica resolution and deprecates older overloads. |
| src/Cassandra/ICluster.cs | Adds new replica-resolution overload and marks older methods obsolete. |
| src/Cassandra/Cluster.cs | Implements new interface overload and routes obsolete overloads through the new method. |
| src/Cassandra.Tests/StatementTests.cs | Updates mock setup/verification for new overload. |
| src/Cassandra.Tests/PoliciesUnitTests.cs | Updates mocks for new overload in token-aware policy tests. |
| src/Cassandra.IntegrationTests/MetadataTests/TokenMapTopologyChangeTests.cs | Updates calls to pass table name (null) to new overload. |
| src/Cassandra.IntegrationTests/MetadataTests/TokenMapSchemaChangeTests.cs | Same as above. |
| src/Cassandra.IntegrationTests/MetadataTests/TokenMapSchemaChangeMetadataSyncTests.cs | Same as above. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
6117c06 to
d48a222
Compare
b17fdd2 to
6918688
Compare
There was a problem hiding this comment.
Let's do the following with API:
- Have API on Metadata per sharding type: vnode, tablets, (maybe later we will add StrongConsistency)
- Each of these APIs won't assert on table/ks configuration or anything like that and if information is missing it will not fallback
- Have a single generic API (on Cluster or Session) that allows you to get replicas for all the cases that will pull table/ks config and use proper API, so that user do not need to look or know sharding configuration.
Why? Why not make Metadata have public APIs that are robust and can handle all cases? |
@dkropachev @wprzytula @Lorak-mmk as for now I don't think any driver have think kind of API, shouldn't this be discussed on cross driver lvl and for now we should just have Metadata lvl API that handles all cases? WDYT? |
6918688 to
1dd27b7
Compare
|
I agree. I also don't see much point in those granular APIs, its just more opportunity for user to shoot themselves in the foot. |
We will defenitely need this API when we will start implementing batching logic, to split queries into a batches, but we can defere having this PR for now. |
| if (!string.IsNullOrEmpty(keyspaceName) && !string.IsNullOrEmpty(tableName)) | ||
| { |
There was a problem hiding this comment.
| if (!string.IsNullOrEmpty(keyspaceName) && !string.IsNullOrEmpty(tableName)) | |
| { | |
| if (string.IsNullOrEmpty(keyspaceName) || string.IsNullOrEmpty(tableName)) | |
| { | |
| return _tokenMap.GetReplicas(keyspaceName, token); | |
| } |
| var token = _tokenMap.Factory.Hash(partitionKey); | ||
| if (!string.IsNullOrEmpty(keyspaceName) && !string.IsNullOrEmpty(tableName)) | ||
| { | ||
| var tabletReplicas = TabletMap.GetReplicas(keyspaceName, tableName, token); |
There was a problem hiding this comment.
The problem here is that it will fallback to tokenmap when tablets are empty, which is acceptable, but then we need to create an issue to address it later.
There was a problem hiding this comment.
What else should we do? I believe Rust Driver assumes token ring routing if there's no tablet information for given token.
There was a problem hiding this comment.
Correction: it uses token ring routing if there is no tablet info for given table. If there are some tablets for given table, but there is no tablet for given token, it will use non-token-aware routing.
There was a problem hiding this comment.
- What does it mean non-token-aware routing ?
- AFAIR a year ago we have discussed exactly this issue and came to conclusion not to reply with token ring replicas in such case, but return empty replicas instead
There was a problem hiding this comment.
What does it mean non-token-aware routing ?
It means routing that we would do for non-token-aware query. I'm not sure what is not clear about that tbh.
In Rust Driver it is the same as having empty replica list, and in fact it is how we do things underneath:
if let Some(tablets) = self.tablets.tablets_for_table(table_spec) {
let replicas: Option<&[(Arc<Node>, Shard)]> = if let Some(datacenter) = datacenter {
tablets.dc_replicas_for_token(token, datacenter)
} else {
tablets.replicas_for_token(token)
};
ReplicaSet {
inner: ReplicaSetInner::PlainSharded(replicas.unwrap_or(
// The table is a tablet table, but we don't have information for given token.
// Let's just return empty set in this case.
&[],
)),
token,
}There was a problem hiding this comment.
What does it mean non-token-aware routing ?
It means routing that we would do for non-token-aware query. I'm not sure what is not clear about that tbh. In Rust Driver it is the same as having empty replica list, and in fact it is how we do things underneath:
if let Some(tablets) = self.tablets.tablets_for_table(table_spec) { let replicas: Option<&[(Arc<Node>, Shard)]> = if let Some(datacenter) = datacenter { tablets.dc_replicas_for_token(token, datacenter) } else { tablets.replicas_for_token(token) }; ReplicaSet { inner: ReplicaSetInner::PlainSharded(replicas.unwrap_or( // The table is a tablet table, but we don't have information for given token. // Let's just return empty set in this case. &[], )), token, }What happens if a client calls
get_token_endpointson a table that has never been queried?
I thought that when we discussed [that issue](scylladb/scylla-rust-driver#1129), we agreed on exactly this wording - that the driver would check the table metadata to determine whether the table uses tablets, rather than relying solely on the presence of tablets.
I clearly remember you pointing out that if we base it only on tablet presence, the user would not be able to tell whether the table is vnode-based with no tablets yet, or it is vnode-based.
There was a problem hiding this comment.
I remember the issue you linked, but I don't remember the discussion.
Quoting the linked issue:
If target table is not tablet-based it should return old vnode-based result
If target table is tablet-based it should return known tablet replicas for given token
if there is no tablet record for given token, it should return empty list
We do that in Rust Driver. Where we are misunderstanding each other is precise definition of " If target table is not tablet-based".
I understand it as " If target table is not tablet-based according to current driver knowledge". The thing is that driver knowledge is based solely on tablet hints currently.
Is that a big problem? I don't think so. The replica list is best-effort anyway (we might not have tablets for given token). Vnode-based list doesn't seem like a much bigger problem than an empty list to me. When you start using those replicas for routing and issuing queries, you'll get some hints soon anyway, and start routing properly (or getting empty lists). So there will be at most a few wrong queries.
We do have an issue open to fetch info about keyspace being tablet based or not: scylladb/scylla-rust-driver#1534
It is primarly to expose this info to the user, but we would also use it for routing, solving the issue above.
There was a problem hiding this comment.
Is that a big problem? I don't think so. The replica list is best-effort anyway (we might not have tablets for given token). Vnode-based list doesn't seem like a much bigger problem than an empty list to me. When you start using those replicas for routing and issuing queries, you'll get some hints soon anyway, and start routing properly (or getting empty lists). So there will be at most a few wrong queries.
It is important, it will work fine only if there is a flow of prepared queries towards target table, but it might be not the case, you never know how users will use it.
That is why it is imporant to make API as user-friendly as possible and as clear as possible.
Imagine yourself as a user, you have an API that returns proper info if it has enough information, but if it does not have it, it returns random BS that you can't even destinct from proper reply.
And on other hand API that returns nothing when it does not have information, now at least you can make decision what to do, error out, guess, skip or try to fill it.
There was a problem hiding this comment.
I agree that detecting tablet-based tables from schema makes sense, which is why we have an issue to address this.
Does it impact this PR in any way?
What batching logic do you mean? Is it the same as in scylladb/scylla-rust-driver#468? |
cfccc9e to
a6ab381
Compare
|
@dkropachev can you look at it again? I believe behavior I added is in line with Rust driver, and we should discuss granular APIs on cross driver level separately, do you agree? |
Yes exactly that. |
There is one thing left to clear out. |
@dkropachev what thing? In terms of batching logic, cannot we add additional API when implementing it? |
a6ab381 to
74ef1bc
Compare
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis change adds a tablet-aware Sequence Diagram(s)sequenceDiagram
participant TokenAwarePolicy
participant Cluster
participant Metadata
participant TabletMap
participant TokenMap
TokenAwarePolicy->>Cluster: GetReplicas(keyspace, table, rawRoutingKey)
Cluster->>Metadata: GetReplicas(keyspace, table, partitionKey)
Metadata->>Metadata: compute token from partitionKey
alt keyspace and table provided
Metadata->>TabletMap: GetReplicas(keyspace, table, token)
TabletMap-->>Metadata: tablet replicas (or empty)
end
alt tablet replicas found
Metadata-->>Cluster: tablet-based replicas
else fallback
Metadata->>TokenMap: GetReplicas(keyspace, token)
TokenMap-->>Metadata: token-map replicas
Metadata-->>Cluster: token-map replicas
end
Cluster-->>TokenAwarePolicy: replica set
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Cassandra.IntegrationTests/MetadataTests/TokenMapSchemaChangeMetadataSyncTests.cs (1)
189-228: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWrong keyspace variable in assertion.
Line 225 checks
GetReplicas(keyspaceName, ...), but the preceding lookup on line 223 useskeyspaceName1. This assertion should verifykeyspaceName1's replicas to actually test that all keyspaces (not justkeyspaceName) were refreshed.🐛 Proposed fix
replicas = newCluster.Metadata.TokenToReplicasMap.GetByKeyspace(keyspaceName1); Assert.AreEqual(newCluster.Metadata.Hosts.Sum(h => h.Tokens.Count()), replicas.Count); - Assert.AreEqual(3, newCluster.Metadata.GetReplicas(keyspaceName, null, Encoding.UTF8.GetBytes("123")).Count); + Assert.AreEqual(3, newCluster.Metadata.GetReplicas(keyspaceName1, null, Encoding.UTF8.GetBytes("123")).Count);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Cassandra.IntegrationTests/MetadataTests/TokenMapSchemaChangeMetadataSyncTests.cs` around lines 189 - 228, The assertion after the TokenToReplicasMap lookup is using the wrong keyspace variable. In TokenMap_Should_RefreshTokenMapForAllKeyspaces_When_RefreshSchemaWithoutKeyspaceIsCalled, update the GetReplicas check that follows the keyspaceName1 lookup so it verifies keyspaceName1 instead of keyspaceName, ensuring the test באמת confirms all keyspaces were refreshed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/Cassandra.IntegrationTests/MetadataTests/TokenMapSchemaChangeMetadataSyncTests.cs`:
- Around line 189-228: The assertion after the TokenToReplicasMap lookup is
using the wrong keyspace variable. In
TokenMap_Should_RefreshTokenMapForAllKeyspaces_When_RefreshSchemaWithoutKeyspaceIsCalled,
update the GetReplicas check that follows the keyspaceName1 lookup so it
verifies keyspaceName1 instead of keyspaceName, ensuring the test באמת confirms
all keyspaces were refreshed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 82bcc3cf-a385-480c-a01a-15c68f192f85
📒 Files selected for processing (11)
src/Cassandra.IntegrationTests/Core/MetadataTests.cssrc/Cassandra.IntegrationTests/MetadataTests/TokenMapSchemaChangeMetadataSyncTests.cssrc/Cassandra.IntegrationTests/MetadataTests/TokenMapSchemaChangeTests.cssrc/Cassandra.IntegrationTests/MetadataTests/TokenMapTopologyChangeTests.cssrc/Cassandra.IntegrationTests/Policies/Tests/LoadBalancingPolicyShortTests.cssrc/Cassandra.Tests/PoliciesUnitTests.cssrc/Cassandra.Tests/StatementTests.cssrc/Cassandra/Cluster.cssrc/Cassandra/ICluster.cssrc/Cassandra/Metadata.cssrc/Cassandra/Policies/TokenAwarePolicy.cs
74ef1bc to
96b0c7f
Compare
The current replica resolution APIs exposed by Metadata:
Because the current C# APIs do not accept a table name, the tablet-based path cannot be used. This effectively forces replica resolution to assume vnode-based replication, which is incorrect for tablet-enabled clusters.
This PR introduces new API:
Fixes: #197