forked from apache/cassandra-python-driver
-
Notifications
You must be signed in to change notification settings - Fork 56
Implement TABLETS_ROUTING_V2 #913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
dawmd
wants to merge
10
commits into
scylladb:master
Choose a base branch
from
dawmd:leader-awareness
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
2bf04aa
Negotiate the TABLETS_ROUTING_V2 protocol extension
dawmd d931138
Track tablet_version and encode the tablet_version_block
dawmd 37c8bd3
Memoize the routing token on Statement
dawmd 4353f4b
Compute and serialize the tablet_version_block per EXECUTE
dawmd eeb99b2
Add integration tests for TABLETS_ROUTING_V2
dawmd 62a0364
Document TABLETS_ROUTING_V2 tablet-version tracking
dawmd ade7832
Expose a private _strongly_consistent flag on keyspace metadata
dawmd 83f3286
Route requests to the tablet leader for strongly-consistent tables
dawmd 60aaf61
Add integration tests for leader-aware routing
dawmd 6133e77
Document leader-aware routing
dawmd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a real pity that we need to perform yet another fetch from system_schema in order to obtain this information - especially when strong consistency is still experimental, people will be paying for support for a feature which they not only do not use, but can't use.
Maybe we should have considered sending some information about whether a table uses strong consistency or not in the prepared statement. I think this could also apply to information like the partitioner and whether a table uses tablets or not. This is, of course, out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. However, if we want to avoid it, we need to modify the server-code before we can merge this PR since we need some way to learn that a table is strongly consistent. I can start working on it in parallel of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that modifications like those I propose would require another round of design review. I don't think we should be implementing this at the moment. The other suggestion about skipping the query to
scylla_keyspacesshould make the current PR palatable enough, we will only pay the cost on experimental clusters with strong consistency enabled.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, we can remove this code for the time being. Unfortunately, the consequence will be ditching all leader awareness from the driver, but it shouldn't be difficult to add it back later on. If that's acceptable, I can proceed with the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would defeat the whole point of this PR.
What I meant by my "we should not be implementing this at the moment" is that we should not be working on further extending the protocol in the way as I suggested in my first message. I did not mean to drop the code that this conversation is attached to (cassandra/metadata.py, lines 2590-2593), we need it to distinguish whether it's a strongly consistent table or not and whether to do leader awareness routing or not.
In #913 (comment) I suggested that we can skip issuing the query if we know that the cluster does not support strong consistency. If we do this, we will not incur the cost for regular users who are not testing strong consistency at the moment. While not great, I don't think an additional metadata query is tragic; we still have some time before release of strong consistency to address it (if there will be a need to address it at all, given the python-over-rust effort).
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gated behind the protocol extension in v3. Leaving the discussion as open since this is still something we might want to improve. It should be easier to remember this way until we create an issue.