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

Adding deprecate message to backup hooks #11491

Merged
merged 7 commits into from
Oct 14, 2022
Merged

Adding deprecate message to backup hooks #11491

merged 7 commits into from
Oct 14, 2022

Conversation

rsajwani
Copy link
Contributor

@rsajwani rsajwani commented Oct 13, 2022 •
edited
Loading

Signed-off-by: Rameez Sajwani [email protected]

Description

This PR generates deprecation message for backup hooks. They are plan to be removed in v16.0

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

@vitess-bot
Copy link
Contributor

vitess-bot bot commented Oct 13, 2022 •
edited by deepthi
Loading

Review Checklist

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

General

  • Ensure that the Pull Request has a descriptive title.
  • If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.

If a new flag is being introduced:

  • Is it really necessary to add this flag?
  • Flag names should be clear and intuitive (as far as possible)
  • Help text should be descriptive.
  • Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

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

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should include a link to an issue that describes the bug.

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.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from VTop, if used there.

ajm188
ajm188 reviewed Oct 13, 2022
View reviewed changes
go/vt/mysqlctl/backup.go Outdated Show resolved Hide resolved
@rsajwani
adding markdeprecated
bccbbd2 
Signed-off-by: Rameez Sajwani <[email protected]>
@rsajwani rsajwani self-assigned this Oct 13, 2022
@rsajwani rsajwani marked this pull request as ready for review October 14, 2022 00:04
@frouioui frouioui mentioned this pull request Oct 14, 2022
Closed
100 tasks
GuptaManan100
GuptaManan100 reviewed Oct 14, 2022
View reviewed changes
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.

Looks good to me!

@GuptaManan100
Copy link
Member

GuptaManan100 commented Oct 14, 2022 •
edited
Loading

@rsajwani I am taking the liberty to merge the release-15.0 branch since the VTOrc test names have changed and they are required now.

ajm188
ajm188 reviewed Oct 14, 2022
View reviewed changes
go/vt/mysqlctl/backup.go Outdated
@@ -103,7 +103,8 @@ func init() {
}

func registerBackupFlags(fs *pflag.FlagSet) {
fs.StringVar(&backupStorageHook, "backup_storage_hook", backupStorageHook, "if set, we send the contents of the backup files through this hook (deprecated: hooks will be disabled in v16.0. Please use ExternalCompressorCmd/ExternalDecompressorCmd instead).")
fs.StringVar(&backupStorageHook, "backup_storage_hook", backupStorageHook, "if set, we send the contents of the backup files through this hook.")
_ = fs.MarkDeprecated("backup_storage_hook", "--backup_storage_hook is deprecated; Consider using ExternalCompressorCmd/ExternalDecompressorCmd instead.")
Copy link
Contributor

Choose a reason for hiding this comment

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

For this, pflag is going to do roughly {{f.Name}} is deprecated.{{ if .Message ne "" }} {{ .Message }}{{ endif }}, so you should try this out and adjust accordingly.

Copy link
Contributor Author

@rsajwani rsajwani Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

fixed it...

Flag --backup_storage_hook has been deprecated, consider using ExternalCompressorCmd/ExternalDecompressorCmd instead.
W1014 10:26:31.712856 67759 backup.go:211] checkNoDB failed, found active db vt_ks
W1014 10:26:32.067150 67759 builtinbackupengine.go:165] --backup_storage_hook is deprecated; Consider using ExternalCompressorCmd/ExternalDecompressorCmd instead.

go/vt/mysqlctl/builtinbackupengine.go Outdated
Comment on lines 164 to 166
if backupStorageHook != "" {
log.Warning("--backup_storage_hook is deprecated; Consider using ExternalCompressorCmd/ExternalDecompressorCmd instead.")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is redundant

Copy link
Member

Choose a reason for hiding this comment

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

We debated this a bit. The flag deprecation warning shows up only at tablet startup, whereas this will show up on every backup/restore, so it does have some additional value.

go/vt/mysqlctl/builtinbackupengine.go Outdated
@@ -591,6 +591,9 @@ func (be *BuiltinBackupEngine) ExecuteRestore(ctx context.Context, params Restor
if err := prepareToRestore(ctx, params.Cnf, params.Mysqld, params.Logger); err != nil {
return nil, err
}
if bm.TransformHook != "" {
log.Warning("--backup_storage_hook is deprecated; Consider using ExternalCompressorCmd/ExternalDecompressorCmd instead.")
Copy link
Contributor

Choose a reason for hiding this comment

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

this is redundant

@ajm188
Copy link
Contributor

ajm188 commented Oct 14, 2022

Looks good to me!

@GuptaManan100 @rsajwani please don't merge this until the deprecation wording is fixed

deepthi
deepthi reviewed Oct 14, 2022
View reviewed changes
go/vt/mysqlctl/backup.go Outdated Show resolved Hide resolved
doc/releasenotes/15_0_0_summary.md Outdated Show resolved Hide resolved
@rsajwani
fixing messaging
cd5ca14 
Signed-off-by: Rameez Sajwani <[email protected]>
@rsajwani
fix flag name
fc0451c 
Signed-off-by: Rameez Sajwani <[email protected]>
@deepthi deepthi merged commit b2a654c into vitessio:release-15.0 Oct 14, 2022
@deepthi deepthi deleted the deprecate-backup-hooks branch October 14, 2022 23:04
@vitess-bot
Copy link
Contributor

vitess-bot bot commented Oct 14, 2022

I was unable to forwardport this Pull Request to the following branches: main.

rsajwani added a commit to planetscale/vitess that referenced this pull request Oct 26, 2022
@rsajwani @GuptaManan100
Adding deprecate message to backup hooks (vitessio#11491)
d34a15e 
* Adding deprecate message to backup hooks

Signed-off-by: Rameez Sajwani <[email protected]>

* adding markdeprecated

Signed-off-by: Rameez Sajwani <[email protected]>

* fixing deprecation message

Signed-off-by: Rameez Sajwani <[email protected]>

* fixing messaging

Signed-off-by: Rameez Sajwani <[email protected]>

* fix flag name

Signed-off-by: Rameez Sajwani <[email protected]>

Signed-off-by: Rameez Sajwani <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Co-authored-by: Manan Gupta <[email protected]>
@rsajwani rsajwani mentioned this pull request Oct 26, 2022
Merged
3 tasks
frouioui pushed a commit that referenced this pull request Oct 28, 2022
@rsajwani @GuptaManan100
[cherry-pick]Adding deprecate message to backup hooks (#11491) (#11587)
8aebb6f 
* Adding deprecate message to backup hooks (#11491)

* Adding deprecate message to backup hooks

Signed-off-by: Rameez Sajwani <[email protected]>

* adding markdeprecated

Signed-off-by: Rameez Sajwani <[email protected]>

* fixing deprecation message

Signed-off-by: Rameez Sajwani <[email protected]>

* fixing messaging

Signed-off-by: Rameez Sajwani <[email protected]>

* fix flag name

Signed-off-by: Rameez Sajwani <[email protected]>

Signed-off-by: Rameez Sajwani <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Co-authored-by: Manan Gupta <[email protected]>

* fix release notes and summary

Signed-off-by: Rameez Sajwani <[email protected]>

* fixing some typos in release notes and summary

Signed-off-by: Rameez Sajwani <[email protected]>

* removing newline

Signed-off-by: Rameez Sajwani <[email protected]>

Signed-off-by: Rameez Sajwani <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Co-authored-by: Manan Gupta <[email protected]>
@deepthi deepthi mentioned this pull request Jan 10, 2023
Merged
3 tasks
@austenLacy austenLacy mentioned this pull request Apr 26, 2023
Merged
@austenLacy austenLacy mentioned this pull request Aug 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuptaManan100 GuptaManan100 GuptaManan100 approved these changes

@ajm188 ajm188 ajm188 approved these changes

@deepthi deepthi deepthi approved these changes

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

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

Assignees

@rsajwani rsajwani

Labels
Component: Cluster management Forwardport to: main This will forward port the PR to the main branch Type: Internal Cleanup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rsajwani @GuptaManan100 @ajm188 @deepthi