feat: Spanner leader routing #11654

amanda-tarafa · 2024-02-06T07:09:51Z

@jskeet as always one commit at a time. Although most of these will be squashed in a single commit for merging.

jskeet

What's there looks fine, but I'm slightly surprised there isn't connection string support. Is the idea that this would be disabled so rarely that we don't need to make it easy? (I can't remember exactly how SpannerConnection construction works - is there a way of passing in arbitrary settings?)

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerClientPartial.cs

jskeet · 2024-02-06T08:32:18Z

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerClientBuilderPartial.cs

 using System;
 using System.Threading;
 using System.Threading.Tasks;

 namespace Google.Cloud.Spanner.V1
 {
-    public partial class SpannerClientBuilder
-    {
+public partial class SpannerClientBuilder


There are some weird changes to indentation in this file...

Uhh, yes, fixing

jskeet · 2024-02-06T08:33:51Z

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerClientBuilderPartial.cs

@@ -76,5 +87,17 @@ public SpannerClientBuilder MaybeCreateEmulatorClientBuilder()
            builder.CopySettingsForEmulator(this);
            return builder;
        }
+
+        internal new T GetEffectiveSettings<T>(T settings) where T : ServiceSettingsBase, new()


Humbug - I guess we should have made the method virtual in GAX? Not that it's a problem, so long as we don't call it from GAX.

Yep, I did add this to the next major version things to consider.

jskeet · 2024-02-06T08:35:47Z

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerClientPartial.cs

+        {
+            if (Settings.LeaderRoutingEnabled)
+            {
+                settings = settings.WithHeader(LeaderRoutingHeader, true.ToString());


I think it would be clearer to put the constant in here, rather than calling true.ToString(). (It will also return "True" - I'd expect "true" to be more idiomatic, although I'm not sure the value actually matters.)

Yep using the constant. Value matters and the specified value on desing is "True". The header set to False, unset or not present are all equivalent and mean "do not route to leader".

jskeet · 2024-02-06T08:37:18Z

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerSettingsPartial.cs

+    /// some operations will never be explicitly routed to the leader, and some operations will
+    /// be routed to the leader depending on the transaction type they are using.
+    /// </remarks>
+    internal bool LeaderRoutingEnabled { get; set; } = true;


I'm slightly surprised this is internal rather than public. Is there a reason for that?

Yes, because this is just how we trickle this setting down to the client where we actually use it. It's not how this setting is exposed for configuration to the user, that is done through the SpannerClientBuilder for users using Spanner.V1 and the connection string for users using Spanner.Data.

If we make it public here and users also can set it, then we'd have to react to users providing different values in the pairs SpannerClientBuilder.LeaderRoutingEnabled + SpannerSettings.LeaderRoutingEnabled and ConnectionString.LeaderRoutingEnabled + SpannerSettings.LeaderRoutingEnabled. An alternative to avoiding that is removing the setting from SpannerClientBuilder and ConnectionString and make it only available through SpannerSettings. That wouldn't be a huge deal for Spanner.V1 users, they'd just have to set a SpannerClientBuilder.SpannerSettings = new SpannerSettings { LeaderRoutingEnabled = false; } but it would be worse for Spanner.Data users as they wouldn't be able to use the default SessionPoolManager, they'd had to build one themselves to be able to pass custom SpannerSettings.
Does this makes sense? The last commit is now the connection string bit, so happy to chat more about this.

Yup, that's fine.

jskeet · 2024-02-07T12:02:06Z

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1.Tests/ResourcePrefixHeaderTest.cs

@@ -146,7 +146,11 @@ public void SetsHeaderOnBeginTransaction()
        {
            var invoker = new FakeCallInvoker();
            var client = new SpannerClientBuilder { CallInvoker = invoker }.Build();
-            client.BeginTransaction(new BeginTransactionRequest { Session = SampleSessionName });
+            client.BeginTransaction(new BeginTransactionRequest


Rather than change the test, perhaps we should fix the code so that it doesn't fail with an NRE? (If it definitely would be an invalid request, we could potentially throw an ArgumentException, but to be honest I think it would be better to just default to a particular mode and let the server reject it.)

I suspect it's just a case of using request.Options?.ModeCase ?? TransactionOptions.ModeOneofCase.None

Yes, of course! Fixing.

amanda-tarafa

@jskeet all review comments addressed in new ordered commits prefixed "Address review". Plus a new commit at the end adding connection string support (I was waiting for confirmation this was wanted).

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerClientPartial.cs

amanda-tarafa · 2024-02-08T01:34:11Z

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerClientBuilderPartial.cs

 using System;
 using System.Threading;
 using System.Threading.Tasks;

 namespace Google.Cloud.Spanner.V1
 {
-    public partial class SpannerClientBuilder
-    {
+public partial class SpannerClientBuilder


Uhh, yes, fixing

amanda-tarafa · 2024-02-08T01:37:19Z

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerClientBuilderPartial.cs

@@ -76,5 +87,17 @@ public SpannerClientBuilder MaybeCreateEmulatorClientBuilder()
            builder.CopySettingsForEmulator(this);
            return builder;
        }
+
+        internal new T GetEffectiveSettings<T>(T settings) where T : ServiceSettingsBase, new()


Yep, I did add this to the next major version things to consider.

amanda-tarafa · 2024-02-08T01:49:00Z

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerClientPartial.cs

+        {
+            if (Settings.LeaderRoutingEnabled)
+            {
+                settings = settings.WithHeader(LeaderRoutingHeader, true.ToString());


Yep using the constant. Value matters and the specified value on desing is "True". The header set to False, unset or not present are all equivalent and mean "do not route to leader".

amanda-tarafa · 2024-02-08T01:50:35Z

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerSettingsPartial.cs

+    /// some operations will never be explicitly routed to the leader, and some operations will
+    /// be routed to the leader depending on the transaction type they are using.
+    /// </remarks>
+    internal bool LeaderRoutingEnabled { get; set; } = true;


Yes, because this is just how we trickle this setting down to the client where we actually use it. It's not how this setting is exposed for configuration to the user, that is done through the SpannerClientBuilder for users using Spanner.V1 and the connection string for users using Spanner.Data.

If we make it public here and users also can set it, then we'd have to react to users providing different values in the pairs SpannerClientBuilder.LeaderRoutingEnabled + SpannerSettings.LeaderRoutingEnabled and ConnectionString.LeaderRoutingEnabled + SpannerSettings.LeaderRoutingEnabled. An alternative to avoiding that is removing the setting from SpannerClientBuilder and ConnectionString and make it only available through SpannerSettings. That wouldn't be a huge deal for Spanner.V1 users, they'd just have to set a SpannerClientBuilder.SpannerSettings = new SpannerSettings { LeaderRoutingEnabled = false; } but it would be worse for Spanner.Data users as they wouldn't be able to use the default SessionPoolManager, they'd had to build one themselves to be able to pass custom SpannerSettings.
Does this makes sense? The last commit is now the connection string bit, so happy to chat more about this.

amanda-tarafa · 2024-02-08T05:43:37Z

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1.Tests/ResourcePrefixHeaderTest.cs

@@ -146,7 +146,11 @@ public void SetsHeaderOnBeginTransaction()
        {
            var invoker = new FakeCallInvoker();
            var client = new SpannerClientBuilder { CallInvoker = invoker }.Build();
-            client.BeginTransaction(new BeginTransactionRequest { Session = SampleSessionName });
+            client.BeginTransaction(new BeginTransactionRequest


Yes, of course! Fixing.

jskeet

Just one simple change requested, but I think it's an important one (or I've misunderstood something).

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerClientPartial.cs

jskeet · 2024-02-08T08:27:30Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerConnectionStringBuilder.cs

@@ -436,6 +437,20 @@ public EmulatorDetection EmulatorDetection
            }
        }

+        /// <summary>
+        /// Options to control leader routing. This is true by default.


I don't think this is true by default, and we don't have a test for it.

I suspect the change is just to call GetValueOrDefault(EnableLeaderRoutingKeyword, "True")

Yes! thanks for catching this. I'll add a test as well.

jskeet · 2024-02-08T08:28:03Z

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerSettingsPartial.cs

+    /// some operations will never be explicitly routed to the leader, and some operations will
+    /// be routed to the leader depending on the transaction type they are using.
+    /// </remarks>
+    internal bool LeaderRoutingEnabled { get; set; } = true;


Yup, that's fine.

amanda-tarafa

@jskeet all previous commits squashed into one plus a new commit fixing the leader routing default in the connection string (I'll squash that one as well before mergin.) PTAL.

amanda-tarafa requested a review from jskeet February 6, 2024 07:09

amanda-tarafa requested a review from a team as a code owner February 6, 2024 07:09

jskeet approved these changes Feb 6, 2024

View reviewed changes

amanda-tarafa force-pushed the spanner-lar branch from 14100f6 to 2b77148 Compare February 6, 2024 20:11

jskeet reviewed Feb 7, 2024

View reviewed changes

amanda-tarafa force-pushed the spanner-lar branch from 2b77148 to 268ac12 Compare February 8, 2024 07:58

amanda-tarafa commented Feb 8, 2024

View reviewed changes

jskeet reviewed Feb 8, 2024

View reviewed changes

refactor: Consolidate some partial classes in single files.

d699190

amanda-tarafa force-pushed the spanner-lar branch from 268ac12 to 070d09a Compare February 8, 2024 17:48

amanda-tarafa commented Feb 8, 2024

View reviewed changes

jskeet approved these changes Feb 8, 2024

View reviewed changes

feat: Add leader routing support.

cd5419c

amanda-tarafa force-pushed the spanner-lar branch from 070d09a to cd5419c Compare February 8, 2024 18:02

amanda-tarafa merged commit eb2157a into googleapis:main Feb 8, 2024
9 checks passed

amanda-tarafa deleted the spanner-lar branch February 8, 2024 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Spanner leader routing #11654

feat: Spanner leader routing #11654

amanda-tarafa commented Feb 6, 2024

jskeet left a comment

jskeet Feb 6, 2024

amanda-tarafa Feb 8, 2024

jskeet Feb 6, 2024

amanda-tarafa Feb 8, 2024

jskeet Feb 6, 2024

amanda-tarafa Feb 8, 2024

jskeet Feb 6, 2024

amanda-tarafa Feb 8, 2024

jskeet Feb 8, 2024

jskeet Feb 7, 2024

amanda-tarafa Feb 8, 2024

amanda-tarafa left a comment

amanda-tarafa Feb 8, 2024

amanda-tarafa Feb 8, 2024

amanda-tarafa Feb 8, 2024

amanda-tarafa Feb 8, 2024

amanda-tarafa Feb 8, 2024

jskeet left a comment

jskeet Feb 8, 2024

amanda-tarafa Feb 8, 2024

jskeet Feb 8, 2024

amanda-tarafa left a comment

feat: Spanner leader routing #11654

feat: Spanner leader routing #11654

Conversation

amanda-tarafa commented Feb 6, 2024

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment