Skip to content

feat(Bigtable): expose totalTimeoutMillis in RetrySettings for Read and Mutate Rows#8223

Merged
bshaffer merged 6 commits intomainfrom
bigtable-expose-operation-timeout
Jun 18, 2025
Merged

feat(Bigtable): expose totalTimeoutMillis in RetrySettings for Read and Mutate Rows#8223
bshaffer merged 6 commits intomainfrom
bigtable-expose-operation-timeout

Conversation

@bshaffer
Copy link
Copy Markdown
Contributor

@bshaffer bshaffer commented Apr 11, 2025

b/391904703

Fixes the Bigtable conformance tests by ensuring that an operation timeout is set:

  • TestMutateRows_Generic_DeadlineExceeded
  • TestReadRow_Generic_DeadlineExceeded
  • TestReadRows_Generic_DeadlineExceeded

An operation timeout (timeout for the entire operation including retries) can now be set by passing totalTimeoutMillis as part of the RetryOptions array:

// This number is passed to gRPC, and ensures a single attempt does not take longer than this number
$perAttemptTimeoutMillis = 100;

// This number is used in the library to keep track of the time spent making the attempts. If the total time
// for this operation exceeds this number, an exception is thrown
$perOperationTimeoutMillis = 3000;

$table->mutateRows($mutations, [
    'timeoutMillis' => $perAttemptTimeoutMillis,
    'retrySettings' => [
        'totalTimeoutMillis' => $perOperationTimeoutMillis,
    ]
]);

Important: I've added preempting rpc timeout in 1e152c1 which overrides timeoutMillis when it's less than the remaining time in the operation. For example, if we retry twice at 100ms and our totalTimeoutMillis is 220ms, then the third retry attempt will have a timeoutMillis of 20s. Otherwise the totalTimeoutMillis wouldn't kick in until after 300ms.

@bshaffer bshaffer requested review from a team April 11, 2025 22:30
@product-auto-label product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Apr 11, 2025
mutianf
mutianf previously approved these changes Apr 30, 2025
Comment thread Bigtable/src/ResumableStream.php
Comment thread Bigtable/src/Table.php
* Only $maxRetries and $totalTimeoutMillis work for RetrySettings in this API.
*
* @option int $maxRetries Number of times to retry. **Defaults to** `3`.
* @option int $totalTimeoutMillis The max accumulative timeout in total.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should we specify in the doc that 0 means no total timeout?

Copy link
Copy Markdown
Contributor Author

@bshaffer bshaffer May 12, 2025

Choose a reason for hiding this comment

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

We can do that, although it maybe relevant to note that in the other API methods, RetrySettings::$totalTimeoutMillis is required, and so setting it to 0 actually results in the value 0 for total timeout (which would essentially disable retries).

For that reason, it probably would make more sense for us to make -1 mean no total timeout, so that if a user passes in 0 in both cases, the behavior is consistent. What do you think? Do you know what other language libraries are doing?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I know in java 0 means no total timeout

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Then unless it's a requirement for this feature (which maybe it is?) I'd rather leave it consistent with the rest of our RetrySettings

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ok I think we can keep the current behavior and just add a comment to call it out

Comment thread Bigtable/src/ResumableStream.php Outdated
return $retrySettings->getTotalTimeoutMillis();
}

return $retrySettings['totalTimeoutMillis'] ?? 0;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

note: this means the default is no retries, which we probably don't want to do

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what's the current behavior?

@bshaffer
Copy link
Copy Markdown
Contributor Author

@mutianf Ping on this, I'd love your LGTM if possible!

mutianf
mutianf previously approved these changes Jun 17, 2025
Comment thread Bigtable/tests/Conformance/proxy/src/ProxyService.php
@bshaffer bshaffer force-pushed the bigtable-expose-operation-timeout branch from 70ec7b7 to 55454c5 Compare June 18, 2025 18:13
@bshaffer bshaffer enabled auto-merge (squash) June 18, 2025 18:39
@bshaffer bshaffer disabled auto-merge June 18, 2025 18:40
@bshaffer bshaffer added the next release PRs to be included in the next release label Jun 18, 2025
@bshaffer bshaffer enabled auto-merge (squash) June 18, 2025 20:16
@bshaffer bshaffer merged commit 1b4a1c9 into main Jun 18, 2025
33 checks passed
@bshaffer bshaffer deleted the bigtable-expose-operation-timeout branch June 18, 2025 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable Issues related to the Bigtable API. next release PRs to be included in the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants