Retry for rows that return retryable error codes. #4116

justinseinlin · 2017-10-03T22:27:29Z

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.

garye

This definitely looks like its on the right track to me. Just a few initial comments...

bigtable/google/cloud/bigtable/table.py

+    RETRY_CODES = (
+        StatusCode.DEADLINE_EXCEEDED.value[0],
+        StatusCode.ABORTED.value[0],
+        StatusCode.INTERNAL.value[0],


bigtable/google/cloud/bigtable/table.py

+        return self._do_mutate_retryable_rows()
+
+    def _is_retryable(self, status):
+        return status is None or status.code in _RetryableMutateRowsWorker.RETRY_CODES


bigtable/google/cloud/bigtable/table.py

+    def __call__(self):
+        return self._do_mutate_retryable_rows()
+
+    def _is_retryable(self, status):


bigtable/google/cloud/bigtable/table.py

+            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)


…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

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%

justinseinlin · 2017-10-05T01:00:29Z

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?

Closes googleapis#4123.

…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.

justinseinlin · 2017-10-06T22:50:24Z

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

…ace.

justinseinlin · 2017-10-07T00:42:38Z

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

Closes googleapis#3956.

igorbernstein2

This looks awesome! Just a few minor nits

bigtable/google/cloud/bigtable/table.py

+        delay_millis = 1000
+        delay_mult = 1.5
+        max_delay_millis = 15 * 1000
+        total_timeout_millis = 5 * 60 * 1000


bigtable/google/cloud/bigtable/table.py

+        total_timeout_millis = 5 * 60 * 1000
+
+        now = time.time()
+        deadline = now + total_timeout_millis / _MILLIS_PER_SECOND


bigtable/google/cloud/bigtable/table.py

+                index += 1
+
+        if num_retryable_responses:
+            raise _MutateRowsRetryableError()


bigtable/google/cloud/bigtable/table.py

+        responses = self.client._data_stub.MutateRows(
+            mutate_rows_request)
+
+        num_retryable_responses = 0


bigtable/google/cloud/bigtable/table.py

+        for response in responses:
+            index = 0
+            for entry in response.entries:
+                index = self._next_retryable_row_index(index)


…s request rows.

* Add '{Bucket,Blob}.user_project' properties, and pass the corresponding 'userProject' query parameter in API requests. Closes googleapis#3474.

bigtable/google/cloud/bigtable/table.py

+                    self.rows[index].clear()
+
+        if retryable_responses:
+            raise _MutateRowsRetryableError(retryable_responses)


bigtable/google/cloud/bigtable/table.py

+                    self.rows[index].clear()
+
+        if retryable_responses:
+            raise _MutateRowsRetryableError(retryable_responses)


bigtable/google/cloud/bigtable/table.py

+        responses = self.client._data_stub.MutateRows(
+            mutate_rows_request)
+
+        num_retryable_responses = 0


…ponses can get overridden by non-retryableresponse.

…or 1) no retry 2) one retry 3) timeout.

…ace.

…s request rows.

…a DEFAULT RETRY.

…of creating a custom exception. Also update documentation.

… coming back.

…f rows requested.

justinseinlin · 2017-10-25T21:42:31Z

Trying to resolve to remove the other commits that are not relevant.

justinseinlin · 2017-10-27T16:40:21Z

Going to close this PR since I made a mistake in doing a rebase with commits already on the remote.
A new PR is created with just the relevant commits cherry-picked on top of master HEAD for api.core -> api_core refactoring: #4256

Retry for rows that return retryable error codes.

cee7db3

justinseinlin requested a review from lukesneeringer as a code owner October 3, 2017 22:27

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 3, 2017

Stop processing once all rows are successful or non-retryable.

1f2e4a7

garye reviewed Oct 3, 2017

View reviewed changes

justinseinlin and others added 2 commits October 3, 2017 17:01

Remove retry for INTERNAL error. Fix the case where has_retryable_res…

ea2e239

…ponses can get overridden by non-retryableresponse.

tseaver previously requested changes Oct 4, 2017

View reviewed changes

Fix lint errors.

98ff97e

tseaver and others added 5 commits October 5, 2017 12:15

Fix pubsub landing page example for 'create_topic'. (googleapis#4124)

400536d

Closes googleapis#4123.

Add tests for google.cloud.bigtable.table.Table.mutate_rows.\nCases f…

7796a50

…or 1) no retry 2) one retry 3) timeout.

Add google.api.core.operations_v1 (googleapis#4081)

3a95612

Fix test assertion in test_wrap_method_with_overriding_retry_deadline (…

3123de6

…googleapis#4131) * Fix test assertion in test_wrap_method_with_overriding_retry_deadline * Making `test_wrap_method_with_overriding_retry_deadline` deterministic.

Add test cases for _RetryableMutateRowsWorker.

2a8f4fc

Refactor test for _RetryableMutateRowsWorker to cover callable interf…

46cdbac

…ace.

Add support for bucket notifications (googleapis#4127)

ce7afe6

Closes googleapis#3956.

igorbernstein2 suggested changes Oct 10, 2017

View reviewed changes

justinseinlin and others added 5 commits October 11, 2017 15:16

Remove assumption that responses come back in the same linear order a…

a4f7643

…s request rows.

Add scope for trace client (googleapis#4158)

318b04f

Include retryable responses in the exception raised.

7142119

Assign default value for init argument.

4c077bf

Re-enable 'Bucket.requester_pays' feature (googleapis#4056)

aeb4cd4

* Add '{Bucket,Blob}.user_project' properties, and pass the corresponding 'userProject' query parameter in API requests. Closes googleapis#3474.

igorbernstein2 suggested changes Oct 12, 2017

View reviewed changes

Jon Wayne Parrott and others added 3 commits October 12, 2017 10:11

Add google.api.core.helpers.general_helpers.wraps (googleapis#4166)

99c4498

Fix Spanner coverage reporting. (googleapis#4169)

598ab72

Fix usage documentation. (googleapis#4168)

e1a2483

igorbernstein2 approved these changes Oct 12, 2017

View reviewed changes

Indy2222 and others added 19 commits October 25, 2017 11:13

Fix parsing of API errors with Unicode err message (googleapis#4251)

a51b202

Port gax proto helper methods (googleapis#4249)

877ef4e

Retry for rows that return retryable error codes.

31a639d

Stop processing once all rows are successful or non-retryable.

b514051

Remove retry for INTERNAL error. Fix the case where has_retryable_res…

e3e7163

…ponses can get overridden by non-retryableresponse.

Fix lint errors.

2eaa9b5

Add tests for google.cloud.bigtable.table.Table.mutate_rows.\nCases f…

22d2feb

…or 1) no retry 2) one retry 3) timeout.

Add test cases for _RetryableMutateRowsWorker.

873235e

Refactor test for _RetryableMutateRowsWorker to cover callable interf…

bd82a16

…ace.

Remove assumption that responses come back in the same linear order a…

7d6e3c9

…s request rows.

Include retryable responses in the exception raised.

7d34a06

Assign default value for init argument.

ad49d6f

Change _MILLIS_PER_SECOND to float for better deadline precision.

3269f18

Use google.api.core.retry.Retry. Allow user-configurable retry-options.

c1c7b17

Revert retry.py. Modify mutate_rows api to take a Retry instead with …

abc7895

…a DEFAULT RETRY.

Use google.api.core.exceptions for internal retryable worker instead …

4ade5fa

…of creating a custom exception. Also update documentation.

Add sanity check for rows requested to mutate and number of responses…

0d86021

… coming back.

Add test for assertion check to ensure num of responses matches num o…

517c8e1

…f rows requested.

Sync rebased local branch and remote branch.

faf90a6

justinseinlin requested review from dhermes, duggelz, liyanhui1228, tswast and waprin as code owners October 25, 2017 20:59

Refactor google.api.core to google.api_core.

d6f7532

justinseinlin mentioned this pull request Oct 25, 2017

Retry for 'mutate_rows' that return retryable errors. #4256

Merged

justinseinlin closed this Oct 27, 2017

justinseinlin deleted the mutate-rows-retry branch November 7, 2017 19:17

Retry for rows that return retryable error codes. #4116

Retry for rows that return retryable error codes. #4116

Uh oh!

Conversation

justinseinlin commented Oct 3, 2017

Uh oh!

garye left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

tseaver left a comment

Choose a reason for hiding this comment

Uh oh!

justinseinlin commented Oct 5, 2017

Uh oh!

justinseinlin commented Oct 6, 2017

Uh oh!

justinseinlin commented Oct 7, 2017

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

justinseinlin commented Oct 25, 2017

Uh oh!

justinseinlin commented Oct 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone