Skip to content

Use introduced tablet manager RPCs in VTOrc#10467

Merged
GuptaManan100 merged 16 commits intovitessio:mainfrom
planetscale:vtorc-use-rpcs
Jun 22, 2022
Merged

Use introduced tablet manager RPCs in VTOrc#10467
GuptaManan100 merged 16 commits intovitessio:mainfrom
planetscale:vtorc-use-rpcs

Conversation

@GuptaManan100
Copy link
Copy Markdown
Contributor

@GuptaManan100 GuptaManan100 commented Jun 8, 2022 •
edited
Loading

Description

This PR uses the RPCs introduced in #10464 in VTOrc.
The RPC ResetReplicationParameters is used in fixPrimaryHasPrimary to reset the replication parameters for the primary tablet.

The RPC FullStatus is used in ReadTopologyInstanceBufferable to read information from MySQL server without connecting directly to it.

As part of this refactor, 3 fields from Instance struct have been removed -

  • Uptime :- Unused in the codebase. Used to store the uptime of each mysql instance
  • SemiSyncAvailable :- Set when both the semi_sync plugins were loaded. Unused in the codebase and is not required.
  • ReplicationCredentialsAvailable :- Unused in the codebase. Used to store if the instance had access to the replication credentials of its replicas

Related Issue(s)

Checklist

  • "Backport me!" label has been added if this change should be backported
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

@GuptaManan100 GuptaManan100 force-pushed the vtorc-use-rpcs branch 5 times, most recently from 18de6e3 to 76aa4af Compare June 15, 2022 17:25
@GuptaManan100 GuptaManan100 mentioned this pull request Jun 15, 2022
Merged
3 tasks
@GuptaManan100 GuptaManan100 changed the title Use RPCs in VTOrc Use introduced tablet manager RPCs in VTOrc Jun 15, 2022
@GuptaManan100 GuptaManan100 marked this pull request as ready for review June 15, 2022 18:12
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 15, 2022

Review Checklist

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

General

  • Ensure that the Pull Request has the correct release notes label. release notes none should only be used for PRs that are so trivial that they need not be included.
  • If a new flag is being introduced, review whether it is really needed. The flag names should be clear and intuitive (as far as possible), and the flag's help should be descriptive.
  • If a workflow is added or modified, each items in Jobs should be named in order to mark it as required. If the workflow should be required, the GitHub Admin should be notified.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should either include a link to an issue that describes the bug OR an actual description of the bug and how to reproduce, along with a description of the fix.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.

@GuptaManan100
Copy link
Copy Markdown
Contributor Author

GuptaManan100 commented Jun 15, 2022

This PR is based on top of #10464 and should be merged after it

deepthi
deepthi reviewed Jun 17, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@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.

This looks fine. I only looked at the orchestrator directory since this PR seems to be on top of #10464.
I can re-review once that is complete and we have a smaller diff in this PR.

go/vt/orchestrator/inst/instance_dao.go Outdated
Copy link
Copy Markdown
Collaborator

@deepthi deepthi Jun 16, 2022

Choose a reason for hiding this comment

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

nit: I don't know which is right, but let's use either UUID or Uuid everywhere and not a mix.

Copy link
Copy Markdown
Contributor Author

@GuptaManan100 GuptaManan100 Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally, it should be UUID. the Uuid is coming from the proto file which is generated. Unfortunately, there is no way to change that because we keep everything in the proto file lower cased.

Copy link
Copy Markdown
Contributor

@ajm188 ajm188 Jun 17, 2022

Choose a reason for hiding this comment

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

+1 to this (preferring UUID except in the case of protobuf-generated code -- vtadmin does similar things)

if you want (read: you should not do this), you can name the field u_u_i_d and protobuf will treat those as individual words and you'll get UUID in the generated code 🙃 🙃 🙃

Copy link
Copy Markdown
Contributor Author

@GuptaManan100 GuptaManan100 Jun 20, 2022

Choose a reason for hiding this comment

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

I was considering u_u_i_d too, but then I elected against it 🤣

@GuptaManan100
feat: use the new RPC for resetting replication
008066e 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: add code to get the full status and use it
d42fbda 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: remove uptime from instance propoerties in VTOrc
78aa63a 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
test: add end to end test for checking the output of ReadInstanceBuff…
8f07b08 
…erable

Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: remove a mysql query for server id, etc and use full status inf…
3b3bfcb 
…ormation instead

Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: use primary status output instead of mysql query
553fa06 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: remove semi-sync available, since it is unused
d956920 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: use semi sync enable from full status instead of mysql query
dcb8c10 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: use semi sync statuses from full status instead of mysql query
68f67ff 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
test: check output of more fields in the test
69f92e5 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: remove ReplicationCredentialsAvailable since it is unused
639e46f 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: use executed, purged gtid set, server uuid, gtid mode and binlo…
1c4feb5 
…g row image from full status instead of mysql query

Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: add testing for replication status fields
2a84e2f 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: check for nil values to prevent panics when some fields are empty
8d7ccb7 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: use replication status from full status instead of mysql query
f6c5dea 
Signed-off-by: Manan Gupta <manan@planetscale.com>
deepthi
deepthi approved these changes Jun 21, 2022
View reviewed changes
@GuptaManan100
test: add license and shorten package name
a4902ee 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100 GuptaManan100 merged commit 5c00d9e into vitessio:main Jun 22, 2022
@GuptaManan100 GuptaManan100 deleted the vtorc-use-rpcs branch June 22, 2022 01:04
rsajwani pushed a commit to planetscale/vitess that referenced this pull request Aug 1, 2022
@GuptaManan100
CherryPick: Use introduced tablet manager RPCs in VTOrc (vitessio#10467…
1d3e190 
…) (vitessio#885)

* Use introduced tablet manager RPCs in VTOrc (vitessio#10467)

* feat: use the new RPC for resetting replication

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add code to get the full status and use it

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: remove uptime from instance propoerties in VTOrc

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: add end to end test for checking the output of ReadInstanceBufferable

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: remove a mysql query for server id, etc and use full status information instead

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: use primary status output instead of mysql query

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: remove semi-sync available, since it is unused

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: use semi sync enable from full status instead of mysql query

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: use semi sync statuses from full status instead of mysql query

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: check output of more fields in the test

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: remove ReplicationCredentialsAvailable since it is unused

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: use executed, purged gtid set, server uuid, gtid mode and binlog row image from full status instead of mysql query

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add testing for replication status fields

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: check for nil values to prevent panics when some fields are empty

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: use replication status from full status instead of mysql query

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: add license and shorten package name

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: fix test flakiness

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat:fix test to work with mysql 8.0 too

Signed-off-by: Manan Gupta <manan@planetscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ajm188 ajm188 Awaiting requested review from ajm188

@doeg doeg Awaiting requested review from doeg

@notfelineit notfelineit Awaiting requested review from notfelineit

@shlomi-noach shlomi-noach Awaiting requested review from shlomi-noach shlomi-noach is a code owner

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

@harshit-gangal harshit-gangal Awaiting requested review from harshit-gangal harshit-gangal is a code owner

@systay systay Awaiting requested review from systay

1 more reviewer

@deepthi deepthi deepthi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Component: Cluster management Component: VTOrc Vitess Orchestrator integration Type: Internal Cleanup

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@GuptaManan100 @deepthi @ajm188