Skip to content
This repository was archived by the owner on Mar 2, 2026. It is now read-only.

Commit 04bb206

Browse files
daniel-sanchegkevinzheng
daniel-sanche
and
gkevinzheng
authored
fix: remove custom retry loop (#948)
* fix: remove custom retry loop * removed async implementation * removed unneeded imports --------- Co-authored-by: Kevin Zheng <147537668+gkevinzheng@users.noreply.github.com>
1 parent fabc701 commit 04bb206

5 files changed

Lines changed: 16 additions & 577 deletions

File tree

‎google/cloud/firestore_v1/async_transaction.py‎

Lines changed: 8 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,12 @@
1515
"""Helpers for applying Google Cloud Firestore changes in a transaction."""
1616

1717

18-
import asyncio
19-
import random
2018
from typing import Any, AsyncGenerator, Callable, Coroutine
2119

2220
from google.api_core import exceptions, gapic_v1
2321
from google.api_core import retry_async as retries
2422

25-
from google.cloud.firestore_v1 import _helpers, async_batch, types
23+
from google.cloud.firestore_v1 import _helpers, async_batch
2624
from google.cloud.firestore_v1.async_document import (
2725
AsyncDocumentReference,
2826
DocumentSnapshot,
@@ -33,18 +31,12 @@
3331
_CANT_COMMIT,
3432
_CANT_ROLLBACK,
3533
_EXCEED_ATTEMPTS_TEMPLATE,
36-
_INITIAL_SLEEP,
37-
_MAX_SLEEP,
38-
_MULTIPLIER,
3934
_WRITE_READ_ONLY,
4035
MAX_ATTEMPTS,
4136
BaseTransaction,
4237
_BaseTransactional,
4338
)
4439

45-
# Types needed only for Type Hints
46-
from google.cloud.firestore_v1.client import Client
47-
4840

4941
class AsyncTransaction(async_batch.AsyncWriteBatch, BaseTransaction):
5042
"""Accumulate read-and-write operations to be sent in a transaction.
@@ -140,8 +132,13 @@ async def _commit(self) -> list:
140132
if not self.in_progress:
141133
raise ValueError(_CANT_COMMIT)
142134

143-
commit_response = await _commit_with_retry(
144-
self._client, self._write_pbs, self._id
135+
commit_response = await self._client._firestore_api.commit(
136+
request={
137+
"database": self._client._database_string,
138+
"writes": self._write_pbs,
139+
"transaction": self._id,
140+
},
141+
metadata=self._client._rpc_metadata,
145142
)
146143

147144
self._clean_up()
@@ -313,76 +310,3 @@ def async_transactional(
313310
the wrapped callable.
314311
"""
315312
return _AsyncTransactional(to_wrap)
316-
317-
318-
# TODO(crwilcox): this was 'coroutine' from pytype merge-pyi...
319-
async def _commit_with_retry(
320-
client: Client, write_pbs: list, transaction_id: bytes
321-
) -> types.CommitResponse:
322-
"""Call ``Commit`` on the GAPIC client with retry / sleep.
323-
324-
Retries the ``Commit`` RPC on Unavailable. Usually this RPC-level
325-
retry is handled by the underlying GAPICd client, but in this case it
326-
doesn't because ``Commit`` is not always idempotent. But here we know it
327-
is "idempotent"-like because it has a transaction ID. We also need to do
328-
our own retry to special-case the ``INVALID_ARGUMENT`` error.
329-
330-
Args:
331-
client (:class:`~google.cloud.firestore_v1.client.Client`):
332-
A client with GAPIC client and configuration details.
333-
write_pbs (List[:class:`google.cloud.proto.firestore.v1.write.Write`, ...]):
334-
A ``Write`` protobuf instance to be committed.
335-
transaction_id (bytes):
336-
ID of an existing transaction that this commit will run in.
337-
338-
Returns:
339-
:class:`google.cloud.firestore_v1.types.CommitResponse`:
340-
The protobuf response from ``Commit``.
341-
342-
Raises:
343-
~google.api_core.exceptions.GoogleAPICallError: If a non-retryable
344-
exception is encountered.
345-
"""
346-
current_sleep = _INITIAL_SLEEP
347-
while True:
348-
try:
349-
return await client._firestore_api.commit(
350-
request={
351-
"database": client._database_string,
352-
"writes": write_pbs,
353-
"transaction": transaction_id,
354-
},
355-
metadata=client._rpc_metadata,
356-
)
357-
except exceptions.ServiceUnavailable:
358-
# Retry
359-
pass
360-
361-
current_sleep = await _sleep(current_sleep)
362-
363-
364-
async def _sleep(
365-
current_sleep: float, max_sleep: float = _MAX_SLEEP, multiplier: float = _MULTIPLIER
366-
) -> float:
367-
"""Sleep and produce a new sleep time.
368-
369-
.. _Exponential Backoff And Jitter: https://www.awsarchitectureblog.com/\
370-
2015/03/backoff.html
371-
372-
Select a duration between zero and ``current_sleep``. It might seem
373-
counterintuitive to have so much jitter, but
374-
`Exponential Backoff And Jitter`_ argues that "full jitter" is
375-
the best strategy.
376-
377-
Args:
378-
current_sleep (float): The current "max" for sleep interval.
379-
max_sleep (Optional[float]): Eventual "max" sleep time
380-
multiplier (Optional[float]): Multiplier for exponential backoff.
381-
382-
Returns:
383-
float: Newly doubled ``current_sleep`` or ``max_sleep`` (whichever
384-
is smaller)
385-
"""
386-
actual_sleep = random.uniform(0.0, current_sleep)
387-
await asyncio.sleep(actual_sleep)
388-
return min(multiplier * current_sleep, max_sleep)

‎google/cloud/firestore_v1/base_transaction.py‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,6 @@
3939
_CANT_ROLLBACK: str = _MISSING_ID_TEMPLATE.format("rolled back")
4040
_CANT_COMMIT: str = _MISSING_ID_TEMPLATE.format("committed")
4141
_WRITE_READ_ONLY: str = "Cannot perform write operation in read-only transaction."
42-
_INITIAL_SLEEP: float = 1.0
43-
"""float: Initial "max" for sleep interval. To be used in :func:`_sleep`."""
44-
_MAX_SLEEP: float = 30.0
45-
"""float: Eventual "max" sleep time. To be used in :func:`_sleep`."""
46-
_MULTIPLIER: float = 2.0
47-
"""float: Multiplier for exponential backoff. To be used in :func:`_sleep`."""
4842
_EXCEED_ATTEMPTS_TEMPLATE: str = "Failed to commit transaction in {:d} attempts."
4943
_CANT_RETRY_READ_ONLY: str = "Only read-write transactions can be retried."
5044

‎google/cloud/firestore_v1/transaction.py‎

Lines changed: 8 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@
1515
"""Helpers for applying Google Cloud Firestore changes in a transaction."""
1616

1717

18-
import random
19-
import time
2018
from typing import Any, Callable, Generator
2119

2220
from google.api_core import exceptions, gapic_v1
@@ -31,17 +29,13 @@
3129
_CANT_COMMIT,
3230
_CANT_ROLLBACK,
3331
_EXCEED_ATTEMPTS_TEMPLATE,
34-
_INITIAL_SLEEP,
35-
_MAX_SLEEP,
36-
_MULTIPLIER,
3732
_WRITE_READ_ONLY,
3833
MAX_ATTEMPTS,
3934
BaseTransaction,
4035
_BaseTransactional,
4136
)
4237
from google.cloud.firestore_v1.document import DocumentReference
4338
from google.cloud.firestore_v1.query import Query
44-
from google.cloud.firestore_v1.types import CommitResponse
4539

4640

4741
class Transaction(batch.WriteBatch, BaseTransaction):
@@ -138,7 +132,14 @@ def _commit(self) -> list:
138132
if not self.in_progress:
139133
raise ValueError(_CANT_COMMIT)
140134

141-
commit_response = _commit_with_retry(self._client, self._write_pbs, self._id)
135+
commit_response = self._client._firestore_api.commit(
136+
request={
137+
"database": self._client._database_string,
138+
"writes": self._write_pbs,
139+
"transaction": self._id,
140+
},
141+
metadata=self._client._rpc_metadata,
142+
)
142143

143144
self._clean_up()
144145
return list(commit_response.write_results)
@@ -301,75 +302,3 @@ def transactional(to_wrap: Callable) -> _Transactional:
301302
the wrapped callable.
302303
"""
303304
return _Transactional(to_wrap)
304-
305-
306-
def _commit_with_retry(
307-
client, write_pbs: list, transaction_id: bytes
308-
) -> CommitResponse:
309-
"""Call ``Commit`` on the GAPIC client with retry / sleep.
310-
311-
Retries the ``Commit`` RPC on Unavailable. Usually this RPC-level
312-
retry is handled by the underlying GAPICd client, but in this case it
313-
doesn't because ``Commit`` is not always idempotent. But here we know it
314-
is "idempotent"-like because it has a transaction ID. We also need to do
315-
our own retry to special-case the ``INVALID_ARGUMENT`` error.
316-
317-
Args:
318-
client (:class:`~google.cloud.firestore_v1.client.Client`):
319-
A client with GAPIC client and configuration details.
320-
write_pbs (List[:class:`google.cloud.proto.firestore.v1.write.Write`, ...]):
321-
A ``Write`` protobuf instance to be committed.
322-
transaction_id (bytes):
323-
ID of an existing transaction that this commit will run in.
324-
325-
Returns:
326-
:class:`google.cloud.firestore_v1.types.CommitResponse`:
327-
The protobuf response from ``Commit``.
328-
329-
Raises:
330-
~google.api_core.exceptions.GoogleAPICallError: If a non-retryable
331-
exception is encountered.
332-
"""
333-
current_sleep = _INITIAL_SLEEP
334-
while True:
335-
try:
336-
return client._firestore_api.commit(
337-
request={
338-
"database": client._database_string,
339-
"writes": write_pbs,
340-
"transaction": transaction_id,
341-
},
342-
metadata=client._rpc_metadata,
343-
)
344-
except exceptions.ServiceUnavailable:
345-
# Retry
346-
pass
347-
348-
current_sleep = _sleep(current_sleep)
349-
350-
351-
def _sleep(
352-
current_sleep: float, max_sleep: float = _MAX_SLEEP, multiplier: float = _MULTIPLIER
353-
) -> float:
354-
"""Sleep and produce a new sleep time.
355-
356-
.. _Exponential Backoff And Jitter: https://www.awsarchitectureblog.com/\
357-
2015/03/backoff.html
358-
359-
Select a duration between zero and ``current_sleep``. It might seem
360-
counterintuitive to have so much jitter, but
361-
`Exponential Backoff And Jitter`_ argues that "full jitter" is
362-
the best strategy.
363-
364-
Args:
365-
current_sleep (float): The current "max" for sleep interval.
366-
max_sleep (Optional[float]): Eventual "max" sleep time
367-
multiplier (Optional[float]): Multiplier for exponential backoff.
368-
369-
Returns:
370-
float: Newly doubled ``current_sleep`` or ``max_sleep`` (whichever
371-
is smaller)
372-
"""
373-
actual_sleep = random.uniform(0.0, current_sleep)
374-
time.sleep(actual_sleep)
375-
return min(multiplier * current_sleep, max_sleep)

0 commit comments

Comments
 (0)