Add support for Directed reads by takabow · Pull Request #163 · cloudspannerecosystem/spanner-cli

takabow · 2024-02-22T05:51:20Z

Spanner's Directed reads are currently in public preview. This PR adds support for Directed reads in spanner-cli.

Currently, Spanner does not expose metrics that easily indicate whether Directed reads are applied, other than CPU util% per location. The most straightforward indicator is to observe the latency added based on the distance to the leader region when performing a strong read with follower replicas specified in Directed reads.

For example, in an asia1 configuration, you can observe the latency added between Osaka and Tokyo when performing directed reads from asia-northeast1(Tokyo) to asia-northeast2(Osaka).

A query without directed read option:

$ ./spanner-cli -p data-db-demo -i test-instance -d test-db 
Connected.
spanner> select 1;
+---+
|   |
+---+
| 1 |
+---+
1 rows in set (1.74 msecs)

A query with directed read option:

$ ./spanner-cli -p data-db-demo -i test-instance -d test-db --directedread asia-northeast2
Connected.
spanner> select 1;
+---+
|   |
+---+
| 1 |
+---+
1 rows in set (9.8 msecs) <--- add network latency between Osaka-Tokyo

Please note that this latency is a metric included in the SELECT results, and it is the latency measured on the Spanner side, not on the application side.

yfuruyama

Thank you for writing this change! I commented some parts of the change, so please take a look when you have a chance. Thanks!

yfuruyama · 2024-02-24T04:36:26Z

+	Priority     string `long:"priority" description:"Set default request priority (HIGH|MEDIUM|LOW)"`
+	Role         string `long:"role" description:"Use the specific database role"`
+	Endpoint     string `long:"endpoint" description:"Set the Spanner API endpoint (host:port)"`
+	DirectedRead string `long:"directedread" description:"Directed read replica location and type."`


Maybe --directed-read might be more readable since it's two terms.

Also let's add what the syntax looks like into description (e.g. Directed read option (replica_location:replica_type). The replicat_type is optional and either READ_ONLY or READ_WRITE).

yfuruyama · 2024-02-24T04:38:42Z

+	directedReadOptionsForClient := &pb.DirectedReadOptions{
+		Replicas: &pb.DirectedReadOptions_IncludeReplicas_{
+			IncludeReplicas: &pb.DirectedReadOptions_IncludeReplicas{
+				ReplicaSelections:    replicaSelections,


We don't need replicaSelections variable, instead we can write as follows:

IncludeReplicas: &pb.DirectedReadOptions_IncludeReplicas{ ReplicaSelections: []*pb.DirectedReadOptions_ReplicaSelection{replicaSelection}, AutoFailoverDisabled: false, }

yfuruyama · 2024-02-24T04:40:09Z

+		Replicas: &pb.DirectedReadOptions_IncludeReplicas_{
+			IncludeReplicas: &pb.DirectedReadOptions_IncludeReplicas{
+				ReplicaSelections:    replicaSelections,
+				AutoFailoverDisabled: false,


How about changing AutoFailoverDisabled to true so that typo in the directed read options will be eventually turned into the error?

https://cloud.google.com/spanner/docs/reference/rpc/google.spanner.v1#google.spanner.v1.DirectedReadOptions.IncludeReplicas

Even if pass true to this AutoFailoverDisabled, typos and other misconfigurations will still be ignored, and directed reads will simply be disabled. This option seems to only work if we pass the correct configuration and the replica is in a failed state at the time. That said, I agree with the idea of setting this to true so I will fix it.

I fix it with AutoFailoverDisabled: true

I didn't know how AutoFailoverDisabled exactly works. Thank you for your clarification.

yfuruyama · 2024-02-24T04:53:24Z

+
+	if len(directedReadOption) == 2 {
+		switch strings.ToUpper(directedReadOption[1]) {
+		case "RO", "READ_ONLY":


Maybe we don't need abbreviation (RO and RW) since it's a bit obscure for people who see this parameter at first. What do you think?

I tried to make the command-line options easy, but I realize I may have gone too far. Let's use the official options for now.

yfuruyama · 2024-02-24T04:54:32Z

+		},
+	}
+
+	return directedReadOptionsForClient, nil


It looks like this variable is not needed. Let's directly return: return &pb.DirectedReadOptions {....}, nil

yfuruyama · 2024-02-24T05:10:04Z

+	} {
+		t.Run(tt.desc, func(t *testing.T) {
+			got, _ := parseDirectedReadOption(tt.option)
+			//fmt.Println(got)


Let's remove this comment.

yfuruyama · 2024-02-24T05:12:23Z

+		t.Run(tt.desc, func(t *testing.T) {
+			got, _ := parseDirectedReadOption(tt.option)
+			//fmt.Println(got)
+			if !reflect.DeepEqual(got, tt.want) {


cmp package is safer alternative to reflect: https://pkg.go.dev/github.com/google/go-cmp/cmp

See an example:

spanner-cli/cli_test.go

Line 109 in 4ad906e

if !cmp.Equal(got, test.Expected) {

Thank you!
Fix with !cmp.Equal(got, tt.want, protocmp.Transform())

yfuruyama · 2024-02-24T05:17:08Z


+### Directed reads mode
+
+spanner-cli now supports Directed reads, a feature that allows you to read data from a specific replica of a Spanner database. 


nit: Maybe we can say directed reads instead of Directed reads except begging of the line to follow the document: https://cloud.google.com/spanner/docs/directed-reads

I've unified the inconsistent.

yfuruyama · 2024-02-24T05:19:25Z

+
+	directedReadOption := strings.Split(directedReadOptionText, ":")
+	if len(directedReadOption) > 2 {
+		return nil, fmt.Errorf("directed read option must be in the form of <replica_region>:<replica_type>, but got %q", directedReadOptionText)


Maybe <replica_location> would be better since API uses location: https://cloud.google.com/spanner/docs/directed-reads#parameters

I've unified the inconsistent.

yfuruyama · 2024-02-24T05:21:53Z

+The `--directedread` flag takes a single argument, which is the name of the replica that you want to read from.
+The replica name can be specified in one of the following formats:
+
+- `<location>`


Let's use consistent naming over document and code. Currently code throws error with <replica_location> and <replica_type>.

I think either is fine, but consistent naming is important: <location>:<type> or <replica_location>:<replica_type>.

I've unified the inconsistent.

yfuruyama · 2024-02-25T12:38:02Z

+      --history=       Set the history file to the specified path
+      --priority=      Set default request priority (HIGH|MEDIUM|LOW)
+      --role=          Use the specific database role
+      --directed-read= Set the location and replica type for directed reads.


Can we add the details to this help message as well?

added the same message as help message.

yfuruyama · 2024-02-25T12:39:27Z

+- `<replica_location>`
+- `<replica_location>:<replica_type>`
+
+The `<replica_location>` part of the replica name specifies the region where the replica is located such as `us-central1`, `asia-northeast2`.  


The <replica_location> part of the replica name specifies the region

Maybe "The <replica_location> specifies the region ..." would be simpler.

yfuruyama · 2024-02-25T12:39:46Z

+- `<replica_location>:<replica_type>`
+
+The `<replica_location>` part of the replica name specifies the region where the replica is located such as `us-central1`, `asia-northeast2`.  
+The `<replica_type>` part of the replica name specifies the type of the replica either `READ_WRITE` or `READ_ONLY`. 


Maybe "The <replica_type> specifies the type of ..." would be simpler?

yfuruyama · 2024-02-25T12:42:40Z

+> [!NOTE]
+> If you specify an incorrect region or type for directed reads, directed reads will not be enabled and [your requsts won't be routed as expected](https://cloud.google.com/spanner/docs/directed-reads#parameters). For example, in a multi-region configuration `nam3`, if you mistype `us-east1` as `us-east-1`, the connection will succeed, but directed reads will not be enabled. 
+>
+> To perform directed reads to `asia-northeast2` in a multi-region configuration `asia1`, you need to specify `asia-northeast2:READ_WRITE`.


you need to specify asia-northeast2:READ_WRITE.

you need to specify asia-northeast2 or asia-northeast2:READ_WRITE?

…actions.

takabow · 2024-02-26T07:10:23Z

 	if s.InReadWriteTransaction() {
+		// The current Go Spanner client library does not apply client-level directed read options to read-write transactions.
+		// Therefore, we explicitly set query-level options here to fail the query during a read-write transaction.
+		opts.DirectedReadOptions = s.clientConfig.DirectedReadOptions


The current Go Spanner client library does not seem to apply client-level directed read options to RW Tx. Therefore, even if we set the directed reads option, queries executed within a RW Tx will be routed to the read-write replicas of the leader region.

From the perspective of spanner-cli usage, it may be preferable for queries to fail when a RW Tx is performed with directed read options set. So, we explicitly set query-level options to cause queries to fail during RW Tx with the following error:

spanner(rw txn)> select 1; ERROR: spanner: code = "InvalidArgument", desc = "Directed reads can only be performed in a read-only transaction."

yfuruyama

LGTM. Thank you for your continuous effort on this PR!

yfuruyama reviewed Feb 24, 2024

View reviewed changes

takabow force-pushed the feature/support-directed-read branch from 7cd94d2 to 289cded Compare February 25, 2024 06:43

Add option for Directed Read

45f6372

takabow force-pushed the feature/support-directed-read branch from 289cded to 562b8f0 Compare February 25, 2024 08:10

yfuruyama reviewed Feb 25, 2024

View reviewed changes

takabow added 2 commits February 26, 2024 04:43

Add test and README for Directed reads

fc24005

Explicitly set query-level directed read options for read-write trans…

d5d2dc5

…actions.

takabow force-pushed the feature/support-directed-read branch from 562b8f0 to d5d2dc5 Compare February 26, 2024 07:03

takabow commented Feb 26, 2024

View reviewed changes

yfuruyama approved these changes Feb 26, 2024

View reviewed changes

Bump up Go version to 1.19

8576507

yfuruyama merged commit 4099da8 into cloudspannerecosystem:master Feb 26, 2024


		### Directed reads mode

		spanner-cli now supports Directed reads, a feature that allows you to read data from a specific replica of a Spanner database.

Conversation

takabow commented Feb 22, 2024

Uh oh!

yfuruyama left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

takabow Feb 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yfuruyama Feb 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yfuruyama left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

takabow Feb 25, 2024 •

edited

Loading

yfuruyama Feb 24, 2024 •

edited

Loading