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

feat: Add PingingPool check on session age #1069

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MostafaOmar98
Copy link

@MostafaOmar98 MostafaOmar98 commented Jan 6, 2024

Fixes #1068

Implemented the first proposed solution mentioned in issue #1068 .

Details

  1. Override _new_session in PingingPool to add a timestamp _created_at on the newly created session.
  2. Change PingingPool.get() to check whether the session is older than 28 days old using the _created_at we added. If so, we go ahead and check the existence of the session

Notes/concerns:

  1. I'm not really sure if the 28-day deletion rule on spanner side is a definite promise or more of a guideline (i.e., sessions older than 28 days may or may not be deleted).
    1.1.) If sessions may not be deleted, would it be better to just go ahead and replace old session with a new one instead of possibly checking the existence each time it is used to avoid latency?
  2. This solution does not guarantee that all sessions in the pool are alive at all times. I intentionally didn't add the check on session age in ping because:
    a) We will have to iterate over all sessions to check their _created_at (since the one on top of the pool might not necessarily be the oldest in terms of age)
    b) Or we will check the _created_at of the least recently used session only. Which fixes the issue for the lru session but doesn't help for other sessions if the lru session is fresh and the pinging loop breaks early.
    c) We already check for session age at the time of get, so the client won't receive dead sessions (atleast the ones deleted for this reason). And it is such an infrequent case (once a month), So it shouldn't add latency on most requests.

@MostafaOmar98 MostafaOmar98 requested review from a team as code owners January 6, 2024 19:11
Copy link

google-cla bot commented Jan 6, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/python-spanner API. labels Jan 6, 2024
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.

Server-side deleted sessions (specifically for being older than 28 days) are not refreshed in PingingPool
1 participant