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

Rename WORKER tablet type into DRAINED #2151

Merged
merged 2 commits into from
Oct 14, 2016
Merged

Conversation

pivanof
Copy link
Contributor

@pivanof pivanof commented Oct 13, 2016

This will make the type look more generic, so that it could be used for other
purposes as well, not only for a worker during resharding. In particular I plan
to use the same tablet type for schema swap reserving a seed vttablet to execute
the schema change.
I also add a "drain_reason" tag that will be set by anyone setting the DRAINED
tablet type, so that it's easy to understand why it is drained.

@mberlin-bot
Copy link

mberlin-bot commented Oct 13, 2016

:lgtm:


Reviewed 6 of 6 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


go/vt/worker/topo_utils.go, line 121 at r1 (raw file):

Used by worker

nit: Please write "vtworker" instead of "worker" because only the Go package is called "worker". Other than that, we always refer to it by "vtworker".


Comments from Reviewable

Approved with PullApprove

@michael-berlin
Copy link
Contributor

I like this idea.

Alain, any objections on this one?

@pivanof-bot
Copy link

Review status: 8 of 9 files reviewed at latest revision, 1 unresolved discussion.


go/vt/worker/topo_utils.go, line 121 at r1 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

Used by worker

nit: Please write "vtworker" instead of "worker" because only the Go package is called "worker". Other than that, we always refer to it by "vtworker".

Done.

Comments from Reviewable

Copy link
Contributor

@alainjobart alainjobart left a comment

Choose a reason for hiding this comment

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

LGTM I don't think we use the worker type name explicitly anywhere.

@@ -128,17 +129,18 @@ func FindWorkerTablet(ctx context.Context, wr *wrangler.Wrangler, cleaner *wrang
// ChangeSlaveType back, so we need to record this tag change after the change
// slave type change in the cleaner.
defer wrangler.RecordTabletTagAction(cleaner, tabletAlias, "worker", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that these Tablet record changes are not atomic. We update the tablet record 2 times upon start, and 3 times upon cleaning. The extra tag makes it worse, but the problem was already there. I guess we can fix that when we switch to having an RPC to make a tablet drained.

This will make the type look more generic, so that it could be used for other
purposes as well, not only for a worker during resharding. In particular I plan
to use the same tablet type for schema swap reserving a seed vttablet to execute
the schema change.
I also add a "drain_reason" tag that will be set by anyone setting the DRAINED
tablet type, so that it's easy to understand why it is drained.
@pivanof pivanof merged commit 3c90125 into vitessio:master Oct 14, 2016
@pivanof pivanof deleted the drained branch October 14, 2016 22:09
pivanof added a commit to pivanof/vitess that referenced this pull request Nov 30, 2016
The site needed publishing after vitessio#2312 and maybe after some other changes.

On the way I fixed a couple of incorrect things in vtctl docs (typo in an error
message and change of tablet type name WORKER->DRAINED that happened in vitessio#2151).
pivanof added a commit that referenced this pull request Nov 30, 2016
The site needed publishing after #2312 and maybe after some other changes.

On the way I fixed a couple of incorrect things in vtctl docs (typo in an error
message and change of tablet type name WORKER->DRAINED that happened in #2151).
frouioui pushed a commit to planetscale/vitess that referenced this pull request Nov 21, 2023
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.

6 participants