-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Implement "Tablet Targeting" RFC #18809
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
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18809 +/- ##
==========================================
- Coverage 69.91% 69.90% -0.01%
==========================================
Files 1613 1613
Lines 216071 216124 +53
==========================================
+ Hits 151073 151089 +16
- Misses 64998 65035 +37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f77286f to
1eb17dd
Compare
84f29f0 to
6c49907
Compare
Signed-off-by: Nick Van Wiggeren <[email protected]>
…re it is not valid to set Signed-off-by: Nick Van Wiggeren <[email protected]>
Signed-off-by: Nick Van Wiggeren <[email protected]>
Signed-off-by: Nick Van Wiggeren <[email protected]>
5b2df7d to
25a0e9c
Compare
Signed-off-by: Nick Van Wiggeren <[email protected]>
6aa9310 to
5c0bc2d
Compare
Signed-off-by: Nick Van Wiggeren <[email protected]>
|
I believe this is ready for another review @harshit-gangal! Let me know if you have any questions. |
|
|
||
| For examples and more details, see the [documentation](https://vitess.io/docs/24.0/reference/compatibility/mysql-compatibility/#window-functions). | ||
|
|
||
| #### <a id="tablet-targeting"/>Tablet targeting via USE statement</a> |
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.
@nickvanw let's say I use this feature to write rows to a PRIMARY that doesn't align with the VSchema, am I creating any data correctness/etc risks we should detail here?
I'm wondering if that would cause query failures or potentially-surprising results. But at the same time, I guess you can only use this feature with intention 🤔
And on a similar track, could vReplication get upset about rows in the wrong shard? One dreamed-up scenario I can think of is merging shards, and the same primary key exists in both, etc
None of what I'm thinking means changing the code here I think (it looks good so far), maybe just documentation
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.
let's say I use this feature to write rows to a
PRIMARYthat doesn't align with the VSchema, am I creating any data correctness/etc risks we should detail here?
I don't think the introduction of this feature changes this - you can already target specific shards and side-step any validation that Vitess would normally do against the queries you are executing (like ensuring inserts only are routed to the "correct" shard).
|
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
|
I'm still working on this! If you have time for review @harshit-gangal, I would appreciate it 😁 |
mattlord
left a comment
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.
LGTM! Just some minor comments/questions/suggestions.
|
|
||
| logging *ExecuteLogger | ||
|
|
||
| // targetTabletAlias is set when using tablet-specific routing via USE keyspace@tablet-alias. |
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 must contain the shard too, no? keyspace:shard@tablet-alias
I'm also not sure when/why you would specify the tablet type in this context from the changelog info. When I read that, I thought that the shard AND tablet type would be required and we would confirm that it still matches when doing the routing.
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.
Yes! This was an older comment, the shard and the tablet type are required (in part, to avoid accidentally trying to write to a replica, etc)
| } | ||
|
|
||
| // TestTargetTabletAlias tests the SetTargetTabletAlias and GetTargetTabletAlias methods. | ||
| func TestTargetTabletAlias(t *testing.T) { |
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.
This feels like overkill to me -- a test for a setter/getter -- but obviously fine too.
arthurschreiber
left a comment
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.
LGTM ❤️
- Fix comment in safe_session.go to use correct syntax - Add bounds check for empty string after @ in ParseDestination - Use require.EqualError instead of manual error checking in tests - Change %v to %s for string formatting in error message - Use require.ErrorContains instead of require.Error + require.Contains Signed-off-by: Nick Van Wiggeren <[email protected]>
Signed-off-by: Nick Van Wiggeren <[email protected]>
Signed-off-by: Nick Van Wiggeren <[email protected]>
|
📝 Documentation updates detected! New suggestion: Document tablet-specific targeting via USE statement |
Description
This PR adds the ability for a user to target a specific tablet by alias when doing
USE, instead of only being able to pick a type for a shard. This means that a user can get "stable" routing, which is extremely useful when they don't have access to the underlying MySQL instance and need to do things like debug.Related Issue(s)
Checklist
AI Disclosure
This PR was written hand-in-hand with Claude Code, which absolutely got the initial few approaches extremely incorrect and required a lot of human help. Robot isn't going to take my job any time soon.