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

[BEAM-1740] Upgrade to bigtable-client-core 0.9.5.1 #2265

Closed
wants to merge 2 commits into from
Closed

[BEAM-1740] Upgrade to bigtable-client-core 0.9.5.1 #2265

wants to merge 2 commits into from

Conversation

andrewsmartin
Copy link
Contributor

@andrewsmartin andrewsmartin commented Mar 17, 2017

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

@andrewsmartin andrewsmartin changed the title [Beam-1740] Upgrade to bigtable-client-core 0.9.5.1 [BEAM-1740] Upgrade to bigtable-client-core 0.9.5.1 Mar 17, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 70.129% when pulling 3860e39 on andrewsmartin:master into b672cde on apache:master.

@asfbot
Copy link

asfbot commented Mar 17, 2017

@davorbonaci
Copy link
Member

R: @dhalperi (welcome back!)

@dhalperi
Copy link
Contributor

LGTM assuming the tests pass!

@tgroh -- any additional changes regarding shading?

@dhalperi
Copy link
Contributor

@davorbonaci @ssisk @jasonkuster I did not see BigtableITs being run in the precommit where they used to be run. Where are they being run now?

@tgroh
Copy link
Member

tgroh commented Mar 20, 2017

No, this should be fine. We may still not be able to shade the Google Cloud Platform IOs without additional changes as bigtable-client-core still exposes Guava on its API surface in return types (specifically in BigtableDataClient with listenable futures and ImmutableList sampleRowKeys(...)

@tgroh
Copy link
Member

tgroh commented Mar 20, 2017

BigtableITs are run in postcommit.

here's one

@dhalperi
Copy link
Contributor

So it looks like this commit breaks those tests. @andrewsmartin can you please take a look? It might be there there has been a change in the version of some Bigtable dependency (gRPC, netty, etc.) https://builds.apache.org/job/beam_PostCommit_Java_MavenInstall/org.apache.beam$beam-sdks-java-io-google-cloud-platform/2959/testReport/junit/org.apache.beam.sdk.io.gcp.bigtable/BigtableReadIT/testE2EBigtableRead/

@dhalperi
Copy link
Contributor

Btw, apparently @andrewsmartin does not have privileges to run the tests without one of the commiters running it for him.

Andrew, I'm assuming you have access to your own Bigtable cluster (if you use BigtableIO) -- suggest running the IT manually in your local developer environment.

@andrewsmartin
Copy link
Contributor Author

@dhalperi You're right, there was a version conflict with netty. This version of the bigtable client relies on a class that only exists in netty-common:4.1.6.Final. I added another commit to bump the netty version project-wide.

@dhalperi
Copy link
Contributor

I kicked off https://builds.apache.org/view/Beam/job/beam_PostCommit_Java_MavenInstall/2972/ to run the ITs manually. Thanks @andrewsmartin

@coveralls
Copy link

Coverage Status

Coverage remained the same at 70.159% when pulling 54393e1 on andrewsmartin:master into 030528f on apache:master.

@asfbot
Copy link

asfbot commented Mar 21, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8632/
--none--

@asfgit asfgit closed this in 7e97820 Mar 21, 2017
@dhalperi
Copy link
Contributor

Passed! Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants