Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
skuruppu opened this issue May 14, 2020 · 11 comments
Closed
Assignees
Labels
api: spanner Issues related to the Spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: question Request for information or clarification. Not an issue.

Comments

@skuruppu
Copy link
Contributor

I have some questions regarding how integration tests are run. I'm testing the C# client library against the Cloud Spanner emulator. When I ran the tests, I got a whole lot of errors like:

Error Message:
   Google.Cloud.Spanner.Data.SpannerException : The operation was aborted.
---- Grpc.Core.RpcException : Status(StatusCode=Aborted, Detail="Transaction 16323 aborted due to active transaction 56. The emulator only supports one transaction at a time.")

Out of 167 tests, 106 of them failed. Then I ran some of these with a filter to run tests one file at a time or as specific tests. Then only 13 tests fail.

The only explanation I had for this was that tests were running in parallel and the emulator doesn't support concurrent transactions.

Then I checked

[assembly: CollectionBehavior(DisableTestParallelization = true)]
and saw that we disable parallelization.

Now I don't have an explanation for why the tests can't be run.

Environment details

Steps to reproduce

  1. Setup the emulator as per instructions.
  2. Patch feat: spanner emulator support #4915.
  3. Run the following commands:
export GOOGLE_APPLICATION_CREDENTIALS=<creds>
export TEST_PROJECT=emulator-test-project
export SPANNER_EMULATOR_HOST=localhost:9010
cd /google-cloud-dotnet/apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.IntegrationTests
dotnet test
@skuruppu skuruppu added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. api: spanner Issues related to the Spanner API. labels May 14, 2020
@skuruppu skuruppu self-assigned this May 14, 2020
@skuruppu
Copy link
Contributor Author

@amanda-tarafa could I kindly ask you if I misunderstood something about the IT setup.

@amanda-tarafa amanda-tarafa added type: question Request for information or clarification. Not an issue. and removed type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels May 14, 2020
@amanda-tarafa amanda-tarafa self-assigned this May 14, 2020
@amanda-tarafa
Copy link
Contributor

I don't think this has to do with test parallelization but with session pool warmup (a pooled session has an active transaction associated with it).
For running in the emulator you should configure the SessionPool so that it only holds one pooled session at any given time. That you can do using SessionPoolOptions wich in Google.Cloud.Spanner.Data you can set by first building a SessionPoolManager with the desired options, and then using that to set the SpannerConnectionStringBuilder.SessionPoolManager.

That said, I don't know what the intended use cases for the emulator are, but if it's to run production like code, I believe it should at least simulate supporting multiple transactions, else, the code ran in the emulator will have to be different from the production code. Those are my two cents with close to no context.

Let me know if this doesn't help and I'll try to reproduce then to see if there are errors in our code. The tests should really be running in parallel.

@skuruppu
Copy link
Contributor Author

Ah ok thanks for the suggestion @amanda-tarafa. I will try your idea (it may be next week before I get to it) and let you know if I need your help with digging into it further.

@amanda-tarafa
Copy link
Contributor

The tests should really be running in parallel.

When I said that, I really meant, "The tests shouldn't be running in parallel given our current testing configuration." Sorry for the very confusing typo.
Let me know if I can help.

@skuruppu
Copy link
Contributor Author

Hmmm I explicitly initialized a session pool at

.

            SessionPoolOptions options = new SessionPoolOptions
            {
                MinimumPooledSessions = 1,
                MaximumActiveSessions = 1
            };
            builder.SessionPoolManager = SessionPoolManager.Create(options);

No luck though.

I'm trying out just one file at time. For example in https://github.com/googleapis/google-cloud-dotnet/blob/master/apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.IntegrationTests/BatchDmlTests.cs, all the failures happen at the first line of the test string key = _fixture.CreateTestRows();.

I'm going to keep digging.

@amanda-tarafa
Copy link
Contributor

        SessionPoolOptions options = new SessionPoolOptions
        {
            MinimumPooledSessions = 1,
            MaximumActiveSessions = 1
        };
        builder.SessionPoolManager = SessionPoolManager.Create(options);

Ahh, but this wouldn't be enough. The builder is only being used to get the connection string and then the SpannerConnetion is being built from the connection string and not from the builder. The SessionPoolManager set in the builder is not "persisted" as part of the connection string, so really the connections are being created with a default SessionPoolManager. In this case you would have to store the builder and build the SpannerConnections from the builder and not from the connection string.

One thing though, for confirming that the emulator runs if the SessionPool is configured to only have one transation, it is allright if you make changes on SpannerTestDatabase, but we wouldn't want these changes permanently, we still want our normal integration tests to run with a "normal" (default) SessionPool and not with one restricted to just one transaction. You would have to detect the emulator on SpannerTestDatabase and configure the pool accordingly. This goes then towards my previous comment re having different code for production and emulator.

@skuruppu
Copy link
Contributor Author

Bingo, that was the problem. Thanks so much for noticing this @amanda-tarafa :)

Oh yeah, I wouldn't commit the change I mentioned in my comment. This is me just hacking the test code to see how it works. In the PR, I'll make sure to only set the session pool conditional on if the tests are going to run against the emulator.

@amanda-tarafa
Copy link
Contributor

Cool! Let me know if I can help and if the tests pass now in the emulator.

@skuruppu
Copy link
Contributor Author

So most of the tests seem to pass, although I think something got stuck because I didn't see it complete the last time I tried to run all the tests. But I'm no longer getting the transaction aborted exception from my first comment.

Now the tests that fail are expected to do so. I'm reporting them to the emulator team so that they can either fix them or if that's infeasible, then we can conditionally skip them if the emulator is enabled.

I may ask you more questions next week as I run into more issues :)

@amanda-tarafa
Copy link
Contributor

@skuruppu Can we close this issue given that the original question is now resolved?

@skuruppu
Copy link
Contributor Author

Yup agree. I will reopen or create a separate issue if anything more comes up. Thanks again for your help @amanda-tarafa.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: question Request for information or clarification. Not an issue.
Projects
None yet
Development

No branches or pull requests

2 participants