Repository navigation
Remove glog logging support
#21309
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
base: main
Are you sure you want to change the base?
Remove glog logging support
#21309
Changes from 1 commit
4d71261
78710cf
b25231a
5da21fd
fb90ce6
bf9fbd7
7072cf7
0dede3b
5f6147c
145e22f
33611a6
30a6e01
406a870
b5bb28a
445f67a
028c751
768da08
b26b716
5ecb9b3
99f06b3
b1cc2c3
4e9fe40
e13ac21
4223046
a8e31bb
e29da24
8e498c3
b8ecc20
4609be0
59d3fd2
522a8a6
40abcee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,12 +29,11 @@ import ( | |
| ) | ||
|
|
||
| var Start = &cobra.Command{ | ||
| Use: "start", | ||
| Short: "Starts mysqld on an already 'init'-ed directory.", | ||
| Long: "Resume an existing `mysqld` instance that was previously bootstrapped with `init` or `init_config`", | ||
| Example: `mysqlctl --tablet-uid 101 --alsologtostderr start`, | ||
| Args: cobra.NoArgs, | ||
| RunE: commandStart, | ||
| Use: "start", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: this removes the whole
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Restored for start, shutdown, and teardown without the flag (7072cf7). |
||
| Short: "Starts mysqld on an already 'init'-ed directory.", | ||
| Long: "Resume an existing `mysqld` instance that was previously bootstrapped with `init` or `init_config`", | ||
| Args: cobra.NoArgs, | ||
| RunE: commandStart, | ||
| } | ||
|
|
||
| var startArgs = struct { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several of these flags never had a deprecation warning, so v25 would reject them without any prior notice:
--log-structured. main's deprecation message for the glog flags actually tells users to pass it:use the default structured logging instead ("--log-structured").--log-rotate-max-size--keep-logs,--keep-logs-by-mtime,--purge-logs-interval--log_link,--logbuflevelvtctldclient,vtctlclient,vtbenchandvtclient. They come in throughAddGoFlagSetwithoutMarkDeprecated, so they are listed in the v24 help output. The v24 operator README also recommendedalias vtctldclient="vtctldclient ... --alsologtostderr".Two external tools also still pass removed flags, and that is what the other two red jobs are:
--logtostderrto vttablet, mysqlctld, vtgate, vtctld and vtorc, and--logtostderr --alsologtostderrto vtadmin (e.g.pkg/operator/vttablet/flags.go,pkg/operator/vtadmin/deployment.go). Every Vitess pod ends up in CrashLoopBackOff. So the operator impact is wider thanoperator.yaml: every operator-managed v25 component fails to start until the operator stops passing these flags.github.com/vitessio/vtpasses--log_dirto vtctld, which exits withunknown flag: --log_dir.Could we keep the removed logging flags in v25 as hidden no-ops that print "has no effect and will be removed in v26", reject
--log-structured=falsewith a clear error since glog is gone, and drop them in v26? The alternative is releasing operator and vt updates first and bumping the pins here.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to update the operator and vt, LMK if you feel strongly about adding the no-ops. The release notes call out the flags that never had a warning (78710cf). For the red jobs I opened planetscale/vitess-operator#846, which drops the flags on main so
:latestpicks it up, and vitessio/vt#100, which moves vt to Vitess main. The tester workflow builds from the vt#100 head until it merges (b26b716).