Retry for rows that return retryable error codes. - #4116
Closed
justinseinlin wants to merge 237 commits into
Closed
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
garye
reviewed
Oct 3, 2017
garye
left a comment
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
This definitely looks like its on the right track to me. Just a few initial comments...
Sorry, something went wrong.
Comment thread
bigtable/google/cloud/bigtable/table.py
Outdated
| RETRY_CODES = ( | ||
| StatusCode.DEADLINE_EXCEEDED.value[0], | ||
| StatusCode.ABORTED.value[0], | ||
| StatusCode.INTERNAL.value[0], |
This comment was marked as spam.
This comment was marked as spam.
Comment thread
bigtable/google/cloud/bigtable/table.py
Outdated
| return self._do_mutate_retryable_rows() | ||
|
|
||
| def _is_retryable(self, status): | ||
| return status is None or status.code in _RetryableMutateRowsWorker.RETRY_CODES |
This comment was marked as spam.
This comment was marked as spam.
Comment thread
bigtable/google/cloud/bigtable/table.py
Outdated
| def __call__(self): | ||
| return self._do_mutate_retryable_rows() | ||
|
|
||
| def _is_retryable(self, status): |
This comment was marked as spam.
Comment thread
bigtable/google/cloud/bigtable/table.py
Outdated
| for entry in response.entries: | ||
| index = self._next_retryable_row_index(index) | ||
| self.responses_statuses[index] = entry.status | ||
| has_retryable_responses = self._is_retryable(entry.status) |
This comment was marked as spam.
This comment was marked as spam.
…ponses can get overridden by non-retryableresponse.
…googleapis#4098) * Removing `googleapis-common-protos` from deps in non-`core` packages. Also - removing `grpcio` from non-`core` packages. - manually specifying the `grpcio` dep in core (rather than getting it from `googleapis-common-protos[grpc]`) * Making `grpc` an extra for `core`. * Adding `googleapis-common-protos` back to `videointelligence`.
tseaver
previously requested changes
Oct 4, 2017
tseaver
left a comment
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
lint failures:
google/cloud/bigtable/table.py:328:80: E501 line too long (99 > 79 characters) google/cloud/bigtable/table.py:393:80: E501 line too long (82 > 79 characters) google/cloud/bigtable/table.py:399:80: E501 line too long (86 > 79 characters) google/cloud/bigtable/table.py:407:16: E225 missing whitespace around operator google/cloud/bigtable/table.py:420:80: E501 line too long (83 > 79 characters)
cover failures:
Name Stmts Miss Branch BrPart Cover Missing ------------------------------------------------------------------------------------ google/cloud/bigtable/__init__.py 5 0 0 0 100% google/cloud/bigtable/client.py 95 0 24 0 100% google/cloud/bigtable/cluster.py 61 0 10 0 100% google/cloud/bigtable/column_family.py 101 0 30 0 100% google/cloud/bigtable/instance.py 87 0 16 0 100% google/cloud/bigtable/row.py 157 0 30 0 100% google/cloud/bigtable/row_data.py 206 0 72 0 100% google/cloud/bigtable/row_filters.py 222 0 68 0 100% google/cloud/bigtable/table.py 193 11 74 7 93% 332-337, 407-408, 418, 430, 436, 329->337, 403->408, 405->407, 413->412, 416->418, 429->430, 435->436 tests/unit/__init__.py 0 0 0 0 100% tests/unit/_testing.py 15 0 0 0 100% tests/unit/test_client.py 351 0 4 0 100% tests/unit/test_cluster.py 262 0 0 0 100% tests/unit/test_column_family.py 396 0 4 0 100% tests/unit/test_instance.py 319 0 2 0 100% tests/unit/test_row.py 508 0 6 0 100% tests/unit/test_row_data.py 487 0 24 0 100% tests/unit/test_row_filters.py 666 0 4 0 100% tests/unit/test_table.py 441 0 12 0 100% ------------------------------------------------------------------------------------ TOTAL 4572 11 380 7 99%
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
|
I pushed a commit to just fix the lint errors first. But linter output is now unmanageably huge since it's checking inside google.cloud.bigtable._generated. Isn't that blocked by .flake8? |
Sorry, something went wrong.
…or 1) no retry 2) one retry 3) timeout.
…googleapis#4131) * Fix test assertion in test_wrap_method_with_overriding_retry_deadline * Making `test_wrap_method_with_overriding_retry_deadline` deterministic.
Copy link
Copy Markdown
Contributor
Author
nox > Ran multiple sessions: nox > * unit_tests(python_version='2.7'): success nox > * unit_tests(python_version='3.4'): success nox > * unit_tests(python_version='3.5'): success nox > * unit_tests(python_version='3.6'): success nox > * cover: success |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
|
Test for google.cloud.bigtable: nox > * unit_tests(python_version='2.7'): success nox > * unit_tests(python_version='3.4'): success nox > * unit_tests(python_version='3.5'): success nox > * unit_tests(python_version='3.6'): success nox > * system_tests(python_version='2.7'): success nox > * system_tests(python_version='3.6'): success nox > * lint: success nox > * lint_setup_py: success nox > * cover: success |
Sorry, something went wrong.
igorbernstein2
requested changes
Oct 10, 2017
igorbernstein2
left a comment
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
This looks awesome! Just a few minor nits
Sorry, something went wrong.
Comment thread
bigtable/google/cloud/bigtable/table.py
Outdated
| delay_millis = 1000 | ||
| delay_mult = 1.5 | ||
| max_delay_millis = 15 * 1000 | ||
| total_timeout_millis = 5 * 60 * 1000 |
This comment was marked as spam.
This comment was marked as spam.
Comment thread
bigtable/google/cloud/bigtable/table.py
Outdated
| total_timeout_millis = 5 * 60 * 1000 | ||
|
|
||
| now = time.time() | ||
| deadline = now + total_timeout_millis / _MILLIS_PER_SECOND |
This comment was marked as spam.
This comment was marked as spam.
Comment thread
bigtable/google/cloud/bigtable/table.py
Outdated
| index += 1 | ||
|
|
||
| if num_retryable_responses: | ||
| raise _MutateRowsRetryableError() |
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
Comment thread
bigtable/google/cloud/bigtable/table.py
| responses = self.client._data_stub.MutateRows( | ||
| mutate_rows_request) | ||
|
|
||
| num_retryable_responses = 0 |
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
Comment thread
bigtable/google/cloud/bigtable/table.py
Outdated
| for response in responses: | ||
| index = 0 | ||
| for entry in response.entries: | ||
| index = self._next_retryable_row_index(index) |
This comment was marked as spam.
This comment was marked as spam.
* Add '{Bucket,Blob}.user_project' properties, and pass the corresponding
'userProject' query parameter in API requests.
Closes googleapis#3474.
igorbernstein2
requested changes
Oct 12, 2017
Comment thread
bigtable/google/cloud/bigtable/table.py
Outdated
| self.rows[index].clear() | ||
|
|
||
| if retryable_responses: | ||
| raise _MutateRowsRetryableError(retryable_responses) |
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
igorbernstein2
approved these changes
Oct 12, 2017
Comment thread
bigtable/google/cloud/bigtable/table.py
Outdated
| self.rows[index].clear() | ||
|
|
||
| if retryable_responses: | ||
| raise _MutateRowsRetryableError(retryable_responses) |
This comment was marked as spam.
Comment thread
bigtable/google/cloud/bigtable/table.py
| responses = self.client._data_stub.MutateRows( | ||
| mutate_rows_request) | ||
|
|
||
| num_retryable_responses = 0 |
This comment was marked as spam.
…ponses can get overridden by non-retryableresponse.
…or 1) no retry 2) one retry 3) timeout.
…of creating a custom exception. Also update documentation.
…f rows requested.
justinseinlin
requested review from
dhermes,
duggelz,
liyanhui1228,
tswast and
waprin
as code owners
October 25, 2017 20:59
Copy link
Copy Markdown
Contributor
Author
|
Trying to resolve to remove the other commits that are not relevant. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
|
Going to close this PR since I made a mistake in doing a rebase with commits already on the remote. |
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
16 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Retry some mutations that are retryable using similar mechanism as google.gax.retry.retryable.
However, we can't use retryable since it raises a RetryError upon final timeout. We want to return responses list with corresponding status codes for the given rows to be mutated.
PS: Creating a PR first to see if we agree on the implementation and how we want to configure the retry parameters (which are currently set locally since no other methods are using it). Will add test cases in another commit.