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.

Sign up for GitHub

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

Jump to bottom

Migrate vreplication commands to vtctldclient: Mount and Migrate #14174

Merged
merged 7 commits into from
Oct 6, 2023
Merged

Migrate vreplication commands to vtctldclient: Mount and Migrate #14174

merged 7 commits into from
Oct 6, 2023

Conversation

rohit-nayak-ps
Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps commented Oct 3, 2023 •
edited by mattlord
Loading

Description

vtctldclient is replacing the legacy vtctlclient. As part of that effort this PR migrates the Mount and Migrate client commands to vtctldclient.

Related Issue(s)

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on the CI
  • Documentation was added or is not required

Deployment Notes

@rohit-nayak-ps rohit-nayak-ps added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: VReplication Skip CI Skip CI actions from running labels Oct 3, 2023
@vitess-bot
Copy link
Contributor

vitess-bot bot commented Oct 3, 2023

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@vitess-bot vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Oct 3, 2023
@github-actions github-actions bot added this to the v19.0.0 milestone Oct 3, 2023
@rohit-nayak-ps rohit-nayak-ps removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says Skip CI Skip CI actions from running NeedsIssue A linked issue is missing for this Pull Request labels Oct 4, 2023
@rohit-nayak-ps rohit-nayak-ps force-pushed the rohit/migrate-mount-migrate branch from c530ab3 to c5d5151 Compare October 4, 2023 18:28
@rohit-nayak-ps rohit-nayak-ps changed the title (WIP) Migrate vreplication commands to vtctldclient: Mount and Migrate Migrate vreplication commands to vtctldclient: Mount and Migrate Oct 4, 2023
@rohit-nayak-ps rohit-nayak-ps force-pushed the rohit/migrate-mount-migrate branch 2 times, most recently from 1c1014b to 6a3c8e2 Compare October 4, 2023 19:58
@rohit-nayak-ps rohit-nayak-ps marked this pull request as ready for review October 4, 2023 21:07
@rohit-nayak-ps rohit-nayak-ps requested review from shlomi-noach and removed request for harshit-gangal and notfelineit October 4, 2023 21:08
mattlord
mattlord reviewed Oct 5, 2023
View reviewed changes
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.

Overall it looks great! I had some minor questions, comments, suggestions. We can discuss and I'll quickly do another review.

@rohit-nayak-ps
Migrate Mount and Migrate to vtctldclient
b57b5d3 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>

WIP

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Working Mount implementation. Need to fix/improve e2e test

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Implement/fix Migrate commands

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Use different proto for MigrateCreate instead of reusing MoveTablesCreate's proto

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Minor refactor

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Update tests to use json parsing

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps
Fix help text. Add new/renamed e2e tests to test json
c5ab335 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps
Address review comments. Add Show/Progress commands to Migrate
43b72d2 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps rohit-nayak-ps force-pushed the rohit/migrate-mount-migrate branch from 8372ca1 to 43b72d2 Compare October 5, 2023 14:25
@rohit-nayak-ps
Address more review comments
3c2408e 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@mattlord mattlord self-requested a review October 5, 2023 17:45
mattlord
mattlord approved these changes Oct 5, 2023
View reviewed changes
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.

LGTM! I only had minor comments/requests/suggestions so I will let you address those as you feel is best before merging.

go/cmd/vtctldclient/command/vreplication/migrate/migrate.go Outdated Show resolved Hide resolved
go/cmd/vtctldclient/command/vreplication/mount/mount.go Show resolved Hide resolved
go/cmd/vtctldclient/command/vreplication/mount/mount.go Outdated Show resolved Hide resolved
go/cmd/vtctldclient/command/vreplication/mount/mount.go Show resolved Hide resolved
go/cmd/vtctldclient/command/vreplication/mount/mount.go Outdated Show resolved Hide resolved
go/test/endtoend/vreplication/migrate_test.go Outdated Show resolved Hide resolved
go/test/endtoend/vreplication/migrate_test.go Show resolved Hide resolved
go/vt/vtctl/grpcvtctldserver/server.go Outdated Show resolved Hide resolved
go/vt/vtctl/workflow/mount.go Outdated Show resolved Hide resolved
go/vt/vtctl/workflow/mount.go Outdated Show resolved Hide resolved
@rohit-nayak-ps
Address more review comments
70b50d4 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps
Generate protos/help files
754dd35 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps rohit-nayak-ps requested a review from a team October 5, 2023 21:15
@deepthi deepthi modified the milestones: v19.0.0, v18.0.0 Oct 5, 2023
ajm188
ajm188 approved these changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@ajm188 ajm188 left a comment

Choose a reason for hiding this comment

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

approving pending the help-text consistency

go/cmd/vtctldclient/command/vreplication/mount/mount.go Outdated Show resolved Hide resolved
go/cmd/vtctldclient/command/vreplication/mount/mount.go Outdated Show resolved Hide resolved
go/cmd/vtctldclient/command/vreplication/mount/mount.go Outdated Show resolved Hide resolved
go/cmd/vtctldclient/command/vreplication/mount/mount.go Outdated Show resolved Hide resolved
go/cmd/vtctldclient/command/vreplication/mount/mount.go Outdated Show resolved Hide resolved
@rohit-nayak-ps
Address review comments
8ef9d80 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps rohit-nayak-ps merged commit f4b8361 into vitessio:main Oct 6, 2023
118 checks passed
@rohit-nayak-ps rohit-nayak-ps deleted the rohit/migrate-mount-migrate branch October 6, 2023 20:06
rohit-nayak-ps added a commit to planetscale/vitess that referenced this pull request Oct 8, 2023
@rohit-nayak-ps
Migrate vreplication commands to vtctldclient: Mount and Migrate (vit…
d132da6 
…essio#14174)

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps rohit-nayak-ps mentioned this pull request Oct 8, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattlord mattlord mattlord approved these changes

@ajm188 ajm188 ajm188 approved these changes

@deepthi deepthi Awaiting requested review from deepthi deepthi is a code owner

@GuptaManan100 GuptaManan100 Awaiting requested review from GuptaManan100 GuptaManan100 is a code owner

@frouioui frouioui Awaiting requested review from frouioui frouioui is a code owner

@shlomi-noach shlomi-noach Awaiting requested review from shlomi-noach

Assignees
No one assigned
Labels
Component: VReplication Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Milestone
v18.0.0
Development

Successfully merging this pull request may close these issues.

Migrate Migrate Command to Vtctldclient Migrate Mount Command to Vtctldclient
4 participants
@rohit-nayak-ps @mattlord @ajm188 @deepthi