RPC retries (second PR) #3324

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!

bigtable/tests/system.py

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


bigtable/google/cloud/bigtable/retry.py

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

bigtable/google/cloud/bigtable/retry.py

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


bigtable/google/cloud/bigtable/retry.py

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


bigtable/google/cloud/bigtable/retry.py

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


bigtable/google/cloud/bigtable/row_data.py


            self._validate_chunk(chunk)

+            if ("ReadRowsIterator" in


bigtable/tests/system.py

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


bigtable/tests/system.py

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


bigtable/tests/system.py

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


bigtable/tests/system.py

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


bigtable/tests/system.py

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


bigtable/tests/unit/test_table.py

+                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' of https://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:master Jul 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():

RPC retries (second PR) #3324

RPC retries (second PR) #3324

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!

lukesneeringer left a comment •

edited

Loading