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

Default buildinfo tablet tags to off #9187

Merged
merged 3 commits into from
Nov 11, 2021

Conversation

ajm188
Copy link
Contributor

@ajm188 ajm188 commented Nov 10, 2021

Signed-off-by: Andrew Mason [email protected]

Description

This is follow-up to #8973 and fixes #9186.

Without changing any of the flags in the local example:

❯ vtctlclient -server ":15999" ListAllTablets
zone1-0000000100 commerce 0 primary localhost:15100 localhost:17100 [] 2021-11-10T20:29:30Z
zone1-0000000101 commerce 0 replica localhost:15101 localhost:17101 [] <null>
zone1-0000000102 commerce 0 rdonly localhost:15102 localhost:17102 [] <null>

Related Issue(s)

Fixes #9186

Checklist

  • Should this PR be backported? no
  • Tests were added or are not required n/a
  • Documentation was added or is not required n/a

Deployment Notes

Copy link
Contributor

@mattlord mattlord left a comment

Choose a reason for hiding this comment

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

Thank you! ❤️

@mattlord
Copy link
Contributor

mattlord commented Nov 10, 2021

@ajm188 looks like we need to adjust the new unit test based on what build flags are being ignored on the tablet side: https://github.com/vitessio/vitess/blob/main/go/vt/vttablet/tabletmanager/tm_init_test.go#L513

Example test failure: https://github.com/vitessio/vitess/runs/4170844287?check_suite_focus=true

@ajm188
Copy link
Contributor Author

ajm188 commented Nov 10, 2021

oh right! i forgot about that, nice catch

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

Is it worth writing a unit test to ensure that when the skip flag is set to "", all the tags do make it into the tablet record?
Otherwise LGTM.

Signed-off-by: Andrew Mason <[email protected]>
@ajm188 ajm188 force-pushed the no-default-buildinfo-tags branch from a089032 to 451103b Compare November 10, 2021 23:11
@ajm188 ajm188 merged commit e83b2eb into vitessio:main Nov 11, 2021
@ajm188 ajm188 deleted the no-default-buildinfo-tags branch November 11, 2021 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix breaking change to ListAllTablets
3 participants