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

[vtadmin-api] Reorganize tablet-related functions into vtadmin/cluster/cluster.go #7553

Merged
merged 8 commits into from Feb 26, 2021
Merged

[vtadmin-api] Reorganize tablet-related functions into vtadmin/cluster/cluster.go #7553

merged 8 commits into from Feb 26, 2021

Conversation

doeg
Copy link
Contributor

@doeg doeg commented Feb 25, 2021

Description

Here's the refactoring I promised to do for #7528 (comment).

Changes:

  • Moves everything out of tablets.go into cluster.go so functions have a signature of func (c *Cluster) ...
  • Moves getTablets, findTablet, and findNTablets out of api.go and into cluster.go + adds tests
  • Moves errors.go out of package vtadmin and into package errors so that it can be shared by api.go + cluster.go

I staged this internally + it works as expected.

Related Issue(s)

Checklist

  • Should this PR be backported? No
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

N/A

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

@doeg
Add vtadmin/errors/errors.go
f222bbc 
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@doeg
Move ParseTablets to cluster.go
208484a 
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@doeg
Add cluster_test.go
5af13c0 
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@doeg
🐐 Nits
3815581 
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@doeg
Move BuildCluster to vtadmin/testutil
ad990e4 
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@doeg
Add TestFindTablets
e5b9db7 
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@doeg
Add TestFindTablet
4e8d5aa 
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
ajm188
ajm188 reviewed Feb 25, 2021
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.

I like this separation a lot better; thank you for doing it!! I have a couple small comments but looks great

go/vt/vtadmin/testutil/proto_compare.go Show resolved Hide resolved
go/vt/vtadmin/testutil/cluster.go Outdated Show resolved Hide resolved
go/vt/vtadmin/cluster/cluster.go Outdated Show resolved Hide resolved
@doeg
Update commentary
0606f3c 
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@doeg
Copy link
Contributor Author

doeg commented Feb 25, 2021

@ajm188 this is ready for another pass, when you get a sec.

@rohit-nayak-ps rohit-nayak-ps merged commit 89766bd into vitessio:master Feb 26, 2021
@askdba askdba added the Component: VTAdmin VTadmin interface label Feb 26, 2021
@askdba askdba added this to the v10.0 milestone Feb 26, 2021
@doeg doeg deleted the sarabee-vtadmin-api-tidying branch March 1, 2021 00:50
@doeg doeg added this to In progress in VTAdmin via automation Mar 16, 2021
@doeg doeg moved this from In progress to Done in VTAdmin Mar 16, 2021
@ajm188 ajm188 mentioned this pull request Jul 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajm188 ajm188 ajm188 approved these changes

@rohit-nayak-ps rohit-nayak-ps Awaiting requested review from rohit-nayak-ps

@deepthi deepthi Awaiting requested review from deepthi

Assignees
No one assigned
Labels
Component: VTAdmin VTadmin interface
Projects
VTAdmin
Done
Milestone
v10.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@doeg @ajm188 @askdba @rohit-nayak-ps