feat: address review comments

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

changelog/22.0/22.0.0/summary.md

@@ -137,9 +137,9 @@ This is the last time this will be needed in the `8.0.x` series, as starting wit
 The base system now uses Debian Bookworm instead of Debian Bullseye for the `vitess/lite` images. This change was brought by [Pull Request #17552].
 
 ### <a id="key-range-vtorc"/>KeyRanges in `--clusters_to_watch` in VTOrc</a>
-VTOrc now supports specifying KeyRanges in the `--clusters_to_watch` flag. This is useful in scenarios where you don't need to restart a VTOrc instance if you run a reshard.
-For example, if a VTOrc is configured to watch `ks/-80`, then it would watch all the shards that fall under the KeyRange `-80`. If a reshard is run and, `-80` is split into new shards `-40`, and `40-80`, the VTOrc instance will automatically start watching the new shard without needing a restart. In the previous logic, watching a shard -80 would watch 1 (or 0) shard only. In the new system, since we interpret -80 as a key range, it can lead to a watch on multiple shards as described in the example.
-The users can still continue to specify exact key ranges too, and the new feature is backward compatible.
+VTOrc now supports specifying keyranges in the `--clusters_to_watch` flag. This means that there is no need to restart a VTOrc instance with a different flag value when you reshard a keyspace.
+For example, if a VTOrc is configured to watch `ks/-80`, then it would watch all the shards that fall under the keyrange `-80`. If a reshard is performed and `-80` is split into new shards `-40` and `40-80`, the VTOrc instance will automatically start watching the new shards without needing a restart. In the previous logic, specifying `ks/-80` for the flag would mean that VTOrc would watch only 1 (or no) shard. In the new system, since we interpret `-80` as a key range, it can watch multiple shards as described in the example.
+Users can continue to specify exact keyranges. The new feature is backward compatible.
 
 ### <a id="query-logs"/>Support for Filtering Query logs on Error</a>
 

go/flags/endtoend/vtorc.txt

@@ -24,7 +24,7 @@ Flags:
       --bind-address string                                         Bind address for the server. If empty, the server will listen on all available unicast and anycast IP addresses of the local system.
       --catch-sigpipe                                               catch and ignore SIGPIPE on stdout and stderr if specified
       --change-tablets-with-errant-gtid-to-drained                  Whether VTOrc should be changing the type of tablets with errant GTIDs to DRAINED
-      --clusters_to_watch strings                                   Comma-separated list of keyspaces or keyspace/key-ranges that this instance will monitor and repair. Defaults to all clusters in the topology. Example: "ks1,ks2/-80"
+      --clusters_to_watch strings                                   Comma-separated list of keyspaces or keyspace/keyranges that this instance will monitor and repair. Defaults to all clusters in the topology. Example: "ks1,ks2/-80"
       --config-file string                                          Full path of the config file (with extension) to use. If set, --config-path, --config-type, and --config-name are ignored.
       --config-file-not-found-handling ConfigFileNotFoundHandling   Behavior when a config file is not found. (Options: error, exit, ignore, warn) (default warn)
       --config-name string                                          Name of the config file (without extension) to search for. (default "vtconfig")

go/vt/vtorc/logic/tablet_discovery.go

@@ -60,7 +60,7 @@ var (
 
 // RegisterFlags registers the flags required by VTOrc
 func RegisterFlags(fs *pflag.FlagSet) {
-	fs.StringSliceVar(&clustersToWatch, "clusters_to_watch", clustersToWatch, "Comma-separated list of keyspaces or keyspace/key-ranges that this instance will monitor and repair. Defaults to all clusters in the topology. Example: \"ks1,ks2/-80\"")
+	fs.StringSliceVar(&clustersToWatch, "clusters_to_watch", clustersToWatch, "Comma-separated list of keyspaces or keyspace/keyranges that this instance will monitor and repair. Defaults to all clusters in the topology. Example: \"ks1,ks2/-80\"")
 	fs.DurationVar(&shutdownWaitTime, "shutdown_wait_time", shutdownWaitTime, "Maximum time to wait for VTOrc to release all the locks that it is holding before shutting down on SIGTERM")
 }
 
@@ -103,8 +103,8 @@ func initializeShardsToWatch() error {
 	return nil
 }
 
-// tabletPartOfWatch checks if the given tablet is part of the watch list.
-func tabletPartOfWatch(tablet *topodatapb.Tablet) bool {
+// shouldWatchTablet checks if the given tablet is part of the watch list.
+func shouldWatchTablet(tablet *topodatapb.Tablet) bool {
 	// If we are watching all keyspaces, then we want to watch this tablet too.
 	if len(shardsToWatch) == 0 {
 		return true
@@ -202,7 +202,7 @@ func refreshTabletsUsing(ctx context.Context, loader func(tabletAlias string), f
 	matchedTablets := make([]*topo.TabletInfo, 0, len(tablets))
 	func() {
 		for _, t := range tablets {
-			if tabletPartOfWatch(t.Tablet) {
+			if shouldWatchTablet(t.Tablet) {
 				matchedTablets = append(matchedTablets, t)
 			}
 		}

go/vt/vtorc/logic/tablet_discovery_test.go

@@ -103,7 +103,7 @@ var (
 	}
 )
 
-func TestTabletsPartOfWatch(t *testing.T) {
+func TestShouldWatchTablet(t *testing.T) {
 	oldClustersToWatch := clustersToWatch
 	defer func() {
 		clustersToWatch = oldClustersToWatch
@@ -113,103 +113,103 @@ func TestTabletsPartOfWatch(t *testing.T) {
 	testCases := []struct {
 		in                  []string
 		tablet              *topodatapb.Tablet
-		expectedPartOfWatch bool
+		expectedShouldWatch bool
 	}{
 		{
 			in: []string{},
 			tablet: &topodatapb.Tablet{
 				Keyspace: keyspace,
 				Shard:    shard,
 			},
-			expectedPartOfWatch: true,
+			expectedShouldWatch: true,
 		},
 		{
 			in: []string{keyspace},
 			tablet: &topodatapb.Tablet{
 				Keyspace: keyspace,
 				Shard:    shard,
 			},
-			expectedPartOfWatch: true,
+			expectedShouldWatch: true,
 		},
 		{
 			in: []string{keyspace + "/-"},
 			tablet: &topodatapb.Tablet{
 				Keyspace: keyspace,
 				Shard:    shard,
 			},
-			expectedPartOfWatch: true,
+			expectedShouldWatch: true,
 		},
 		{
 			in: []string{keyspace + "/" + shard},
 			tablet: &topodatapb.Tablet{
 				Keyspace: keyspace,
 				Shard:    shard,
 			},
-			expectedPartOfWatch: true,
+			expectedShouldWatch: true,
 		},
 		{
 			in: []string{"ks/-70", "ks/70-"},
 			tablet: &topodatapb.Tablet{
 				Keyspace: "ks",
 				KeyRange: key.NewKeyRange([]byte{0x50}, []byte{0x70}),
 			},
-			expectedPartOfWatch: true,
+			expectedShouldWatch: true,
 		},
 		{
 			in: []string{"ks/-70", "ks/70-"},
 			tablet: &topodatapb.Tablet{
 				Keyspace: "ks",
 				KeyRange: key.NewKeyRange([]byte{0x40}, []byte{0x50}),
 			},
-			expectedPartOfWatch: true,
+			expectedShouldWatch: true,
 		},
 		{
 			in: []string{"ks/-70", "ks/70-"},
 			tablet: &topodatapb.Tablet{
 				Keyspace: "ks",
 				KeyRange: key.NewKeyRange([]byte{0x70}, []byte{0x90}),
 			},
-			expectedPartOfWatch: true,
+			expectedShouldWatch: true,
 		},
 		{
 			in: []string{"ks/-70", "ks/70-"},
 			tablet: &topodatapb.Tablet{
 				Keyspace: "ks",
 				KeyRange: key.NewKeyRange([]byte{0x60}, []byte{0x90}),
 			},
-			expectedPartOfWatch: false,
+			expectedShouldWatch: false,
 		},
 		{
 			in: []string{"ks/50-70"},
 			tablet: &topodatapb.Tablet{
 				Keyspace: "ks",
 				KeyRange: key.NewKeyRange([]byte{0x50}, []byte{0x70}),
 			},
-			expectedPartOfWatch: true,
+			expectedShouldWatch: true,
 		},
 		{
 			in: []string{"ks2/-70", "ks2/70-", "unknownKs/-", "ks/-80"},
 			tablet: &topodatapb.Tablet{
 				Keyspace: "ks",
 				KeyRange: key.NewKeyRange([]byte{0x60}, []byte{0x80}),
 			},
-			expectedPartOfWatch: true,
+			expectedShouldWatch: true,
 		},
 		{
 			in: []string{"ks2/-70", "ks2/70-", "unknownKs/-", "ks/-80"},
 			tablet: &topodatapb.Tablet{
 				Keyspace: "ks",
 				KeyRange: key.NewKeyRange([]byte{0x80}, []byte{0x90}),
 			},
-			expectedPartOfWatch: false,
+			expectedShouldWatch: false,
 		},
 		{
 			in: []string{"ks2/-70", "ks2/70-", "unknownKs/-", "ks/-80"},
 			tablet: &topodatapb.Tablet{
 				Keyspace: "ks",
 				KeyRange: key.NewKeyRange([]byte{0x90}, []byte{0xa0}),
 			},
-			expectedPartOfWatch: false,
+			expectedShouldWatch: false,
 		},
 	}
 
@@ -218,7 +218,7 @@ func TestTabletsPartOfWatch(t *testing.T) {
 			clustersToWatch = tt.in
 			err := initializeShardsToWatch()
 			require.NoError(t, err)
-			assert.Equal(t, tt.expectedPartOfWatch, tabletPartOfWatch(tt.tablet))
+			assert.Equal(t, tt.expectedShouldWatch, shouldWatchTablet(tt.tablet))
 		})
 	}
 }
@@ -259,7 +259,7 @@ func TestInitializeShardsToWatch(t *testing.T) {
 		},
 		{
 			in:          []string{"test/324"},
-			expectedErr: `Invalid key range "324" while parsing clusters to watch`,
+			expectedErr: `invalid key range "324" while parsing clusters to watch`,
 		},
 		{
 			in: []string{"test/0"},