refactor: PingingPool pings sessions using SELECT 1

[larkee]

Currently, PingingPool pings sessions in the background by calling session.exists() which calls GetSession.

Using SELECT 1 is preferred and is used in other client libraries such as Go:

[hengfengli]

Generally, it looks good to me. Please resolve the comments.

[hengfengli]

LGTM. So when a session is deleted, calling session.ping() will cause a NotFound error, is this right?

[larkee]

LGTM. So when a session is deleted, calling session.ping() will cause a NotFound error, is this right?

Yes. However there seems to be a delay before the error appears. Given the following code, the error only occurs when DELAY is over ~35s

from google.cloud import spanner
from time import sleep

client = spanner.Client()
instance = client.instance(INSTANCE_ID)
database = instance.database(DATABASE_ID)
session = database.session()
session.create()
session.delete()
sleep(DELAY)
session.ping()

[hengfengli]

It makes sense. The deletion needs time to be propagated.

So if a session is deleted, when we still execute a transaction on it, what will happen then? When it gets a SessionNotFound error, will it try to get a new session?

[larkee]

It makes sense. The deletion needs time to be propagated.

So if a session is deleted, when we still execute a transaction on it, what will happen then? When it gets a SessionNotFound error, will it try to get a new session?

Currently, it will not. However, if we keep the session.exists() call in the get() then we don't have to worry about users encountering this error. Since, that isn't called in the background I think it's still acceptable.

[hengfengli]

LGTM. So now, pool.get() uses session.exists() to verify the validness of a session instead of session.ping(). Can we leave a comment over there why we don't use session.ping()?

The reason why we don't use session.ping() is that it has a cached/outdated result, which is inaccurate, is this correct?

[larkee]

LGTM. So now, pool.get() uses session.exists() to verify the validness of a session instead of session.ping(). Can we leave a comment over there why we don't use session.ping()?

Good suggestion! Done :)

The reason why we don't use session.ping() is that it has a cached/outdated result, which is inaccurate, is this correct?

Yes, the backend uses a cached value which means a delete may have occurred but not propagated yet.

[hengfengli]

LGTM.