refactor retry logic on mutate row by aneepct · Pull Request #4665 · googleapis/google-cloud-python

aneepct · 2017-12-27T03:37:13Z

Added retry logic for commit on a DirectRow. The retry uses the retry.py module and performs retry when a Rendezvous exception is thrown due to a disconnect on the channel. The retry will call MutateRow on the original protobuf request with a timeout backoff algorithm provided in retry.py. This has been tested using an endurance test framework that runs for 23 hours and was receiving 8+ exceptions previously. Now all the retries are successful.

sduskis · 2017-12-27T16:31:56Z

Can you please add a description for the pull request?

theacodes

This PR also needs tests.

bigtable/google/cloud/bigtable/row.py

    bigtable_pb2 as messages_v2_pb2)
+from google.api_core import retry
+from google.cloud import exceptions
+from functools import partial, update_wrapper


bigtable/google/cloud/bigtable/row.py

 """The maximum number of mutations that a row can accumulate."""


+def wrapped_partial(func, *args, **kwargs):


bigtable/google/cloud/bigtable/row.py

+                                       self._table,
+                                       request_pb)
+        retry_ = retry.Retry(
+            predicate=retry.if_exception_type(exceptions.GrpcRendezvous),


bigtable/google/cloud/bigtable/row.py

            mutations_list.extend(to_append)


+def call_mutate_row(table, request_pb):


theacodes · 2018-01-16T21:06:42Z

@aneepct this PR has open comments and little activity, do you still intend to merge this?

aneepct · 2018-01-17T05:33:44Z

I am actively resolving all the review questions now and will advance to the state needed for merging. Please also see question from @zakons above.

zakons · 2018-01-18T04:40:28Z

All questions / requests have been responded to. If it is not possible in the current release to get the mapping to exceptions.ServiceUnavailable from the _Rendezvous exception (see comment above), this could be done later when that mapping is available. Can we move forward with the merge?

theacodes

This still needs tests.

bigtable/google/cloud/bigtable/row.py

+                                       self._table,
+                                       request_pb)
+        retry_ = retry.Retry(
+            predicate=retry.if_exception_type(exceptions.GrpcRendezvous),


bigtable/google/cloud/bigtable/row.py

+    return partial_func
+
+
+def call_mutate_row(table, request_pb):


bigtable/google/cloud/bigtable/row.py

            mutations_list.extend(to_append)


+def wrapped_partial(func, *args, **kwargs):


core/google/cloud/exceptions.py

 BadGateway = exceptions.BadGateway
 ServiceUnavailable = exceptions.ServiceUnavailable
 GatewayTimeout = exceptions.GatewayTimeout
+RetryError = exceptions.RetryError


zakons · 2018-01-19T04:26:56Z

Thanks. Almost there. Just not sure how to do the checking for ServiceUnavailable (the exception that we want). Here is the retry logic with the predicate you are recommending:

retry_commit = functools.partial(
    _call_mutate_row, 
     table=self._table, 
     request_pb=request_pb)

retry_ = retry.Retry(
    predicate=retry.if_exception_type(exceptions.from_grpc_error),
    deadline=30)

retry_(retry_commit)()

Where does the checking for exceptions.ServiceUnavailable go?

theacodes · 2018-01-19T17:58:33Z

@zakons you'll need to basically write a custom predicate function:

def _should_retry_commit_exception(exc):
    if isinstance(exc, grpc.RpcError):
        exc = exceptions.from_grpc_error(exc)
    return isinstance(exc, exceptions.ServiceUnavailable)

...

retry_commit = functools.partial(
    _call_mutate_row,
    table=self._table,
    request_pb=request_pb)

retry_ = retry.Retry(
    predicate=_should_retry_commit_exception,
    deadline=30)

retry_(retry_commit)()

aneepct · 2018-01-26T15:24:02Z

@tseaver @jonparrott @sduskis The circleci coverage is now at 100% and our endurance test looks good, can we please have this PR merged? Many thanks for your help.

tseaver · 2018-01-26T15:26:22Z

LGTM if @jonparrott says go.

bigtable/google/cloud/bigtable/row.py


 import struct

+import functools


bigtable/google/cloud/bigtable/row.py


+import functools
 import six
+import grpc


bigtable/google/cloud/bigtable/row.py

    data_pb2 as data_v2_pb2)
 from google.cloud.bigtable._generated import (
    bigtable_pb2 as messages_v2_pb2)
+from google.api_core import retry


bigtable/google/cloud/bigtable/row.py

+    return isinstance(exc, exceptions.ServiceUnavailable)
+
+
+def _call_mutate_row(table, request_pb):


bigtable/google/cloud/bigtable/row.py

-        client = self._table._instance._client
-        client._data_stub.MutateRow(request_pb)
+
+        retry_commit = functools.partial(


bigtable/tests/unit/test_row.py

+
+        expected_result = True
+        result = _check_rendezvous_exception(
+            grpc._channel._Rendezvous(State, None, None, 0))


core/google/cloud/exceptions.py

 BadGateway = exceptions.BadGateway
 ServiceUnavailable = exceptions.ServiceUnavailable
 GatewayTimeout = exceptions.GatewayTimeout
+RetryError = exceptions.RetryError


aneepct · 2018-01-30T05:53:34Z

@jonparrott How do i use RpcError and set the code to UNAVAILABLE?

aneepct · 2018-01-31T05:13:03Z

@jonparrott @zakons OK, we are at 100% coverage and all of your review comments have been incorporated. Please review and if OK, please merge.

zakons · 2018-02-01T13:12:06Z

@jonparrott @tseaver Can we please merge this PR now that all requested changes have been applied? Our endurance test results are all fine as well, meaning that the UNAVAILABLE exception is caught and the commit retried until success.

theacodes · 2018-02-01T23:42:54Z

@zakons @aneepct just a heads up - I'm traveling and will not have a chance to review this until later next week. Apologies for the delay, and thank you for your patience.

theacodes · 2018-02-05T21:34:53Z

Thanks everyone! Sorry for the protracted review process here, I'm glad we ended up with something short and sweet. :)

zakons · 2018-02-05T21:56:09Z

@jonparrott Thank-you! We were impressed with your reviews. In fact, I have been looking at your python testing style guide and am finding it very clear and helpful.

theacodes · 2018-02-05T23:52:17Z

Awesome, I'm glad to be helpful. :)

Refactor retry logic on mutate row

157c762

aneepct requested a review from lukesneeringer as a code owner December 27, 2017 03:37

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

Aneep Tandel added 3 commits December 27, 2017 12:22

Fix long line issue on code

31a561f

fix E501 line too long issue

001ada4

remove retry exception

8470825

zakons mentioned this pull request Dec 28, 2017

Bigtable: how to simplify the retry pattern #4660

Closed

theacodes self-requested a review January 2, 2018 17:00

theacodes suggested changes Jan 2, 2018

View reviewed changes

tseaver added the api: bigtable Issues related to the Bigtable API. label Jan 2, 2018

chemelnucfin assigned theacodes and chemelnucfin Jan 15, 2018

chemelnucfin added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Jan 15, 2018

Done

10761f8

theacodes suggested changes Jan 18, 2018

View reviewed changes

Aneep Tandel added 6 commits January 24, 2018 10:59

Use functools.partial

619b7dc

Refactor retry predicate

3c9c4cd

Add unit test for retry logic

97deb80

Add functool on retry unittest for attributes

9877323

Fix code for circleci

42c2052

Added retry exception unit test

e19bc6a

theacodes suggested changes Jan 26, 2018

View reviewed changes

Test case for Rendevouz exception

0b56b1d

Update to retry commit test case

c6bdd89

Refactore test_retry_commit_exception

437415b

theacodes approved these changes Feb 5, 2018

View reviewed changes

theacodes merged commit a5bb904 into googleapis:master Feb 5, 2018

zakons deleted the feature/refactor_retry_logic_on_mutate_row branch February 5, 2018 21:56

chemelnucfin pushed a commit to chemelnucfin/google-cloud-python that referenced this pull request Feb 8, 2018

Use api_core.retry for mutate_row (googleapis#4665)

9d445f1

theacodes unassigned chemelnucfin Sep 28, 2018

parthea pushed a commit that referenced this pull request Nov 22, 2025

Use api_core.retry for mutate_row (#4665)

48a994b

		"""The maximum number of mutations that a row can accumulate."""


		def wrapped_partial(func, args, *kwargs):

		mutations_list.extend(to_append)


		def call_mutate_row(table, request_pb):

		return isinstance(exc, exceptions.ServiceUnavailable)


		def _call_mutate_row(table, request_pb):

Conversation

aneepct commented Dec 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sduskis commented Dec 27, 2017

Uh oh!

theacodes 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!

theacodes commented Jan 16, 2018

Uh oh!

aneepct commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zakons commented Jan 18, 2018

Uh oh!

theacodes 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!

zakons commented Jan 19, 2018

Uh oh!

theacodes commented Jan 19, 2018

Uh oh!

aneepct commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tseaver commented Jan 26, 2018

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!

aneepct commented Jan 30, 2018

Uh oh!

aneepct commented Jan 31, 2018

Uh oh!

aneepct commented Dec 27, 2017 •

edited

Loading

aneepct commented Jan 17, 2018 •

edited

Loading

aneepct commented Jan 26, 2018 •

edited

Loading