Skip to content
This repository was archived by the owner on Apr 1, 2026. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions google/cloud/bigtable/data/_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from google.api_core import exceptions as core_exceptions
from google.api_core.retry import exponential_sleep_generator
from google.api_core.retry import RetryFailureReason
from google.rpc.error_details_pb2 import RetryInfo
from google.cloud.bigtable.data.exceptions import RetryExceptionGroup

if TYPE_CHECKING:
Expand Down Expand Up @@ -288,11 +289,10 @@ def set_next(self, next_value: float):
self._next_override = next_value

def __next__(self) -> float:
next_backoff = next(self.subgenerator)
if self._next_override is not None:
next_backoff = self._next_override
self._next_override = None
else:
next_backoff = next(self.subgenerator)
self.history.append(next_backoff)
return next_backoff

Expand All @@ -308,3 +308,15 @@ def get_attempt_backoff(self, attempt_idx) -> float:
if attempt_idx < 0:
raise IndexError("received negative attempt number")
return self.history[attempt_idx]

def set_from_exception_info(self, retry_info: RetryInfo):
"""
Use a RetryInfo object to set the next sleep time.

If a problem is encountered, this method does nothing.
"""
try:
retry_seconds = retry_info.retry_delay.ToTimedelta().total_seconds()
self.set_next(retry_seconds)
except Exception:
pass
Comment thread
daniel-sanche marked this conversation as resolved.
Outdated
12 changes: 11 additions & 1 deletion google/cloud/bigtable/data/_metrics/tracked_retry.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
from grpc import StatusCode
from google.api_core.exceptions import GoogleAPICallError
from google.api_core.retry import RetryFailureReason
from google.rpc.error_details_pb2 import RetryInfo
from google.cloud.bigtable.data.exceptions import _MutateRowsIncomplete
from google.cloud.bigtable.data._helpers import _retry_exception_factory
from google.cloud.bigtable.data._metrics import ActiveOperationMetric
Expand All @@ -43,10 +44,14 @@

def _track_retryable_error(
operation: ActiveOperationMetric,
backoff_generator: TrackedBackoffGenerator,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-medium medium

The function _track_retryable_error has been updated to require a mandatory backoff_generator argument. However, the call site on line 118 (within _track_terminal_error) was not updated to provide this argument. This will lead to a TypeError and an application crash whenever a timeout occurs during an active retry attempt. Please ensure all call sites are updated to provide the required argument (e.g., _track_retryable_error(operation, operation.backoff_generator)(attempt_exc)).

) -> Callable[[Exception], None]:
"""
Used as input to api_core.Retry classes, to track when retryable errors are encountered

If an exception is encountered with Retryinfo set, it will inform the backoff generator
to give it a chance to override the next backoff value

Should be passed as on_error callback
"""

Expand All @@ -59,6 +64,11 @@ def wrapper(exc: Exception) -> None:
rpc_error.initial_metadata()
)
operation.add_response_metadata({k: v for k, v in metadata})
# check for RetryInfo:
if exc.details:
info_matches = [field for field in exc.details if isinstance(field, RetryInfo)]
if info_matches:
backoff_generator.set_from_exception_info(info_matches[0])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The list comprehension [field for ...] builds a full list of all RetryInfo objects found in exc.details, even though only the first one is used. For efficiency, especially if exc.details could be large, it's better to use a generator expression with next() to find only the first match without iterating over the entire list unnecessarily.

Suggested change
info_matches = [field for field in exc.details if isinstance(field, RetryInfo)]
if info_matches:
backoff_generator.set_from_exception_info(info_matches[0])
first_info = next((field for field in exc.details if isinstance(field, RetryInfo)), None)
if first_info:
backoff_generator.set_from_exception_info(first_info)

except Exception:
# ignore errors in metadata collection
pass
Expand Down Expand Up @@ -127,7 +137,7 @@ def tracked_retry(
kwargs.pop("sleep_generator", None)
return retry_fn(
sleep_generator=operation.backoff_generator,
on_error=_track_retryable_error(operation),
on_error=_track_retryable_error(operation, operation.backoff_generator),
exception_factory=_track_terminal_error(operation, in_exception_factory),
**kwargs,
)
Loading