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

Make Durabler interface methods public #15548

Merged
merged 11 commits into from
Mar 29, 2024

Conversation

timvaillancourt
Copy link
Contributor

@timvaillancourt timvaillancourt commented Mar 22, 2024

Description

This PR implements an open RFC for moving the Durabler interface from the reparentutil package to public methods so it is easier to integrate Durability Policies from external packages

Related Issue(s)

RFC: #15544

Checklist

  • "Backport to:" labels have been added if this change should be back-ported to release branches
  • If this change is to be back-ported to previous releases, a justification is included in the PR description
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on CI?
  • Documentation was added or is not required

Deployment Notes

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Copy link
Contributor

vitess-bot bot commented Mar 22, 2024

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 NeedsBackportReason If backport labels have been applied to a PR, a justification is required 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 Mar 22, 2024
@github-actions github-actions bot added this to the v20.0.0 milestone Mar 22, 2024
@timvaillancourt timvaillancourt marked this pull request as ready for review March 22, 2024 13:09
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 65.78%. Comparing base (2078d62) to head (941846f).
Report is 1 commits behind head on main.

Files Patch % Lines
go/vt/vtctl/reparentutil/durability.go 86.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15548      +/-   ##
==========================================
- Coverage   65.78%   65.78%   -0.01%     
==========================================
  Files        1561     1561              
  Lines      194838   194838              
==========================================
- Hits       128183   128174       -9     
- Misses      66655    66664       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@deepthi deepthi removed NeedsIssue A linked issue is missing for this Pull Request NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Mar 27, 2024
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.

LGTM. We also need approval from @GuptaManan100 as the primary author/maintainer of these files.

@deepthi deepthi added Component: VTorc Vitess Orchestrator integration Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Mar 27, 2024
@deepthi
Copy link
Member

deepthi commented Mar 27, 2024

@GuptaManan100 should we add this to the release notes for v20?

@timvaillancourt timvaillancourt linked an issue Mar 27, 2024 that may be closed by this pull request
Copy link
Member

@GuptaManan100 GuptaManan100 left a comment

Choose a reason for hiding this comment

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

Yes! The changes look good to me, but we do need to add a section describing this change to the interface in changelog/20.0/20.0.0/summary.md

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
@timvaillancourt
Copy link
Contributor Author

Yes! The changes look good to me, but we do need to add a section describing this change to the interface in changelog/20.0/20.0.0/summary.md

@GuptaManan100 / @deepthi sounds good, changelog added 👍. Edits are appreciated

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
timvaillancourt and others added 4 commits March 29, 2024 00:10
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
…hods

Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100 GuptaManan100 merged commit aa52fbd into vitessio:main Mar 29, 2024
102 checks passed
@timvaillancourt timvaillancourt deleted the durabler-public-methods branch April 8, 2024 13:42
timvaillancourt added a commit to slackhq/vitess that referenced this pull request Apr 8, 2024
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Co-authored-by: Manan Gupta <manan@planetscale.com>
timvaillancourt added a commit to slackhq/vitess that referenced this pull request May 22, 2024
…cy (#266)

* `slack-vitess-r15.0.5`: add `slack_cross_cell` custom durability policy

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* go mod tidy

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* go mod tidy

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* go mod tidy again

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Use tag

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* use v0.15.1

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* update shim

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Make `Durabler` interface methods public (vitessio#15548)

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Co-authored-by: Manan Gupta <manan@planetscale.com>

* Update `slack_cross_cell` shim

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* go mod tidy

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* go mod tidy again

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Allow private go module from vitess-additions repo

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Fix typo

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Missing non-template update

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Missing non-template update

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Missing non-template update, pt 3

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Fix docker tests

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* make proto

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* fix upgrade/downgrade tests

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Update all the Go dependencies (vitessio#11741)

* Update all the Go dependencies

This seems to work for the basics just fine, so let's have CI take a run
at this as well to update these.

Only one small update to the Azure blob storage handling seems needed so
far.

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

* Use correct proto comparisons

We're using `reflect.DeepEqual` or `testify` helpers that essentially
use that in a number of places are comparing protobufs. This is not
supported though, protobufs are not comparable with `reflect.DeepEqual`.

This is exposed because of the tiny patch bump of protobuf which changes
some internal optimization of how it initializes protobufs that breaks
all this.

Instead, move to the appropriate helpers here.

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

* Resolve signature mismatch

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* revert protobuf version

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* revert protobuf version, pt 2

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Revert "Update all the Go dependencies (vitessio#11741)"

This reverts commit 18faa1e.

* go mod tidy

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

---------

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Co-authored-by: Manan Gupta <manan@planetscale.com>
Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com>
timvaillancourt added a commit to slackhq/vitess that referenced this pull request May 30, 2024
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Co-authored-by: Manan Gupta <manan@planetscale.com>
timvaillancourt added a commit to slackhq/vitess that referenced this pull request Jul 9, 2024
* Make `Durabler` interface methods public (vitessio#15548)

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Co-authored-by: Manan Gupta <manan@planetscale.com>

* Load `--grpc_auth_static_client_creds` file once (vitessio#15030)

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Limit concurrent creation of healthcheck gRPC connections (vitessio#15053)

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Filter by keyspace earlier in `tabletgateway`s `WaitForTablets(...)` (vitessio#15347)

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Use slack-15.0 as previous release

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* empty commit

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* force ci to run

* Update GH Action runners

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* test templates

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* set GH access token in build

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Fix reparent old tests

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Remove CIs we don't need

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Remove CIs we don't need again

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Add private repo setup to upgrade_downgrade_test_backups_e2e.yml

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Add private repo setup to more CI

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* remove CI skip logic for upstream stuff

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* CODEOWNERS

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* [release-19.0] Add timeout to all the contexts used for RPC calls in vtorc (vitessio#15991) (vitessio#16103)

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

* `slack-vitess-r15.0.5`: forward-port consul topo limits PR #111 (#297)

* `slack-vitess-r14.0.5`: allow conn overrides in consul topo (#111)

* `slack-vitess-r14.0.5`: allow conn overrides in consul topo

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* fix e2e test

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

---------

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Update flags tests that didn't exist in v14

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

---------

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* update vtcombo e2e

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Fix err with installing percona-xtrabackup-24

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* `slack-vitess-r15.0.5`: fix races in `Unit Test (Race)` CI, fix "old" reparent CIs (#356)

* update vtcombo e2e test

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Fix bad merge conflict fix

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* go mod tidy

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* update vtcombo e2e test again

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* [release-19.0] Upgrade the Golang version to `go1.22.5` (vitessio#16322)

Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Co-authored-by: frouioui <frouioui@users.noreply.github.com>
Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr>

* merge conflict fixes

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* make vtadmin_web_proto_types

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

---------

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Co-authored-by: Manan Gupta <manan@planetscale.com>
Co-authored-by: Manan Gupta <35839558+GuptaManan100@users.noreply.github.com>
Co-authored-by: vitess-bot <139342327+vitess-bot@users.noreply.github.com>
Co-authored-by: frouioui <frouioui@users.noreply.github.com>
Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: VTorc Vitess Orchestrator integration Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: public methods for Durabler interface in reparentutil
3 participants