Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "RPC retries (second PR) (#3324)" #3642

Merged
merged 1 commit into from
Jul 20, 2017
Merged

Revert "RPC retries (second PR) (#3324)" #3642

merged 1 commit into from
Jul 20, 2017

Conversation

theacodes
Copy link
Contributor

This reverts commit 67f4ba4.

This is due to system test failures

We're happy to reconsider the PR that we reverted, but it must not break the build.

@theacodes theacodes added api: bigtable Issues related to the Bigtable API. priority: p0 Highest priority. Critical issue. P0 implies highest priority. labels Jul 20, 2017
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 20, 2017
Copy link
Contributor

@dhermes dhermes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dhermes
Copy link
Contributor

dhermes commented Jul 20, 2017

@jonparrott It's probably easier if we make a branch with the "broken" code already in, and then @calpeyser or someone else can send a PR to fix that branch?

@theacodes
Copy link
Contributor Author

@dhermes you / they can do that by starting a new branch from master and cherry-picking 67f4ba4:

git checkout master
git pull
git checkout -b new-pr-for-retries
git cherry-pick 67f4ba4

@dhermes
Copy link
Contributor

dhermes commented Jul 20, 2017

@jonparrott I'm OK with just the current master: 14e570a

Why get fancy?

@dhermes
Copy link
Contributor

dhermes commented Jul 20, 2017

@dhermes
Copy link
Contributor

dhermes commented Jul 20, 2017

CircleCI is green, let's merge this puppy?

@theacodes
Copy link
Contributor Author

Why get fancy?

cherry-picking places the changes on top of the current master instead of being based on the past (which will be important when we merge the operation future stuff)

@theacodes theacodes merged commit 419588c into master Jul 20, 2017
@theacodes theacodes deleted the revert-3324 branch July 20, 2017 17:07
@dhermes
Copy link
Contributor

dhermes commented Jul 20, 2017

OK, on it.

@dhermes
Copy link
Contributor

dhermes commented Jul 20, 2017

Done. After I cherry-picked I git commit --amend-ed so the commit message ("Adding RPC retries to Bigtable.") was a bit more useful / didn't reference a PR ID.

@theacodes
Copy link
Contributor Author

OK, on it.

Don't do it until #3623 is merged

@dhermes
Copy link
Contributor

dhermes commented Jul 20, 2017

I shall garden away on the pr-3324-to-fix branch

@dhermes
Copy link
Contributor

dhermes commented Jul 20, 2017

Rebase went smoothly 👍

@dhermes
Copy link
Contributor

dhermes commented Jul 26, 2017

@calpeyser @garye @mbrukman FYI #3324 has been reverted and the contents (which need fixing, e.g. because they broken the Bigtable system tests) are in the pr-3324-to-fix branch

@mbrukman
Copy link
Contributor

Thanks for the heads up, @dhermes! For our future reference, how does one run all the tests that Circle CI will run after submit? Is it just running nox at the top of tree, or in the bigtable dir for a single service?

@tseaver
Copy link
Contributor

tseaver commented Jul 26, 2017

@mbrukman one needs to run nox with the appropriate environment variables set to configure system account credentials (GOOGLE_APPLICATION_CREDENTIALS, GOOGLE_CLOUD_TESTS_PROJECT_ID, GOOGLE_CLOUD_PROJECT). E.g.:

$ . /path/to/configure_my_credentials.sh
$ cd /path/to/google-cloud-python/bigtable
$ nox

@theacodes
Copy link
Contributor Author

FYI: we're working on a more comprehensive retry story for this library.

@mbrukman
Copy link
Contributor

@dhermes — were you planning to address the build issues in the pr-3324-to-fix branch, or are you looking to us to fix it?

@jonparrott — will that work on a more comprehensive retry story remove the need for this change for Bigtable, or should we still re-submit this change anyway, and that will be a more generic change that we might incorporate into the Bigtable library at some point in the future?

@dhermes
Copy link
Contributor

dhermes commented Jul 28, 2017

were you planning to address the build issues in the pr-3324-to-fix branch

Sorry for miscommunicating here. I unfortunately do not have cycles for this.

@theacodes
Copy link
Contributor Author

will that work on a more comprehensive retry story remove the need for this change for Bigtable

My hope is that the work will provide the foundation for adding retries to all libraries, at which point we can revisit this change. I'd love to work with you to validate the new functionality and get it into BigTable.

@garye
Copy link
Contributor

garye commented Jul 28, 2017

@jonparrott That's an exciting development. Please keep us looped in!

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 21, 2017
landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement. priority: p0 Highest priority. Critical issue. P0 implies highest priority.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants