feat: spanner emulator support #4915

skuruppu · 2020-04-29T05:07:52Z

Allows users to set EmulatorDetection through SpannerConnectionStringBuilder.
Configures integration tests with EmulatorDetection.EmulatorOrProduction.

skuruppu · 2020-04-29T05:11:01Z

Currently this only addresses the second dot point of the comment. I am still figuring out how to address the third point.

@jskeet I just wanted some quick feedback from you on whether I'm on the right track :) No need to do a detailed review for now.

jskeet

Same comments will apply for all three - but yes, this looks it's the the right starting point. Once this is in place, I'm hoping the Spanner.Data integration should be straightforward.

...ud.Spanner.Admin.Database.V1/Google.Cloud.Spanner.Admin.Database.V1/DatabaseAdminClient.g.cs

...anner.Admin.Database.V1/Google.Cloud.Spanner.Admin.Database.V1/DatabaseAdminClientBuilder.cs

skuruppu · 2020-05-01T02:39:49Z

@jskeet this is now ready for review. I ran the ITs and they ran for the most part:

Total tests: 167
Passed: 92
Failed: 74
Skipped: 1

I will go through the failed tests to figure out which ones need to be skipped (due to the emulator not supporting all features that the production backend supports). This will need to be done in a separate PR. If there's anything that can be fixed in terms of the setup, I will do so in this PR.

jskeet

I suggest we split this PR in half: let's get the easy bits (the three underlying API clients) done first, then look at how we configure it from Spanner.Data as a separate PR.

But fundamentally I'd really like SpannerConnectionStringBuilder and SpannerClientCreationOptions to do the bulk of the work here.

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

jskeet · 2020-05-01T07:08:01Z

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

@@ -313,7 +323,8 @@ private static async Task<SpannerClient> CreateClientAsync(SpannerClientCreation
            return new SpannerClientBuilder
            {
                CallInvoker = callInvoker,
-                Settings = spannerSettings
+                Settings = spannerSettings,
+                EmulatorDetection = emulatorDetection


Setting a call invoker and emulator detection would be an odd thing to do. (I'm slightly surprised we don't prohibit it.)
If we do the work in SpannerClientCreationOptions (which may mean some code duplication, unfortunately - let's see what we can do) the endpoint and credentials will already be set correctly, so we shouldn't need to make any changes here.

Oh I see, so we shouldn't rely on the automated emulator detection for the data client. We should only do that for the admin clients. So we'll manually read the env var and set the endpoint and credentials. Is the worry that it would confuse users because they would set an endpoint and credentials but we'd be using a different parameters for the actual connection?

It's not a matter of "not relying" on the automated emulation code - it's not that we don't trust it. It's a matter of making it fit in well with the rest of the infrastructure.
I'd like to investigate how to do this without code duplication though - for example, it may be possible to add a public member to SpannerClientBuilder to say "please let me know how you would be configured for emulation" (effectively like the current partial method does) and then use the endpoint and credentials from that.

I think our aim should be for the behavior to be consistent between "using SpannerClientBuilder directly" and "using SpannerConnectionStringBuilder directly" in terms of the connection.

I've made the minimal amount of changes to get this working. But I'm still not updating the endpoint and credentials in the connection string to reflect what is actually being connected to. I think this is where your comment regarding updating the SpannerClientBuilder comes in.

The other issue that I want to solve is that if the connection string has EmulatorDetection then users shouldn't be asked to specify credentials. I'll also work on trying to bypass this check.

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

skuruppu · 2020-05-05T03:43:44Z

Thanks @jskeet. I agree about doing this as two separate PRs. To preserve the discussion, I may leave this PR as is to continue the work on setting the connection properties. I'll cherry-pick the builders into a separate PR.

jskeet · 2020-05-05T05:34:57Z

I've merged #4928 now, so it's probably worth rebasing this PR, unless you'd prefer to start a new one.

jskeet · 2020-05-13T06:01:39Z

I'll get to this when I can, but it may go into next week I'm afraid.

skuruppu · 2020-05-13T06:06:45Z

No worries @jskeet. I'm also sporadically working on it when I find bits of time here and there.

jskeet

This is moving in the right direction, but I think we probably need to move the CallInvoker creation from SessionPoolManager into SpannerClientCreationOptions. We may need to add something to SpannerClientBuilder to make it easier to say "What endpoint and credentials would you use for this?" without actually performing the build afterwards.

It's tricky for me to speculate on exactly what's required without basically doing it myself, unfortunately. Let me know how you'd like us to proceed.

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

skuruppu · 2020-05-20T04:20:04Z

This is what I came up with for now e98b470. I don't think you need to move the CallInvoker to SpannerClientCreationOptions, as long as the endpoint and credentials can be correctly extracted from that class.

My proposed change to SpannerClientBuilder is a bit of a mess since there's no nice way to extract the endpoint and credentials without attempting to build an instance of the class. For now I'm reading the env var manually. But if there was a static version of GetEmulatorEnvironment that also accepts an EmulatorDetection argument, then it might be cleaner.

Sorry, I know the back-and-forth is taking a bit of time. I'm more than happy for you to take a crack at this but I know you have a lot more stuff going on so I want to help as much as possible.

jskeet · 2020-05-20T05:41:26Z

Building an instance of SpannerClientBuilder is the way to go - we want to get the same exact behaviour as if we were specifying the emulator detection in the builder.

I would suggest just making MaybeCreateEmulatorClientBuilder public - then you can use that in SessionPoolManager. If it returns a non-null value, use the endpoint and credentials from that.

skuruppu · 2020-05-26T05:21:10Z

PTAL @jskeet. I made all the discussed changes.

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

jskeet · 2020-05-26T05:40:49Z

Will have a look when I get a chance, but it may well be quite late in the week.

jskeet

Getting a lot closer - but the session pool options bother me. Do you want me to see if I can figure out a way that we can make the options "just work"?

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

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

jskeet · 2020-05-27T10:42:59Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.CommonTesting/SpannerTestDatabase.cs

            };
+            var emulatorBuilder = clientBuilder.MaybeCreateEmulatorClientBuilder();


I wonder whether this should be in the production code, if everyone will always need to do that.
Do we know whether this is a long-term restriction?

In general, the changes in this file (beyond just setting EmulatorDetection) feel like they're a symptom of us not doing quite enough to "just make it work".

jskeet · 2020-05-27T10:46:19Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.CommonTesting/SpannerTestDatabase.cs

-            ConnectionString = databaseBuilder.ConnectionString;
-            DatabaseName = databaseBuilder.DatabaseName;
+            Builder = builder.WithDatabase(SpannerDatabase);
+            Builder.EmulatorDetection = emulatorDetection;


We shouldn't need to set the EmulatorDetection again, should we?

jskeet · 2020-05-27T10:47:35Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.CommonTesting/SpannerTestDatabase.cs

@@ -116,6 +143,6 @@ private static string GetEnvironmentVariableOrDefault(string name, string defaul
            return string.IsNullOrEmpty(value) ? defaultValue : value;
        }

-        public SpannerConnection GetConnection() => new SpannerConnection(ConnectionString);        
+        public SpannerConnection GetConnection() => new SpannerConnection(Builder);


Why is this change required? I'd expect the connection string to be fine.

Or is this because we've set the SessionPoolManager because of the options? It feels like a lot of this change could go away if we didn't need to set the options explicitly.

This reverts commit ccbc10d.

skuruppu · 2020-05-28T02:23:19Z

I addressed most of the changes except for the ones in https://github.com/googleapis/google-cloud-dotnet/pull/4915/files#diff-450b4cc43d8fb620f621d63b7de37a31 relating to the session pool.

For context, the changes to that file came out of #4970 when I was trying to run the existing ITs against the emulator. To be honest, I haven't quite gotten the whole thing to work yet even with that change.

My preference would be to revert the changes to that file and work on it separately. The highest priority is to add the emulator support to the client since there is a customer that's keen to try this out in C#.

Getting the ITs to work is a bonus so that we can report emulator issues to the dev team and perhaps have a presubmit that runs the ITs against the emulator. But this will require a bunch of work from my side and from the emulator team to fix some of the failing tests.

@jskeet I would be happy for you to try out a fix for the session pool issue. I'm not familiar with it enough to know what to change. But only if you have time. Otherwise, I'll work on it until I figure it out.

I don't know if the emulator team is planning to fix the concurrent transaction issue before GA. I will check with them and get back to you.

Let me know if you're ok with me reverting changes to that file.

jskeet

I agree that we should revert the integration test change - and the rest now looks good to me. I can try the integration test piece early next week, I expect, but we could get this in before then and do a beta release. The customer would need to configure the session pool options themselves in that beta release, but we can hope to either do it for them in a later release, or not need to do it at all if the emulator changes.

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

This reverts commit 677ae2f.

skuruppu · 2020-05-28T09:47:22Z

All done, thanks for the review feedback @jskeet.

jskeet

Thanks - will squash and merge, then look into the SessionPoolOptions aspect.

skuruppu added the api: spanner Issues related to the Spanner API. label Apr 29, 2020

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 29, 2020

skuruppu requested a review from jskeet April 29, 2020 05:09

jskeet reviewed Apr 29, 2020

View reviewed changes

...ud.Spanner.Admin.Database.V1/Google.Cloud.Spanner.Admin.Database.V1/DatabaseAdminClient.g.cs Outdated Show resolved Hide resolved

...anner.Admin.Database.V1/Google.Cloud.Spanner.Admin.Database.V1/DatabaseAdminClientBuilder.cs Outdated Show resolved Hide resolved

skuruppu marked this pull request as ready for review May 1, 2020 02:35

jskeet reviewed May 1, 2020

View reviewed changes

skuruppu marked this pull request as draft May 7, 2020 09:57

skuruppu force-pushed the spanner-emulator-support branch from 8cef6dd to 0968dd4 Compare May 13, 2020 01:36

skuruppu marked this pull request as ready for review May 13, 2020 05:06

skuruppu requested a review from a team as a code owner May 13, 2020 05:06

skuruppu mentioned this pull request May 14, 2020

Spanner: ITs running in parallel even though parallelization is disabled #4970

Closed

jskeet reviewed May 18, 2020

View reviewed changes

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerConnectionStringBuilder.cs Outdated Show resolved Hide resolved

skuruppu force-pushed the spanner-emulator-support branch from b4da2fa to e98b470 Compare May 20, 2020 02:45

skuruppu force-pushed the spanner-emulator-support branch from e98b470 to 0ce5165 Compare May 26, 2020 04:47

skuruppu commented May 26, 2020

View reviewed changes

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerClientCreationOptions.cs Outdated Show resolved Hide resolved

jskeet reviewed May 27, 2020

View reviewed changes

skuruppu added 4 commits May 28, 2020 11:43

feat: add emulator support in database admin client

18fd5e0

feat: add emulator support in instance admin client

61d8dec

feat: add emulator support in data client

4372112

fix: use InterceptBuild instead of PartialBuild

8e2d096

skuruppu added 15 commits May 28, 2020 11:43

fix: pass EmulatorDetection to SpannerClientBuilder

8825fda

fix: pass EmulatorDetection to db admin client builder

869a0b1

fix: set EmulatorOrProduction for ITs

e771962

fix: fixed tests

3ddfe42

test: added test for creating client with emulator

be9ef63

fix: EmulatorDetection is a connection string property

4c135c4

Revert "fix: fixed tests"

5577715

This reverts commit ccbc10d.

fix: get emulator detection from client options

4f419c0

fix: remove debug log

67160cd

fix: make MaybeCreateEmulatorClientBuilder public

74f9637

fix: call client builder w/ emulator endpoint/creds

f29ea42

test: pool has one session if using emulator

677ae2f

fix: conn string stores EmulatorDetection as str

5ac5fc4

fix: endpoint/creds calculated in client options

9899aeb

docs: improved MaybeCreateEmulatorClientBuilder docs

d367870

skuruppu force-pushed the spanner-emulator-support branch from c77186d to d367870 Compare May 28, 2020 02:15

jskeet reviewed May 28, 2020

View reviewed changes

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerClientBuilder.cs Outdated Show resolved Hide resolved

skuruppu added 2 commits May 28, 2020 19:43

Revert "test: pool has one session if using emulator"

97818d3

This reverts commit 677ae2f.

docs: fixed the doc string

23eb358

jskeet approved these changes May 28, 2020

View reviewed changes

jskeet merged commit a4455b9 into googleapis:master May 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: spanner emulator support #4915

feat: spanner emulator support #4915

skuruppu commented Apr 29, 2020 •

edited

Loading

skuruppu commented Apr 29, 2020

jskeet left a comment

skuruppu commented May 1, 2020 •

edited

Loading

jskeet left a comment

jskeet May 1, 2020

skuruppu May 5, 2020

jskeet May 5, 2020

skuruppu May 13, 2020

skuruppu commented May 5, 2020

jskeet commented May 5, 2020

jskeet commented May 13, 2020

skuruppu commented May 13, 2020

jskeet left a comment

skuruppu commented May 20, 2020

jskeet commented May 20, 2020

skuruppu commented May 26, 2020

jskeet commented May 26, 2020

jskeet left a comment

jskeet May 27, 2020

jskeet May 27, 2020

jskeet May 27, 2020

jskeet May 27, 2020

jskeet May 27, 2020

skuruppu commented May 28, 2020 •

edited

Loading

jskeet left a comment

skuruppu commented May 28, 2020

jskeet left a comment

		};
		var emulatorBuilder = clientBuilder.MaybeCreateEmulatorClientBuilder();

feat: spanner emulator support #4915

feat: spanner emulator support #4915

Conversation

skuruppu commented Apr 29, 2020 • edited Loading

skuruppu commented Apr 29, 2020

jskeet left a comment

Choose a reason for hiding this comment

skuruppu commented May 1, 2020 • edited Loading

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

skuruppu commented May 5, 2020

jskeet commented May 5, 2020

jskeet commented May 13, 2020

skuruppu commented May 13, 2020

jskeet left a comment

Choose a reason for hiding this comment

skuruppu commented May 20, 2020

jskeet commented May 20, 2020

skuruppu commented May 26, 2020

jskeet commented May 26, 2020

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

skuruppu commented May 28, 2020 • edited Loading

jskeet left a comment

Choose a reason for hiding this comment

skuruppu commented May 28, 2020

jskeet left a comment

Choose a reason for hiding this comment

skuruppu commented Apr 29, 2020 •

edited

Loading

skuruppu commented May 1, 2020 •

edited

Loading

skuruppu commented May 28, 2020 •

edited

Loading