feat: address review comments

Signed-off-by: Manan Gupta <[email protected]>

changelog/20.0/20.0.0/summary.md

@@ -5,7 +5,7 @@
 - **[Major Changes](#major-changes)**
   - **[Breaking changes](#breaking-changes)**
     - [`shutdown_grace_period` Default Change](#shutdown-grace-period-default)
-    - [New `unmanaged` Flag](#unmanaged-flag)
+    - [New `unmanaged` Flag and `disable_active_reparents` deprecation](#unmanaged-flag)
   - **[Query Compatibility](#query-compatibility)**
     - [Vindex Hints](#vindex-hints)
     - [Update with Limit Support](#update-limit)
@@ -32,9 +32,9 @@ This makes reparenting in Vitess resilient to client errors, and prevents Planne
 
 In order to preserve the old behaviour, the users can set the flag back to `0 seconds` causing open transactions to never be shutdown, but in that case, they run the risk of PlannedReparentShard calls timing out.
 
-#### <a id="unmanaged-tablet"/> New `unmanaged` Flag
+#### <a id="unmanaged-tablet"/> New `unmanaged` Flag and `disable_active_reparents` deprecation
 
-New flag `--unmanaged` has been introduced in this release to make it easier to flag unmanaged tablets. It also runs validations to make sure the unmanaged tablets are configured properly.
+New flag `--unmanaged` has been introduced in this release to make it easier to flag unmanaged tablets. It also runs validations to make sure the unmanaged tablets are configured properly. `--disable_active_reparents` flag has been deprecated for `vttablet`, `vtcombo` and `vttestserver` binaries and will be removed in future releases. Specifying the `--unmanaged` flag will also block replication commands and replication repairs.
 
 Starting this release, all unmanaged tablets should specify this flag.
 

go/cmd/vttablet/cli/cli.go

@@ -82,7 +82,7 @@ See "Unmanaged Tablet" for the full guide.
 Even if a MySQL is external, you can still make vttablet perform some management functions. They are as follows:
 
 ` +
-			"* `--disable_active_reparents`: If this flag is set, then any reparent or replica commands will not be allowed. These are InitShardPrimary, PlannedReparentShard, EmergencyReparentShard, and ReparentTablet. In this mode, you should use the TabletExternallyReparented command to inform vitess of the current primary.\n" +
+			"* `--unmanaged`: This flag indicates that this tablet is running in unmanaged mode. In this mode, any reparent or replica commands are not allowed. These are InitShardPrimary, PlannedReparentShard, EmergencyReparentShard, and ReparentTablet. You should use the TabletExternallyReparented command to inform vitess of the current primary.\n" +
 			"* `--replication_connect_retry`: This value is give to mysql when it connects a replica to the primary as the retry duration parameter.\n" +
 			"* `--heartbeat_enable` and `--heartbeat_interval duration`: cause vttablet to write heartbeats to the sidecar database. This information is also used by the replication reporter to assess replica lag.\n",
 		Example: `

go/flags/endtoend/vtcombo.txt

@@ -108,7 +108,6 @@ Flags:
       --ddl_strategy string                                              Set default strategy for DDL statements. Override with @@ddl_strategy session variable (default "direct")
       --default_tablet_type topodatapb.TabletType                        The default tablet type to set for queries, when one is not explicitly selected. (default PRIMARY)
       --degraded_threshold duration                                      replication lag after which a replica is considered degraded (default 30s)
-      --disable_active_reparents                                         if set, do not allow active reparents. Use this to protect a cluster using external reparents.
       --emit_stats                                                       If set, emit stats to push-based monitoring and stats backends
       --enable-consolidator                                              Synonym to -enable_consolidator (default true)
       --enable-consolidator-replicas                                     Synonym to -enable_consolidator_replicas

go/flags/endtoend/vttablet.txt

@@ -17,7 +17,7 @@ See "Unmanaged Tablet" for the full guide.
 
 Even if a MySQL is external, you can still make vttablet perform some management functions. They are as follows:
 
-* `--disable_active_reparents`: If this flag is set, then any reparent or replica commands will not be allowed. These are InitShardPrimary, PlannedReparentShard, EmergencyReparentShard, and ReparentTablet. In this mode, you should use the TabletExternallyReparented command to inform vitess of the current primary.
+* `--unmanaged`: This flag indicates that this tablet is running in unmanaged mode. In this mode, any reparent or replica commands are not allowed. These are InitShardPrimary, PlannedReparentShard, EmergencyReparentShard, and ReparentTablet. You should use the TabletExternallyReparented command to inform vitess of the current primary.
 * `--replication_connect_retry`: This value is give to mysql when it connects a replica to the primary as the retry duration parameter.
 * `--heartbeat_enable` and `--heartbeat_interval duration`: cause vttablet to write heartbeats to the sidecar database. This information is also used by the replication reporter to assess replica lag.
 
@@ -139,7 +139,6 @@ Flags:
       --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)
       --degraded_threshold duration                                      replication lag after which a replica is considered degraded (default 30s)
-      --disable_active_reparents                                         if set, do not allow active reparents. Use this to protect a cluster using external reparents.
       --emit_stats                                                       If set, emit stats to push-based monitoring and stats backends
       --enable-consolidator                                              Synonym to -enable_consolidator (default true)
       --enable-consolidator-replicas                                     Synonym to -enable_consolidator_replicas

go/flags/endtoend/vttestserver.txt

@@ -31,7 +31,6 @@ Flags:
       --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)
       --default_schema_dir string                                        Default directory for initial schema files. If no schema is found in schema_dir, default to this location.
-      --disable_active_reparents                                         if set, do not allow active reparents. Use this to protect a cluster using external reparents.
       --enable_direct_ddl                                                Allow users to submit direct DDL statements (default true)
       --enable_online_ddl                                                Allow users to submit, review and control Online DDL (default true)
       --enable_system_settings                                           This will enable the system settings to be changed per session at the database connection level (default true)

go/vt/mysqlctl/mysqld.go

@@ -122,9 +122,12 @@ func init() {
 	for _, cmd := range []string{"mysqlctl", "mysqlctld", "vtcombo", "vttablet", "vttestserver"} {
 		servenv.OnParseFor(cmd, registerMySQLDFlags)
 	}
-	for _, cmd := range []string{"vtcombo", "vttablet", "vttestserver", "vtctld", "vtctldclient"} {
+	for _, cmd := range []string{"vtctld", "vtctldclient"} {
 		servenv.OnParseFor(cmd, registerReparentFlags)
 	}
+	for _, cmd := range []string{"vtcombo", "vttablet", "vttestserver"} {
+		servenv.OnParseFor(cmd, registerDeprecatedReparentFlags)
+	}
 	for _, cmd := range []string{"mysqlctl", "mysqlctld", "vtcombo", "vttablet", "vttestserver"} {
 		servenv.OnParseFor(cmd, registerPoolFlags)
 	}
@@ -141,6 +144,11 @@ func registerReparentFlags(fs *pflag.FlagSet) {
 	fs.BoolVar(&DisableActiveReparents, "disable_active_reparents", DisableActiveReparents, "if set, do not allow active reparents. Use this to protect a cluster using external reparents.")
 }
 
+func registerDeprecatedReparentFlags(fs *pflag.FlagSet) {
+	fs.BoolVar(&DisableActiveReparents, "disable_active_reparents", DisableActiveReparents, "if set, do not allow active reparents. Use this to protect a cluster using external reparents.")
+	fs.MarkDeprecated("disable_active_reparents", "Use --unmanaged flag instead for unmanaged tablets.")
+}
+
 func registerPoolFlags(fs *pflag.FlagSet) {
 	fs.IntVar(&dbaPoolSize, "dba_pool_size", dbaPoolSize, "Size of the connection pool for dba connections")
 	fs.DurationVar(&DbaIdleTimeout, "dba_idle_timeout", DbaIdleTimeout, "Idle timeout for dba connections")

go/vt/vttablet/tabletserver/tabletenv/config.go

@@ -909,9 +909,8 @@ func (c *TabletConfig) verifyUnmanagedTabletConfig() error {
 	if c.DB.App.Password == "" {
 		return errors.New("database app user password not specified")
 	}
-	if !mysqlctl.DisableActiveReparents {
-		return errors.New("fixing replication should be disabled on unmanaged tablets")
-	}
+	// Replication fixes should be disabled for Unmanaged tablets.
+	mysqlctl.DisableActiveReparents = true
 
 	return c.checkConnectionForExternalMysql()
 }

go/vt/vttablet/tabletserver/tabletenv/config_test.go

@@ -478,11 +478,6 @@ func TestVerifyUnmanagedTabletConfig(t *testing.T) {
 	assert.EqualError(t, err, "database app user password not specified")
 
 	config.DB.App.Password = "testPassword"
-	mysqlctl.DisableActiveReparents = false
-	err = config.verifyUnmanagedTabletConfig()
-	assert.EqualError(t, err, "fixing replication should be disabled on unmanaged tablets")
-
-	mysqlctl.DisableActiveReparents = true
 	err = config.verifyUnmanagedTabletConfig()
 	assert.Nil(t, err)
 }