googleapis / google-cloud-java

Google Cloud Client Library for Java
https://cloud.google.com/java/docs/reference
Apache License 2.0
1.89k stars 1.06k forks source link

Spanner client session pool: timeout for creating sessions, more visibility #2688

Closed maxqch closed 5 years ago

maxqch commented 6 years ago

Hi,

I've noticed that sometimes com.google.cloud.spanner.DatabaseClient#readWriteTransaction takes a long time (~5 seconds) to return the transaction runner.

We have setFailIfPoolExhausted set to true, so I believe this is the result of a CreateSession request taking a long time, possibly due to failing + retrying the rpc, though we have no visibility into this.

The library does not allow us to set a timeout for attempts to create sessions which can result in our threads being hung up for several seconds waiting for a session. This is problematic when clients retry and exhausting our server's thread pool since all the threads are blocked waiting for sessions.

It would be great if there was the option (either set in SessionPoolOption or as a parameter on the DatabaseClient functions) that dictates the maximum amount of time to wait trying to get a session.

Thanks!

vkedia commented 6 years ago

Thanks for filing this. I think what we need to do is to respect the deadline specified in grpc Context while waiting for a session to be available. As for visibility I am working on adding Tracing ( #2677 ) support into client library which should make it easier to debug such issues.

vchudnov-g commented 6 years ago

Does this address your question, @maxqch ?

maxqch commented 6 years ago

Tracing sounds great, I'm looking forward to it :)

I'm not sure I understand the grpc context deadline. How will this be exposed through the API? Also is there a ticket tracking the issue so it can get prioritized and worked on?

vkedia commented 6 years ago

We can use this issue to prioritize this. We already use deadline specified in grpc Context elsewhere in the spanner client library. We just need to change the place where we block for a session to be created to timeout if the deadline has elapsed. There will be no change in the API as far as I can see. You would do something like this:

Context.CancellableContext context =
        Context.current().withDeadlineAfter(10, TimeUnit.SECONDS, executor);
    Runnable work =
        context.wrap(
            new Runnable() {
              @Override
              public void run() {
                dbClient.readWriteTransaction().run(transactionCallable);
              }
            });
    work.run();

You can do this even now and we would respect the deadline and fail the operation if transaction took a time longer than the deadline except that we do not respect this while waiting for a session.

maxqch commented 6 years ago

Ah, I see, that makes sense. It would probably be helpful to add that to the API documentations. Maybe it's just me but I wasn't aware that an existing grpc context was being used for timeouts.

Sounds good to me.

JasonRosenberg commented 5 years ago

Any updates on this? We are running into this issue as well.

ajaaym commented 5 years ago

Change to use grpc context for timeouts while creating session is done and released.

ajaaym commented 5 years ago

PR is not merged yet, will track this issue in #3616