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.

Sign in to view

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


Sign in to view

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


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


Sign in to view

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


Sign in to view

 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.

Sign in to view

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


Sign in to view

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


Sign in to view

 mutations_list.extend(to_append)


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


Sign in to view

 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):
ifisinstance(exc, grpc.RpcError):
exc=exceptions.from_grpc_error(exc)
returnisinstance(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.

Sign in to view


 import struct

+import functools


Sign in to view


+import functools
 import six
+import grpc


Sign in to view

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


Sign in to view

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


Sign in to view

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


Sign in to view

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


Sign in to view

 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:masterFeb 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
Useapi_core.retryformutate_row(googleapis#4665)
9d445f1

theacodes unassigned chemelnucfin Sep 28, 2018

parthea pushed a commit that referenced this pull request Nov 22, 2025
Useapi_core.retryformutate_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):

Uh oh!

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

aneepct commented Dec 27, 2017•
edited
Loading

aneepct commented Jan 17, 2018•
edited
Loading

aneepct commented Jan 26, 2018•
edited
Loading