Skip to content

Commit 7a07c2b

Browse files
authored
refactor: PingingPool pings sessions using SELECT 1 (#75)
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](https://github.com/googleapis/google-cloud-go/blob/53898305c6f21b3c3eef34fcff6c61a2cb36f602/spanner/session.go#L227):
1 parent 8a3d700 commit 7a07c2b

File tree

4 files changed

+89
-5
lines changed

4 files changed

+89
-5
lines changed

google/cloud/spanner_v1/pool.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,7 @@ class PingingPool(AbstractSessionPool):
314314
- Sessions are used in "round-robin" order (LRU first).
315315
316316
- "Pings" existing sessions in the background after a specified interval
317-
via an API call (``session.exists()``).
317+
via an API call (``session.ping()``).
318318
319319
- Blocks, with a timeout, when :meth:`get` is called on an empty pool.
320320
Raises after timing out.
@@ -387,6 +387,9 @@ def get(self, timeout=None): # pylint: disable=arguments-differ
387387
ping_after, session = self._sessions.get(block=True, timeout=timeout)
388388

389389
if _NOW() > ping_after:
390+
# Using session.exists() guarantees the returned session exists.
391+
# session.ping() uses a cached result in the backend which could
392+
# result in a recently deleted session being returned.
390393
if not session.exists():
391394
session = self._new_session()
392395
session.create()
@@ -430,7 +433,9 @@ def ping(self):
430433
# Re-add to queue with existing expiration
431434
self._sessions.put((ping_after, session))
432435
break
433-
if not session.exists(): # stale
436+
try:
437+
session.ping()
438+
except NotFound:
434439
session = self._new_session()
435440
session.create()
436441
# Re-add to queue with new expiration

google/cloud/spanner_v1/session.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,17 @@ def delete(self):
153153

154154
api.delete_session(self.name, metadata=metadata)
155155

156+
def ping(self):
157+
"""Ping the session to keep it alive by executing "SELECT 1".
158+
159+
:raises: ValueError: if :attr:`session_id` is not already set.
160+
"""
161+
if self._session_id is None:
162+
raise ValueError("Session ID not set by back-end")
163+
api = self._database.spanner_api
164+
metadata = _metadata_with_prefix(self._database.name)
165+
api.execute_sql(self.name, "SELECT 1", metadata=metadata)
166+
156167
def snapshot(self, **kw):
157168
"""Create a snapshot to perform a set of reads with shared staleness.
158169

tests/unit/test_pool.py

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -567,7 +567,7 @@ def test_ping_oldest_fresh(self):
567567

568568
pool.ping()
569569

570-
self.assertFalse(SESSIONS[0]._exists_checked)
570+
self.assertFalse(SESSIONS[0]._pinged)
571571

572572
def test_ping_oldest_stale_but_exists(self):
573573
import datetime
@@ -584,7 +584,7 @@ def test_ping_oldest_stale_but_exists(self):
584584
with _Monkey(MUT, _NOW=lambda: later):
585585
pool.ping()
586586

587-
self.assertTrue(SESSIONS[0]._exists_checked)
587+
self.assertTrue(SESSIONS[0]._pinged)
588588

589589
def test_ping_oldest_stale_and_not_exists(self):
590590
import datetime
@@ -602,7 +602,7 @@ def test_ping_oldest_stale_and_not_exists(self):
602602
with _Monkey(MUT, _NOW=lambda: later):
603603
pool.ping()
604604

605-
self.assertTrue(SESSIONS[0]._exists_checked)
605+
self.assertTrue(SESSIONS[0]._pinged)
606606
SESSIONS[1].create.assert_called()
607607

608608

@@ -850,6 +850,7 @@ def __init__(self, database, exists=True, transaction=None):
850850
self._database = database
851851
self._exists = exists
852852
self._exists_checked = False
853+
self._pinged = False
853854
self.create = mock.Mock()
854855
self._deleted = False
855856
self._transaction = transaction
@@ -861,6 +862,13 @@ def exists(self):
861862
self._exists_checked = True
862863
return self._exists
863864

865+
def ping(self):
866+
from google.cloud.exceptions import NotFound
867+
868+
self._pinged = True
869+
if not self._exists:
870+
raise NotFound("expired session")
871+
864872
def delete(self):
865873
from google.cloud.exceptions import NotFound
866874

tests/unit/test_session.py

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,66 @@ def test_exists_error(self):
210210
metadata=[("google-cloud-resource-prefix", database.name)],
211211
)
212212

213+
def test_ping_wo_session_id(self):
214+
database = self._make_database()
215+
session = self._make_one(database)
216+
with self.assertRaises(ValueError):
217+
session.ping()
218+
219+
def test_ping_hit(self):
220+
gax_api = self._make_spanner_api()
221+
gax_api.execute_sql.return_value = "1"
222+
database = self._make_database()
223+
database.spanner_api = gax_api
224+
session = self._make_one(database)
225+
session._session_id = self.SESSION_ID
226+
227+
session.ping()
228+
229+
gax_api.execute_sql.assert_called_once_with(
230+
self.SESSION_NAME,
231+
"SELECT 1",
232+
metadata=[("google-cloud-resource-prefix", database.name)],
233+
)
234+
235+
def test_ping_miss(self):
236+
from google.api_core.exceptions import NotFound
237+
238+
gax_api = self._make_spanner_api()
239+
gax_api.execute_sql.side_effect = NotFound("testing")
240+
database = self._make_database()
241+
database.spanner_api = gax_api
242+
session = self._make_one(database)
243+
session._session_id = self.SESSION_ID
244+
245+
with self.assertRaises(NotFound):
246+
session.ping()
247+
248+
gax_api.execute_sql.assert_called_once_with(
249+
self.SESSION_NAME,
250+
"SELECT 1",
251+
metadata=[("google-cloud-resource-prefix", database.name)],
252+
)
253+
254+
def test_ping_error(self):
255+
from google.api_core.exceptions import Unknown
256+
257+
gax_api = self._make_spanner_api()
258+
gax_api.execute_sql.side_effect = Unknown("testing")
259+
database = self._make_database()
260+
database.spanner_api = gax_api
261+
session = self._make_one(database)
262+
session._session_id = self.SESSION_ID
263+
264+
with self.assertRaises(Unknown):
265+
session.ping()
266+
267+
gax_api.execute_sql.assert_called_once_with(
268+
self.SESSION_NAME,
269+
"SELECT 1",
270+
metadata=[("google-cloud-resource-prefix", database.name)],
271+
)
272+
213273
def test_delete_wo_session_id(self):
214274
database = self._make_database()
215275
session = self._make_one(database)

0 commit comments

Comments
 (0)