Skip to content

RPC retries (resubmission) - #3811

Merged
tseaver merged 7 commits into
googleapis:masterfrom
calpeyser:fix-retry
Nov 15, 2017
Merged

RPC retries (resubmission)#3811
tseaver merged 7 commits into
googleapis:masterfrom
calpeyser:fix-retry

Conversation

@calpeyser

@calpeysercalpeyser commented Aug 14, 2017

Copy link
Copy Markdown
Contributor

Re-submission for PR #3324.

@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 14, 2017
@calpeysercalpeyser changed the title Fix retryRPC retries (resubmission)Aug 14, 2017
@calpeyser

calpeyser commented Aug 14, 2017

Copy link
Copy Markdown
ContributorAuthor

@dhermes please take a look. Is there anything I should do outside of regular CI to run the tests that failed the last time around?

@dhermes

dhermes commented Aug 14, 2017

Copy link
Copy Markdown
Contributor

@calpeyser Sending a PR from your remote will skip the system tests, which is partly why I kept the change around in a branch on the GoogleCloudPlatform remote.

So before we merge this, we should push this branch to the GoogleCloudPlatform remote so the system tests can be run. (FWIW, I have yet to review, though someone else can sign off, I'm not the only one with authority here.)

dhermes
dhermes reviewed Aug 14, 2017
View reviewed changes
Comment threadbigtable/google/cloud/bigtable/table.py
"""
request_pb = _create_row_request(
self.name, start_key=start_key, end_key=end_key, filter_=filter_,
limit=limit, end_inclusive=end_inclusive)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@calpeyser
calpeyser changed the base branch from master to pr-3324-to-fixAugust 15, 2017 16:38
@calpeyser

calpeyser commented Aug 15, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@dhermes PTAL. Would it be easier to PR against an empty branch, so as not to deal with the backlog of commits? I don't seem to have the privileges to rebase this branch from master.

Looking at the test logs, it seems that the system tests are still being skipped.

@googlebot

googlebot commented Aug 15, 2017

Copy link
Copy Markdown

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.

@googlebotgooglebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 15, 2017
@googlebot

googlebot commented Aug 15, 2017

Copy link
Copy Markdown

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.

@dhermes
dhermes changed the base branch from pr-3324-to-fix to masterAugust 15, 2017 17:41
@dhermes

dhermes commented Aug 15, 2017

Copy link
Copy Markdown
Contributor

@calpeyser I changed the base back to master. I'll just manually push your branch to the other remote so CI can run in parallel with this.

@dhermes

dhermes commented Aug 15, 2017

Copy link
Copy Markdown
Contributor

@calpeyser Here is the build and the branch:

https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/2901
https://github.com/GoogleCloudPlatform/google-cloud-python/tree/pr-3811

@mbrukman

mbrukman commented Aug 15, 2017

Copy link
Copy Markdown
Contributor

@calpeyser – looks like the build/test failed on CircleCI; could you take a look, please?

@googlebot

googlebot commented Aug 16, 2017

Copy link
Copy Markdown

CLAs look good, thanks!

@googlebotgooglebot 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 Aug 16, 2017
@calpeyser

calpeyser commented Aug 16, 2017

Copy link
Copy Markdown
ContributorAuthor

@dhermes can you patch the change "encode -> decode for bytestring in bigtable system test" into the branch on the remote? The system tests don't seem to run here.

The current failure is in "Update the docs", and seems unrelated to this change.

@calpeyser

calpeyser commented Aug 21, 2017

Copy link
Copy Markdown
ContributorAuthor

@dhermes ping on pushing the lastest commit to the remote branch so we can run the system tests.

@dhermes

dhermes commented Aug 21, 2017

Copy link
Copy Markdown
Contributor

Sorry, doing it now!

@dhermes

dhermes commented Aug 21, 2017

Copy link
Copy Markdown
Contributor

(Email is a crappy task list, my bad.)

@dhermes

dhermes commented Aug 21, 2017

Copy link
Copy Markdown
Contributor

New build: https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/2951
Non-system-test red build: https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/2917

@calpeyser I see the docs breakage in the last build, you should probably rebase against master due to some moves / nox config changes?

@dhermes

dhermes commented Aug 21, 2017

Copy link
Copy Markdown
Contributor

@calpeyser The system tests failed in the build. Do they pass when you run them locally? If you haven't run them locally, is there anything I can do to help you get them running?

$ pip install --upgrade nox-automation
$ cd /path/to/your/git-checkout/of/google-cloud-python/
$ cd bigtable/
$ nox -s "system_tests(python_version='2.7')"
$ nox -s "system_tests(python_version='3.6')"

@calpeyser

calpeyser commented Oct 22, 2017

Copy link
Copy Markdown
ContributorAuthor

@garye what do you think?

@garye

garye commented Oct 31, 2017

Copy link
Copy Markdown
Contributor

LGTM! Just need @dhermes, @lukesneeringer or someone from the team to pull the trigger I think.

@googlebot

googlebot commented Nov 10, 2017

Copy link
Copy Markdown

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 all authors are ok 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.

@googlebotgooglebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Nov 10, 2017
@tseavertseaver 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 Nov 10, 2017
@googlebot

googlebot commented Nov 14, 2017

Copy link
Copy Markdown

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 all authors are ok 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.

@googlebotgooglebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Nov 14, 2017
@calpeyser

calpeyser commented Nov 14, 2017

Copy link
Copy Markdown
ContributorAuthor

@tseaver - I've fixed the issues with the merge above - can this PR be merged?

@googlebot

googlebot commented Nov 14, 2017

Copy link
Copy Markdown

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 all authors are ok 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.

@tseaver
tseaver merged commit 5a0e549 into googleapis:masterNov 15, 2017
@dhermes

dhermes commented Nov 15, 2017

Copy link
Copy Markdown
Contributor

https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4249

I think this may need to be reverted

@dhermes

dhermes commented Nov 16, 2017

Copy link
Copy Markdown
Contributor

@calpeyser@garye Here is another (albeit less severe) failure:

https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4256

There have been no other builds on master since the previous failure.

It's worth noting that we have other packages with tests that are flaky (which we tolerate mostly due to lack of developer resources), but this one seems to be a preventable flakiness.

@dhermes

dhermes commented Nov 16, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

I'm just going to collect a list of broken builds here (for debugging purposes):

  1. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4249
  2. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4256
  3. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4262
  4. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4277
  5. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4278
  6. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4290
  7. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4323
  8. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4330
  9. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4334
  10. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4337
  11. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4338
  12. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4349
  13. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4361
  14. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4399
  15. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4404
  16. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4407
  17. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4408
  18. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4413
  19. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4417
  20. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4419
  21. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4421
  22. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4423
  23. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4432
  24. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4439
  25. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4452
  26. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4458
  27. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4459
  28. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4468
  29. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4470
  30. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4472
  31. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4474
  32. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4478
  33. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4501
  34. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4506
  35. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4508
  36. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4513
  37. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4517
  38. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4524
  39. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4526
  40. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4527
  41. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4528
  42. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4529
  43. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4535
  44. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4536
  45. https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/4540

I may edit it as more occur / if more occur.

@dhermes

dhermes commented Nov 21, 2017

Copy link
Copy Markdown
Contributor

@calpeyser@garye Please weigh in here. I am leaning towards reverting this PR (we need our builds to not fail).

@igorbernstein2

igorbernstein2 commented Nov 21, 2017

Copy link
Copy Markdown
Contributor

I can try to fix this tomorrow. After a quick look, I think that the issue is that the test_retry test never cleans up the BIGTABLE_EMULATOR_HOST env var, causing the rest of the tests to connect to a dead port

@dhermes

dhermes commented Nov 21, 2017

Copy link
Copy Markdown
Contributor

Thanks @igorbernstein2. I'm happy to help (e.g. if you're unfamiliar with unittest.mock.patch).

@calpeyser

calpeyser commented Nov 22, 2017

Copy link
Copy Markdown
ContributorAuthor

This breakage certainly looks related to this change. @igorbernstein2, thanks a ton - I think you're right about the env var.

dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Dec 4, 2017
@dhermes
Revert "Add RPC retries to Bigtable (googleapis#3811)"
e521fd4
This reverts commit 5a0e549 / PR googleapis#3811.
This was referenced Dec 4, 2017
Merged
Closed
dhermes added a commit that referenced this pull request Dec 4, 2017
@dhermes
Revert "Add RPC retries to Bigtable (#3811)"
bd023fe
This reverts commit 5a0e549 / PR #3811.
dhermes added a commit that referenced this pull request Dec 4, 2017
@dhermes
Revert "Add RPC retries to Bigtable (#3811)" (#4524)
dfe8a41
This reverts commit 5a0e549 / PR #3811.
parthea pushed a commit that referenced this pull request Nov 22, 2025
@calpeyser@tseaver
Add RPC retries to Bigtable (#3811)
8a8c305
parthea pushed a commit that referenced this pull request Nov 22, 2025
@dhermes
Revert "Add RPC retries to Bigtable (#3811)" (#4524)
f425409
This reverts commit 1c699f36584d8c507118dae9ab8a2a31bc951ce6 / PR #3811.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@duggelzduggelzAwaiting requested review from duggelz
@theacodestheacodesAwaiting requested review from theacodes
@liyanhui1228liyanhui1228Awaiting requested review from liyanhui1228
@lukesneeringerlukesneeringerAwaiting requested review from lukesneeringer
@tseavertseaverAwaiting requested review from tseaver
@tswasttswastAwaiting requested review from tswast
@waprinwaprinAwaiting requested review from waprin
4 more reviewers
@dhermesdhermesdhermes left review comments
@justinseinlinjustinseinlinjustinseinlin left review comments
@garyegaryegarye requested changes
@mbrukmanmbrukmanmbrukman requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: bigtableIssues related to the Bigtable API.cla: noThis human has *not* signed the Contributor License Agreement.type: feature request‘Nice-to-have’ improvement, new feature or different behavior or design.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@calpeyser@dhermes@googlebot@mbrukman@igorbernstein2@garye@justinseinlin@tseaver