RPC retries (second PR) by calpeyser · Pull Request #3324 · googleapis/google-cloud-python

calpeyser · 2017-04-24T21:57:16Z

Since the repo's layout has changed significantly since the last PR, I've opened a new PR. I've attempted to address the comments from the last go-around - please have a look.

googlebot · 2017-04-24T21:57:22Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2017-04-26T14:18:26Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2017-04-26T17:35:06Z

CLAs look good, thanks!

dhermes · 2017-04-27T17:27:10Z

@calpeyser The cover environment is also failing because several lines do not have tests exercising them / some branches may not have all paths covered:


google/cloud/bigtable/retry.py: 60-61, 78-120, 123, 126, 175, 45->exit, 172->175
google/cloud/bigtable/row_data.py: 279, 277->279

garye

Looks like test coverage is the other thing to fix here as @dhermes points out. Looking good so far!

Sign in to view

+ table = instance.table("table")
+
+ # Run test, line by line
+ script = open(TEST_SCRIPT, 'r')


Sign in to view

+ Sets the row key at which this iterator will begin reading.
+ """
+ self.start_key = start_key
+ self.start_key_closed = False


…e-cloud-python

tseaver · 2017-05-15T17:44:20Z

Supersedes #3279.

garye · 2017-05-16T14:19:19Z

Hi @dhermes, can you help take a look at this?

lukesneeringer

Hi @calpeyser, thanks for submitting.

I have a few concerns about this as given. The principle of retrying when we know that we can seems fine, but I would ask that you do some cleanup before we accept this.

Thank you very much! :-)

Sign in to view

+_MILLIS_PER_SECOND = 1000
+
+
+class ReadRowsIterator():


Sign in to view

+ if code not in self.retry_options.retry_codes:
+ six.reraise(type(error), error)
+
+ # pylint: disable=redefined-variable-type


Sign in to view

+ deadline - now)
+ self.set_stream()
+
+ six.reraise(errors.RetryError, exc)


Sign in to view


 self._validate_chunk(chunk)

+ if ("ReadRowsIterator" in


Sign in to view

+
+ test_platform = platform.system()
+ if (test_platform not in MOCK_SERVER_URLS):
+ self.fail("Retry server not available for platform " + test_platform)


Sign in to view

+ table = instance.table("table")
+
+ # Run test, line by line
+ script = open(TEST_SCRIPT, 'r')


Sign in to view

+ if line.startswith("CLIENT:"):
+ chunks = line.split(" ")
+ op = chunks[1]
+ if (op != "SCAN"):


Sign in to view

+ process_scan(table, chunks[2], chunks[3])
+
+ # Clean up
+ server.kill()


Sign in to view

+ if (op != "SCAN"):
+ self.fail("Script contained " + op + " operation. Only \'SCAN\' is supported.")
+ else:
+ process_scan(table, chunks[2], chunks[3])


Sign in to view

+ return StatusCode.DEADLINE_EXCEEDED
+
+ def _wait_then_raise():
+ time.sleep(0.5)


calpeyser · 2017-06-09T21:45:44Z

@lukesneeringer @garye - I've attempted to address the comments above. Thanks for bearing with me!

mbrukman · 2017-06-29T15:35:01Z

@calpeyser -- looks like you have some conflicts with the base branch, please resolve them.

@lukesneeringer, @garye -- please take a look at the PR, it would be great to merge this functionality soon.

Thanks everyone!

garye · 2017-07-14T14:45:40Z

LGTM from a bigtable perspective

calpeyser · 2017-07-14T15:53:27Z

Conflict resolved, tests are passing. @lukesneeringer, what do you think?

lukesneeringer · 2017-07-17T02:50:17Z

Reading now

lukesneeringer

This seems fine to me at this point.

calpeyser · 2017-07-17T14:28:40Z

@lukesneeringer - can you merge it? Neither I nor @garye have write permissions.

lukesneeringer · 2017-07-17T15:37:41Z

🎉

mbrukman · 2017-07-17T17:27:14Z

Thank you @calpeyser for your work on this and thanks to @garye and @lukesneeringer for the reviews!

dhermes · 2017-07-20T16:24:28Z

This caused a system test breakage, I'm very tempted to roll it back and iterate in a PR / branch until the system tests are passing

This reverts commit 67f4ba4.

This reverts commit 0213b69cdf0177d9158d7608633b4d9c66930e03.

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Apr 24, 2017

implement retries for read_rows
2d26f4f

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Apr 26, 2017

fix linter errors
d25cafa

garye suggested changes May 2, 2017
View reviewed changes

calpeyser added 7 commits May 2, 2017 15:40

Merge branch 'master' ofhttps://github.com/GoogleCloudPlatform/googl…
7facbb4
…e-cloud-python

add some tests for read_rows retries
5a8fdc0

add move test coverage for read_rows retries
537e8b6

correct import for _create_row_request in test_table.py
bddf673

add test coverage for start_key_closed in read_rows
9688b3c

remove redundant ReadRowsIterator#__iter__
c4bad6e

add newline in retry.py for linter
68ef3af

tseaver added api: bigtable Issues related to the Bigtable API. in progress labels May 15, 2017

tseaver mentioned this pull request May 15, 2017
Retry test #3279
Closed

tseaver added the priority: p2 Moderately-important priority. Fix may not be included in next release. label May 15, 2017

lukesneeringer suggested changes May 16, 2017
View reviewed changes

calpeyser added 2 commits June 9, 2017 17:29

check test outcome
d085312

address comments
70d920f

calpeyser added 2 commits July 14, 2017 09:46

Merge branch 'master' into master
88e52fd

remove extra import
7cffbe0

adding newline in bigtable/table.py to satisfy linter
19079c3

lukesneeringer approved these changes Jul 17, 2017
View reviewed changes

lukesneeringer merged commit 67f4ba4 into googleapis:masterJul 17, 2017

theacodes pushed a commit that referenced this pull request Jul 20, 2017
Revert "RPC retries (second PR) (#3324)"
e6e0b1c
This reverts commit 67f4ba4.

theacodes pushed a commit that referenced this pull request Jul 20, 2017
Revert "RPC retries (second PR) (#3324)" (#3642)
419588c
This reverts commit 67f4ba4.

dhermes mentioned this pull request Jul 26, 2017
Revert "RPC retries (second PR) (#3324)" #3642
Merged

mbrukman mentioned this pull request Aug 11, 2017
Retry temporary failures for Bigtable scans #2255
Closed

This was referenced Aug 14, 2017
Adding RPC retries to Bigtable. #3808
Closed
Pr 3324 to fix #3810
Closed
RPC retries (resubmission) #3811
Merged

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 21, 2017
RPC retries (second PR) (googleapis#3324)
12360f5

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 21, 2017
Revert "RPC retries (second PR) (googleapis#3324)" (googleapis#3642)
e173504
This reverts commit 67f4ba4.

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
RPC retries (second PR) (googleapis#3324)
1016b28

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
Revert "RPC retries (second PR) (googleapis#3324)" (googleapis#3642)
68233c6
This reverts commit 67f4ba4.

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
RPC retries (second PR) (googleapis#3324)
9ac36fb

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
Revert "RPC retries (second PR) (googleapis#3324)" (googleapis#3642)
dca982c
This reverts commit 67f4ba4.

parthea pushed a commit that referenced this pull request Nov 22, 2025
RPC retries (second PR) (#3324)
208d97c

parthea pushed a commit that referenced this pull request Nov 22, 2025
Revert "RPC retries (second PR) (#3324)" (#3642)
350984b
This reverts commit 0213b69cdf0177d9158d7608633b4d9c66930e03.

		_MILLIS_PER_SECOND = 1000


		class ReadRowsIterator():

Uh oh!

Conversation

calpeyser commented Apr 24, 2017

Uh oh!

googlebot commented Apr 24, 2017

Uh oh!

googlebot commented Apr 26, 2017

Uh oh!

googlebot commented Apr 26, 2017

Uh oh!

dhermes commented Apr 27, 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!

tseaver commented May 15, 2017

Uh oh!

garye commented May 16, 2017

Uh oh!

lukesneeringer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

calpeyser commented Jun 9, 2017

Uh oh!

mbrukman commented Jun 29, 2017

Uh oh!

garye commented Jul 14, 2017

Uh oh!

calpeyser commented Jul 14, 2017

Uh oh!

lukesneeringer commented Jul 17, 2017

Uh oh!

lukesneeringer left a comment

Choose a reason for hiding this comment

Uh oh!

calpeyser commented Jul 17, 2017

Uh oh!

lukesneeringer commented Jul 17, 2017

Uh oh!

mbrukman commented Jul 17, 2017

Uh oh!

lukesneeringer left a comment •
edited
Loading