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

fix: add NOT_FOUND error check in __exit__ method of SessionCheckout. #718

Merged
merged 4 commits into from
Apr 20, 2022

Conversation

vi3k6i5
Copy link
Contributor

@vi3k6i5 vi3k6i5 commented Apr 19, 2022

PingingPool currently checks if session exists in .get() method only when _NOW() > ping_after. If the session is used in any query and then returned the pool then the ping_after is reset to _NOW() _ + delta. If the session is deleted in the backend and the checkout gives a not found error the session will still get returned to the pool with a reset for ping_after = _NOW() _ + delta.

As described in the comment in get() method.

Using session.exists() guarantees the returned session exists.
session.ping() uses a cached result in the backend which could
result in a recently deleted session being returned.

Example:
An Application using PingingPool with 1 Session. The session has been killed on the server side, but the ping method has not cleared out the session from the pool yet because ping_after > _NOW() is not True.

Now pool.get() will return the session, and SnapshotCheckout will use the session and fail and return the session back into the pool.

Put Method will put the session in the pool with a new wait_time, and the process will continue.

To avoid this inside the __exit__ method of SessionCheckout we should check if NOT_FOUND error was raised or not. If that error was raised then create a new session and push it inside the pool.

…on was raised for the session and create new session if needed
@vi3k6i5 vi3k6i5 requested review from a team as code owners April 19, 2022 11:22
@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: spanner Issues related to the googleapis/python-spanner API. labels Apr 19, 2022
@vi3k6i5 vi3k6i5 marked this pull request as draft April 19, 2022 11:36
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: xs Pull request size is extra small. labels Apr 19, 2022
@vi3k6i5 vi3k6i5 changed the title fix: add session.exists() check in PingingPool.get() method. fix: add NOT_FOUND error check in __exit__ method of SessionCheckout. Apr 19, 2022
@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: m Pull request size is medium. labels Apr 19, 2022
@vi3k6i5 vi3k6i5 marked this pull request as ready for review April 19, 2022 16:46
Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@larkee larkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 One minor suggestion for another test case

tests/unit/test_database.py Show resolved Hide resolved
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Apr 20, 2022
@vi3k6i5 vi3k6i5 merged commit 265e207 into googleapis:main Apr 20, 2022
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 googleapis/python-spanner API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants