[Bigtable] Migrate BigtableIO to use the veneer client under the hood by mutianf · Pull Request #24015 · apache/beam

mutianf · 2022-11-07T19:04:46Z

Migrate BigtableIO to use the java veneer client under the hood. In the future, we'll move to java-bigtable-hbase 2.x versions and deprecate bigtable-client-core.

The new code has the following structure:

BigatableConfig - defines the client connection level settings
BigtableReadOptions and BigtableWriteOptions - defines the tables to read / write and their timeout settings
deprecate BigtableOptions

BigtableIO.Read or BigtableIO.Write with the same configuration will share the same BigtableService. The service is cached in BigtableServiceFactory. Whenever we instantiating a new BigtableService, translate all the configurations including BigtableOptions to bigtable veneer settings.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

codecov · 2022-12-01T16:20:48Z

Codecov Report

Merging #24015 (ce231c4) into master (2e7584c) will increase coverage by 0.02%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #24015 +/- ##
==========================================
+ Coverage 72.79% 72.81% +0.02% 
==========================================
Files 775 775 Lines 102840 102928 +88 ==========================================
+ Hits 74864 74949 +85 - Misses 26522 26525 +3 
Partials 1454 1454

Flag	Coverage Δ
python	`81.96% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
sdks/python/apache_beam/io/textio.py	`94.59% <0.00%> (-2.47%)`	⬇️
sdks/python/apache_beam/coders/row_coder.py	`94.26% <0.00%> (-0.70%)`	⬇️
sdks/python/apache_beam/coders/coders.py	`87.04% <0.00%> (-0.44%)`	⬇️
sdks/python/apache_beam/transforms/combiners.py	`93.05% <0.00%> (-0.39%)`	⬇️
sdks/python/apache_beam/coders/coder_impl.py	`93.53% <0.00%> (-0.24%)`	⬇️
...hon/apache_beam/runners/worker/bundle_processor.py	`94.22% <0.00%> (-0.12%)`	⬇️
sdks/python/apache_beam/io/kafka.py	`80.00% <0.00%> (ø)`
sdks/python/apache_beam/io/fileio.py	`96.12% <0.00%> (+0.01%)`	⬆️
...on/apache_beam/runners/dataflow/dataflow_runner.py	`81.88% <0.00%> (+0.14%)`	⬆️
sdks/python/apache_beam/coders/slow_stream.py	`94.87% <0.00%> (+0.18%)`	⬆️
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

github-actions · 2022-12-02T18:35:43Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @kennknowles for label java.
R: @Abacn for label build.
R: @Abacn for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

diegomez17

Reviewed BigtableServiceImpl and it's tests.

github-actions · 2022-12-20T12:14:52Z

Reminder, please take a look at this pr: @kennknowles @Abacn @Abacn

Abacn · 2022-12-21T15:58:58Z

Thanks for the work! Looking into it.

Abacn

Left some initial comments. Not yet finished (now at BigtableHBaseVeneeringSettings)

github-actions · 2023-01-06T12:13:50Z

Reminder, please take a look at this pr: @kennknowles @Abacn @Abacn

Abacn · 2023-01-06T14:42:28Z

waiting on author

mutianf · 2023-02-21T19:23:23Z

Hi @Abacn , this pr is ready for review, can you take a look? @igorbernstein2 from my team already took a look and thinks it looks good. Thanks!

Abacn · 2023-02-21T19:47:46Z

Thanks @mutianf, will looking into it tomorrow

Abacn

Thanks, this looks good to me. Would you mind open a GitHub Issue for this PR for tracking, and adding an announcement in CHANGES.md ?https://github.com/apache/beam/blob/master/CHANGES.md?plain=1#L70

Abacn · 2023-02-22T15:49:32Z

 }

- /** Tests that credentials are used from PipelineOptions if not supplied by BigtableOptions. */
- @Test


Just wondering the purpose of removing these unit tests, is it because the scenario is no longer applicable for the new client, or are there any breaking change / regression could be introduced?

Good catch. The way we get BigtableService is different now so these tests are no longer applicable. I moved the tests to BigtableConfigTranslator which will test the same behavior, PTAL, thanks! 97e96e8#diff-b53a99acd5a4cd30f4b76e8b94f7cd09df6f0fc196d949842d20bc845b48661c

Abacn

Thanks for the prompt fix. Reviewed tests. If the change is not trivial good to have other reviewes to take another look for main source change.

Abacn · 2023-02-23T14:35:38Z

- CredentialFactory credentialFactory = config.getCredentialFactory();
- try {
+ // Skip resetting the credentials if it's connected to an emulator
+ if (!emulator) {


Here the code path is made different intentionally for test and real use cases. Does this cause the if {} clause no longer covered by unit test where it was intended to be?

This is still covered by testUsingPipelineOptionsCredential and testUsingCredentialsFromBigtableOptions in BigtableConfigTranslatorTest to make sure the credentials are updated correctly. The emulator test case is tested by the sql tests.

Abacn · 2023-02-23T14:38:58Z

 * Support for X source added (Java/Python) ([#X](https://github.com/apache/beam/issues/X)).
 * Added in JmsIO a retry policy for failed publications (Java) ([#24971](https://github.com/apache/beam/issues/24971)).
 * Support for `LZMA` compression/decompression of text files added to the Python SDK ([#25316](https://github.com/apache/beam/issues/25316))
+* Update BigtableIO to use the idiomatic bigtable client under the hood (Java) ([25592](https://github.com/apache/beam/issues/25592))


Unfortunately this won't be in Beam 2.46.0. We can remove it here and insert to the proper line again as a follow up PR after this is in.

mutianf · 2023-02-23T15:52:15Z

Thanks for the prompt fix. Reviewed tests. If the change is not trivial good to have other reviewes to take another look for main source change.

Sounds good, thanks @Abacn! I'll have Igor review it again after he's back from vacation next week.

igorbernstein2 · 2023-03-02T15:20:16Z

- for (SampleRowKeysResponse response : sampleRowKeys) {
- ByteKey responseEndKey = makeByteKey(response.getRowKey());
- long responseOffset = response.getOffsetBytes();
+ for (KeyOffset keyOffset : sampleRowKeys) {


In a followup PR, we should migrate the splitting logic to the utils in veneer (also we should fix the logic in veneer to use split points as inclusive endpoints...it currently uses the split points as inclusive start points)

igorbernstein2 · 2023-03-02T15:28:30Z

+ }
 }

 @ProcessElement


This DoFn has a bunch of additional logic for checking for failures, in a future PR we should just use the error tracking veneer's batching logic

Abacn · 2023-03-02T19:48:04Z

Run PostCommit_Java_DataflowV2

Abacn · 2023-03-02T19:48:10Z

Run PostCommit_Java_Dataflow

Abacn · 2023-03-02T22:29:55Z

Run Java PreCommit

Abacn · 2023-03-02T23:46:29Z

postcommit tests passed. Two failing precommit suites due to known flakes. Merging for now.

…apache#24015)

github-actionsBot added build extensions gcp io java sql labels Nov 7, 2022

mutianf changed the title ~~Migration~~Bigtable: migrate BigtableIO to use the veneer client under the hoodDec 1, 2022

mutianf force-pushed the migration branch from 1602239 to cacba60Compare December 1, 2022 15:41

mutianf changed the title ~~Bigtable: migrate BigtableIO to use the veneer client under the hood~~[Bigtable] Migrate BigtableIO to use the veneer client under the hoodDec 2, 2022

chore: migrate BigtableIO to use Veneer
597523f

mutianf force-pushed the migration branch from f91b14f to 597523fCompare December 2, 2022 18:04

mutianf marked this pull request as ready for review December 2, 2022 18:05

github-actionsBot added the Next Action: Reviewers label Dec 2, 2022

diegomez17 reviewed Dec 8, 2022
View reviewed changes

address comments
5b27c90

github-actionsBot added the slow-review label Dec 20, 2022

github-actionsBot removed the slow-review label Dec 21, 2022

Abacn reviewed Dec 22, 2022
View reviewed changes

Comment thread...-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableClientWrapper.java

Abacn reviewed Dec 23, 2022
View reviewed changes

Comment thread.../google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableConfig.java Outdated
Comment thread...atform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableHBaseVeneeringSettings.java Outdated

github-actionsBot added the slow-review label Jan 6, 2023

github-actionsBot added Next Action: Author and removed Next Action: Reviewers slow-review labels Jan 6, 2023

refactor setting configurations for veneer
bb579f6

igorbernstein2 approved these changes Feb 21, 2023
View reviewed changes

Abacn approved these changes Feb 22, 2023
View reviewed changes

mutianf added 4 commits February 22, 2023 13:26

add 2 tests on credentials, update CHANGES.md
97e96e8

Merge branch 'master' into migration
c873135

use as(GcpOptions.class) instead of casting, fixing tests
9eca668

nit move set emulator = true up
2aeb151

Abacn reviewed Feb 23, 2023
View reviewed changes

Abacn mentioned this pull request Feb 23, 2023
Disable triggering most IO_Direct precommit on buildSrc and core change #25612
Merged
3 tasks

revert credential changes, piping channel count
0bc1995

mutianf force-pushed the migration branch from 4927be5 to 0bc1995Compare February 27, 2023 23:08

remove bigtable change from CHANGES.md
b08fcf1

igorbernstein2 approved these changes Mar 2, 2023
View reviewed changes

igorbernstein2 reviewed Mar 2, 2023
View reviewed changes

fix nits
ce231c4

Abacn merged commit f30f267 into apache:masterMar 2, 2023

mutianf deleted the migration branch March 3, 2023 15:14

ruslan-ikhsan pushed a commit to akvelon/beam that referenced this pull request Mar 10, 2023
[Bigtable] Migrate BigtableIO to use the veneer client under the hood (…
867f731
…apache#24015)

This was referenced Apr 7, 2023
[Bug]: Regression in BigtableIO in the new 2.47.0 versions #26166
Closed
fix: Fix BigtableIO to share one BigtableService per worker and increase default attempt timeout #26205
Merged

RustedBones mentioned this pull request May 15, 2023
Update beam to v2.47.0 spotify/scio#4805
Merged

Abacn mentioned this pull request Jun 7, 2023
[Task]: Update BigtableIO to use veneer client under the hood #25592
Closed
15 tasks

Uh oh!

Conversation

mutianf commented Nov 7, 2022• edited by Abacn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GitHub Actions Tests Status (on master branch)

Uh oh!

codecovBot commented Dec 1, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actionsBot commented Dec 2, 2022

Uh oh!

diegomez17 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actionsBot commented Dec 20, 2022

Uh oh!

Abacn commented Dec 21, 2022

Uh oh!

Uh oh!

Abacn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actionsBot commented Jan 6, 2023

Uh oh!

Abacn commented Jan 6, 2023

Uh oh!

mutianf commented Feb 21, 2023

Uh oh!

Abacn commented Feb 21, 2023

Uh oh!

Abacn left a comment

Choose a reason for hiding this comment

Uh oh!

AbacnFeb 22, 2023

Choose a reason for hiding this comment

Uh oh!

mutianfFeb 22, 2023

Choose a reason for hiding this comment

Uh oh!

Abacn left a comment

Choose a reason for hiding this comment

Uh oh!

AbacnFeb 23, 2023

Choose a reason for hiding this comment

Uh oh!

mutianfFeb 23, 2023

Choose a reason for hiding this comment

Uh oh!

AbacnFeb 23, 2023

Choose a reason for hiding this comment

Uh oh!

mutianf commented Feb 23, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igorbernstein2Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

igorbernstein2Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

Abacn commented Mar 2, 2023

Uh oh!

mutianf commented Nov 7, 2022•
edited by Abacn
Loading

codecovBot commented Dec 1, 2022•
edited
Loading