Skip to content

Difference between latest 15.0 main files with 14.0-release #2

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

Draft
wants to merge 7 commits into
base: flags-14.0
Choose a base branch
from

Conversation

rohit-nayak-ps
Copy link
Owner

@rohit-nayak-ps rohit-nayak-ps commented Oct 2, 2022

We did a major refactor of flags in 15.0. Flags are now registered for each binary, rather than being global. As a result it is possible that we might removed flags from binaries that should be there. Also, we may have enabled unnecessary flags for binaries.

This PR contains a diff of the current help output (used in TestHelpOutput) with the one generated by 14.0 to be used to audit the flag changes. This is linked from the Vitess PR: vitessio/vitess#11434.

…min help was not generated for 14.0 as part of this PR :(

Signed-off-by: Rohit Nayak <[email protected]>
Copy link

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

we need one minor text change to the vtadmin help output (which i am making now) otherwise lgtm

@@ -1,48 +1,48 @@
Usage of vtgate:
--allowed_tablet_types value Specifies the tablet types this vtgate is allowed to route queries to
--allowed_tablet_types []topodatapb.TabletType Specifies the tablet types this vtgate is allowed to route queries to.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way we can make this string? and it's supposed to be comma-separated, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ajm188 ☝️

Copy link

@ajm188 ajm188 Oct 14, 2022

Choose a reason for hiding this comment

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

Not sure what your question is exactly, tbh. This is a list of tablet types, which can be both comma separated ("replica,primary") and by specifying the flag multiple times (edit: nope i read the code and it's just CSV)

Copy link

Choose a reason for hiding this comment

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

if the question is about the type (value => []topodatapb.TabletType) that's just coming from here, but you can see in the actual Set that the behavior is as i've described

Copy link
Collaborator

Choose a reason for hiding this comment

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

to clarify my question: when the users are seeing the help text, they should not need to know what topodatapb.TabletType is. They just need to know to provide a CSV. so it will be good if we can somehow make this print

--allowed_tablet_types string      Specifies the tablet types this vtgate is allowed to route queries to. Should be provided as a comma-separated set of tablet types

or something like that.

Copy link

Choose a reason for hiding this comment

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

yeah, so this comes from the Type() method on a pflag.Value so you can make it say whatever you want (you shouldn't do that though - e.g. func (f *myFlag) Type() string { "crashes-your-program-and-steals-your-data" } :P

Choose a reason for hiding this comment

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

done

Comment on lines 21 to 23
--backup_storage_number_blocks int if backup_storage_compress is true, backup_storage_number_blocks sets the number of blocks that can be processed, at once, before the writer blocks, during compression (default is 2). It should be equal to the number of CPUs available for compression. (default 2)
--builtinbackup_mysqld_timeout duration how long to wait for mysqld to shutdown at the start of the backup. (default 10m0s)
--builtinbackup_progress duration how often to send progress updates when backing up large files. (default 5s)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe all backup flags can be removed from mysqlctl.

Choose a reason for hiding this comment

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

fixed

Comment on lines 25 to 26
--compression-engine-name string compressor engine used for compression. (default "pargzip")
--compression-level int what level to pass to the compressor. (default 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Backup flags can be removed.

Choose a reason for hiding this comment

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

fixed

@@ -54,97 +51,73 @@ Usage of mysqlctl:
--db_ssl_ca_path string connection ssl ca path
--db_ssl_cert string connection ssl certificate
--db_ssl_key string connection ssl key
--db_ssl_mode value SSL mode to connect with. One of disabled, preferred, required, verify_ca & verify_identity.
--db_ssl_mode SslMode SSL mode to connect with. One of disabled, preferred, required, verify_ca & verify_identity.
--db_tls_min_version string Configures the minimal TLS version negotiated when SSL is enabled. Defaults to TLSv1.2. Options: TLSv1.0, TLSv1.1, TLSv1.2, TLSv1.3.
--dba_idle_timeout duration Idle timeout for dba connections (default 1m0s)
--dba_pool_size int Size of the connection pool for dba connections (default 20)
--disable_active_reparents if set, do not allow active reparents. Use this to protect a cluster using external reparents.
Copy link
Collaborator

@deepthi deepthi Oct 14, 2022

Choose a reason for hiding this comment

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

disable_active_reparents can be removed from here

Choose a reason for hiding this comment

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

fixed

Comment on lines 59 to 61
--external-compressor string command with arguments to use when compressing a backup.
--external-compressor-extension string extension to use when using an external compressor.
--external-decompressor string command with arguments to use when decompressing a backup.
Copy link
Collaborator

Choose a reason for hiding this comment

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

backup flags.

Choose a reason for hiding this comment

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

fixed

Comment on lines 116 to 122
--xbstream_restore_flags string flags to pass to xbstream command during restore. These should be space separated and will be added to the end of the command. These need to match the ones used for backup e.g. --compress / --decompress, --encrypt / --decrypt
--xtrabackup_backup_flags string flags to pass to backup command. These should be space separated and will be added to the end of the command
--xtrabackup_prepare_flags string flags to pass to prepare command. These should be space separated and will be added to the end of the command
--xtrabackup_root_path string directory location of the xtrabackup and xbstream executables, e.g., /usr/bin
--xtrabackup_stream_mode string which mode to use if streaming, valid values are tar and xbstream (default tar)
--xtrabackup_stream_mode string which mode to use if streaming, valid values are tar and xbstream (default "tar")
--xtrabackup_stripe_block_size uint Size in bytes of each block that gets sent to a given stripe before rotating to the next stripe (default 102400)
--xtrabackup_stripes uint If greater than 0, use data striping across this many destination files to parallelize data transfer and decompression
Copy link
Collaborator

Choose a reason for hiding this comment

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

backup flags

Choose a reason for hiding this comment

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

fixed

@@ -2,48 +2,33 @@ Usage of mysqlctld:
--alsologtostderr log to standard error as well as files
--app_idle_timeout duration Idle timeout for app connections (default 1m0s)
--app_pool_size int Size of the connection pool for app connections (default 40)
--backup_engine_implementation string Specifies which implementation to use for creating new backups (builtin or xtrabackup). Restores will always be done with whichever engine created a given backup. (default builtin)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be pretty much identical to mysqlctl.txt

Choose a reason for hiding this comment

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

fixed

@@ -1,84 +1,53 @@
Usage of vtctld:
--action_timeout duration time to wait for an action before resorting to force (default 2m0s)
--allowed_tablet_types value Specifies the tablet types this vtgate is allowed to route queries to
--allowed_tablet_types []topodatapb.TabletType Specifies the tablet types this vtgate is allowed to route queries to.
Copy link
Collaborator

Choose a reason for hiding this comment

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

vtgate flag, remove from vtctld.

Choose a reason for hiding this comment

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

fixed

Comment on lines 28 to 29
--dba_idle_timeout duration Idle timeout for dba connections (default 1m0s)
--dba_pool_size int Size of the connection pool for dba connections (default 20)
Copy link
Collaborator

@deepthi deepthi Oct 14, 2022

Choose a reason for hiding this comment

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

I'm unsure if these two flags are actually relevant for vtctld. I don't think it actually creates a conn pool which means we should be able to remove these.

Choose a reason for hiding this comment

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

fixed

--enable_hot_row_protection_dry_run If true, hot row protection is not enforced but logs if transactions would have been queued.
--enable_lag_throttler If true, vttablet will run a throttler service, and will implicitly enable heartbeats
--enable_queries [DEPRECATED - query commands via vtctl are being deprecated] if set, allows vtgate and vttablet queries. May have security implications, as the queries will be run from this process.
--enable_query_plan_field_caching This option fetches & caches fields (columns) when storing query plans (default true)
--enable_realtime_stats Required for the Realtime Stats view. If set, vtctld will maintain a streaming RPC to each tablet (in all cells) to gather the realtime health stats.
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should probably deprecate this, because it is used by the old web UI that is already deprecated.

Choose a reason for hiding this comment

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

fixed

--enable_replication_reporter Use polling to track replication lag.
--enable_transaction_limit If true, limit on number of transactions open at the same time will be enforced for all users. User trying to open a new transaction after exhausting their limit will receive an error immediately, regardless of whether there are available slots or not.
--enable_transaction_limit_dry_run If true, limit on number of transactions open at the same time will be tracked for all users, but not enforced.
--enable_tx_throttler If true replication-lag-based throttling on transactions will be enabled.
--enable_vtctld_ui If true, the vtctld web interface will be enabled. Default is true. (default true)
Copy link
Collaborator

@deepthi deepthi Oct 14, 2022

Choose a reason for hiding this comment

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

Deprecate enable_vtctld_ui but make sure to keep using it to control behavior.

Choose a reason for hiding this comment

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

fixed

--jaeger-agent-host string host and port to send spans to. if empty, no tracing will be done
--keep_logs duration keep logs for this long (using ctime) (zero to keep forever)
--keep_logs_by_mtime duration keep logs for this long (using mtime) (zero to keep forever)
--keyspaces_to_watch value Specifies which keyspaces this vtgate should have access to while routing queries or accessing the vschema
--keyspaces_to_watch strings Specifies which keyspaces this vtgate should have access to while routing queries or accessing the vschema.
Copy link
Collaborator

@deepthi deepthi Oct 14, 2022

Choose a reason for hiding this comment

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

keyspaces_to_watch is only relevant to vtgate. delete.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm I wonder if vtctld -> realtime_stats -> healthcheck -> keyspaces_to_watch
@frouioui do we need to keep this?

Choose a reason for hiding this comment

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

we still use keyspaces_to_watch in the discovery.

Choose a reason for hiding this comment

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

we actively use it to filter out keyspaces

Choose a reason for hiding this comment

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

we create a healthcheck in vtcltd and i am assuming people should be able to filter out which keyspace they want to see in vtctld and vtgate

--mysql_auth_server_static_string string JSON representation of the users/passwords config.
--mysql_auth_static_reload_interval duration Ticker to reload credentials
--mysql_clientcert_auth_method string client-side authentication method to use. Supported values: mysql_clear_password, dialog. (default mysql_clear_password)
--mysql_server_flush_delay duration Delay after which buffered response will be flushed to the client. (default 100ms)
--mysql_server_version string MySQL server version to advertise.
Copy link
Collaborator

@deepthi deepthi Oct 14, 2022

Choose a reason for hiding this comment

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

mysql_server_version is only relevant to vtgate. delete.

Choose a reason for hiding this comment

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

fixed

Comment on lines 78 to 79
--mysqlctl_mycnf_template string template file to use for generating the my.cnf file during server init
--mysqlctl_socket string socket file to use for remote mysqlctl actions (empty for local actions)
Copy link
Collaborator

Choose a reason for hiding this comment

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

these two can be deleted too.

Choose a reason for hiding this comment

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

fixed

Comment on lines -205 to -207
--srv_topo_cache_refresh duration how frequently to refresh the topology for cached entries (default 1s)
--srv_topo_cache_ttl duration how long to use cached entries for topology (default 1s)
--srv_topo_timeout duration topo server timeout (default 5s)
Copy link
Collaborator

Choose a reason for hiding this comment

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

need to revisit whether it is safe to delete these. though I guess if tests are passing it is fine.

Comment on lines 170 to 172
--vtgate_grpc_crl string the server crl to use to validate server certificates when connecting
--vtgate_grpc_key string the key to use to connect
--vtgate_grpc_server_name string the server name to use to validate server certificate
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we already deleted the VtGateExecute command. If yes, then we also don't need these vtgate_ flags.

Choose a reason for hiding this comment

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

fixed

Comment on lines 173 to 174
--web_dir string NOT USED, here for backward compatibility
--web_dir2 string NOT USED, here for backward compatibility
Copy link
Collaborator

Choose a reason for hiding this comment

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

deprecate these.

Choose a reason for hiding this comment

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

fixed

Comment on lines 178 to 185
--xbstream_restore_flags string flags to pass to xbstream command during restore. These should be space separated and will be added to the end of the command. These need to match the ones used for backup e.g. --compress / --decompress, --encrypt / --decrypt
--xtrabackup_backup_flags string flags to pass to backup command. These should be space separated and will be added to the end of the command
--xtrabackup_prepare_flags string flags to pass to prepare command. These should be space separated and will be added to the end of the command
--xtrabackup_root_path string directory location of the xtrabackup and xbstream executables, e.g., /usr/bin
--xtrabackup_stream_mode string which mode to use if streaming, valid values are tar and xbstream (default tar)
--xtrabackup_stream_mode string which mode to use if streaming, valid values are tar and xbstream (default "tar")
--xtrabackup_stripe_block_size uint Size in bytes of each block that gets sent to a given stripe before rotating to the next stripe (default 102400)
--xtrabackup_stripes uint If greater than 0, use data striping across this many destination files to parallelize data transfer and decompression
--xtrabackup_user string User that xtrabackup will use to connect to the database server. This user must have all necessary privileges. For details, please refer to xtrabackup documentation.
Copy link
Collaborator

@deepthi deepthi Oct 14, 2022

Choose a reason for hiding this comment

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

I don't think vtctld needs any xtrabackup or xbstream flags. it only needs backup storage related flags.

Choose a reason for hiding this comment

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

fixed

Comment on lines 175 to 177
--workflow_manager_disable strings comma separated list of workflow types to disable
--workflow_manager_init Initialize the workflow manager in this vtctld instance.
--workflow_manager_use_election if specified, will use a topology server-based master election to ensure only one workflow manager is active at a time.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should deprecate these?
vtctld UI is going away, that had workflows.
We probably should also deprecate the vtctl Workflow commands.

Choose a reason for hiding this comment

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

fixed

--tablet_grpc_ca string the server ca to use to validate servers when connecting
--tablet_grpc_cert string the cert to use to connect
--tablet_grpc_crl string the server crl to use to validate server certificates when connecting
--tablet_grpc_key string the key to use to connect
--tablet_grpc_server_name string the server name to use to validate server certificate
--tablet_manager_protocol string the protocol to use to talk to vttablet (default grpc)
Copy link
Collaborator

@deepthi deepthi Oct 14, 2022

Choose a reason for hiding this comment

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

Why did this go away from the help? was this flag deprecated? or unused?

Choose a reason for hiding this comment

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

Not sure about this. But i saw couple its being used in vtcombo and vtgr ...

--tablet_refresh_known_tablets tablet refresh reloads the tablet address/port map from topo in case it changes (default true)
--tablet_protocol string Protocol to use to make queryservice RPCs to vttablets. (default "grpc")
--tablet_refresh_interval duration Tablet refresh interval. (default 1m0s)
--tablet_refresh_known_tablets Whether to reload the tablet's address/port map from topo in case they change. (default true)
--tablet_types_to_wait string wait till connected for specified tablet types during Gateway initialization
Copy link
Collaborator

Choose a reason for hiding this comment

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

how come this is a string but allowed_tablet_types is a slice?

Choose a reason for hiding this comment

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

I have made tablet_types_to_wait as string

Choose a reason for hiding this comment

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

also i have added that is command separated string

Comment on lines 198 to 104
--enable_query_plan_field_caching This option fetches & caches fields (columns) when storing query plans (default true)
--enable_query_plan_field_caching (DEPRECATED) This option fetches & caches fields (columns) when storing query plans (default true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should MarkDeprecated for this flag.

Choose a reason for hiding this comment

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

fixed

Comment on lines 331 to 220
--queryserver-config-pool-prefill-parallelism int query server read pool prefill parallelism, a non-zero value will prefill the pool using the specified parallism.
--queryserver-config-pool-prefill-parallelism int (DEPRECATED) query server read pool prefill parallelism, a non-zero value will prefill the pool using the specified parallism.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should use MarkDeprecated

Choose a reason for hiding this comment

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

fixed

Comment on lines 343 to 232
--queryserver-config-stream-pool-prefill-parallelism int query server stream pool prefill parallelism, a non-zero value will prefill the pool using the specified parallelism
--queryserver-config-stream-pool-prefill-parallelism int (DEPRECATED) query server stream pool prefill parallelism, a non-zero value will prefill the pool using the specified parallelism
Copy link
Collaborator

Choose a reason for hiding this comment

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

should use MarkDeprecated

Choose a reason for hiding this comment

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

fixed

Comment on lines 350 to 239
--queryserver-config-transaction-prefill-parallelism int query server transaction prefill parallelism, a non-zero value will prefill the pool using the specified parallism.
--queryserver-config-transaction-prefill-parallelism int (DEPRECATED) query server transaction prefill parallelism, a non-zero value will prefill the pool using the specified parallism.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should use MarkDeprecated.

Choose a reason for hiding this comment

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

fixed

Comment on lines 519 to 380
--xtrabackup_stream_mode string which mode to use if streaming, valid values are tar and xbstream (default tar)
--xtrabackup_stream_mode string which mode to use if streaming, valid values are tar and xbstream (default "tar")
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we document that tar is no longer supported in new versions of 8.0? I forget when it changed.
cc @mattlord

Choose a reason for hiding this comment

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

Yeah, it's not supported in XtraBackup 8.0 (and XtraBackup 8.0.X does not support MySQL 8.0.X+1)

Choose a reason for hiding this comment

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

done

--log_dir string If non-empty, write log files in this directory
--log_rotate_max_size uint size in bytes at which logs are rotated (glog.MaxSize) (default 1887436800)
--logtostderr log to standard error instead of files
--mysql_server_version string MySQL server version to advertise.
Copy link
Collaborator

Choose a reason for hiding this comment

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

how did this get into vtctldclient??!!!

Copy link

Choose a reason for hiding this comment

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

local client => ApplyVSchema => sqlparser.Parse => depends on mysql_server_version

Copy link

Choose a reason for hiding this comment

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

(among other commands that parse sql)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants