Skip to content
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

Add a new field called "name" to VitessShardTabletPool as part of x-kubernetes-list-map-keys #432

Merged

Conversation

yoheimuta
Copy link
Contributor

@yoheimuta yoheimuta commented Jun 14, 2023

Description

This PR introduces a new field named "index" "name" to the x-kubernetes-list-map-keys of TabletPools. It enables the presence of multiple TabletPool instances with a specific tablet type in a given cell.

By assigning a default value of 0 an empty string to the index name, the existing users and most use cases remain unaffected. The index name value can be explicitly set to a value greater than 0 something only when building a redundant system with unmanaged tablets.

Related Issue(s)

@deepthi
Copy link
Collaborator

deepthi commented Jun 14, 2023

index seems like an anti-pattern. Any code that is fetching the objects will get a slice with each object having its own index in that slice. Why not use name as the identifier?
Also, we should limit this to externalDatastore. It does not make sense for Vitess-managed keyspaces.

@yoheimuta
Copy link
Contributor Author

Thank you for the review! I will address these two points and make the necessary fixes.

@yoheimuta yoheimuta force-pushed the support-more-VitessShardTabletPool branch from 1bddd17 to 3695721 Compare June 21, 2023 04:05
@yoheimuta yoheimuta changed the title Add a new field called "index" to VitessShardTabletPool as part of x-kubernetes-list-map-keys Add a new field called "name" to VitessShardTabletPool as part of x-kubernetes-list-map-keys Jun 21, 2023
@yoheimuta
Copy link
Contributor Author

@deepthi I've made the fixes about naming and limitation: a042c49 3695721

@yoheimuta
Copy link
Contributor Author

@deepthi Hi, is there anything missing in this PR?

@@ -43,11 +43,30 @@ lead time to develop a smarter way to handle tablet identity and MySQL server
IDs in Vitess itself.

WARNING: DO NOT change the behavior of this function, as that may result in
the deletion and recreation of all tablets.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can remove this extra line i think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your review!
I fixed it: 62ea41d

Copy link
Member

@frouioui frouioui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! thank you @yoheimuta

Signed-off-by: Yohei Yoshimuta <[email protected]>
Copy link
Collaborator

@GuptaManan100 GuptaManan100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@GuptaManan100 GuptaManan100 merged commit a3e5c8b into planetscale:main Oct 9, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Allow more than one VitessShardTabletPool with a certain tablet type in a given cell
4 participants