Replace StorageV1 client with GCS client (take 4) #28079

BjornPrime · 2023-08-21T17:52:51Z

Addresses #25676.

Now with improved error handling on batch requests (and test coverage of same).

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

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 or the workflows README to see a list of phrases to trigger workflows.

codecov · 2023-08-21T18:09:50Z

Codecov Report

Merging #28079 (904550e) into master (042ac52) will decrease coverage by 0.36%.
Report is 37 commits behind head on master.
The diff coverage is 48.59%.

@@            Coverage Diff             @@
##           master   #28079      +/-   ##
==========================================
- Coverage   72.31%   71.96%   -0.36%     
==========================================
  Files         684      680       -4     
  Lines      101727   100163    -1564     
==========================================
- Hits        73567    72080    -1487     
+ Misses      26581    26505      -76     
+ Partials     1579     1578       -1

Flag	Coverage Δ
python	`82.53% <48.59%> (-0.28%)`	⬇️

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

Files	Coverage Δ
...pache_beam/dataframe/pandas_top_level_functions.py	`90.62% <100.00%> (+0.33%)`	⬆️
sdks/python/apache_beam/internal/gcp/auth.py	`79.72% <100.00%> (+0.56%)`	⬆️
sdks/python/apache_beam/io/gcp/bigquery_tools.py	`75.00% <ø> (ø)`
sdks/python/apache_beam/io/gcp/bigtableio.py	`74.04% <ø> (-0.96%)`	⬇️
sdks/python/apache_beam/io/gcp/gcsfilesystem.py	`89.78% <100.00%> (+1.12%)`	⬆️
sdks/python/apache_beam/typehints/schemas.py	`88.91% <100.00%> (-2.71%)`	⬇️
sdks/python/apache_beam/version.py	`100.00% <100.00%> (ø)`
sdks/python/setup.py	`0.00% <ø> (ø)`
sdks/python/apache_beam/io/gcp/bigquery.py	`69.92% <50.00%> (-0.25%)`	⬇️
...ks/python/apache_beam/runners/interactive/utils.py	`94.69% <90.00%> (-0.40%)`	⬇️
... and 4 more

... and 14 files with indirect coverage changes

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

Abacn · 2023-08-23T15:30:21Z

sdks/python/apache_beam/io/gcp/gcsio.py


-    return [(pair[0], pair[1], pair_to_status[pair]) for pair in src_dest_pairs]
+      for pair, resp in list(zip(current_pairs, current_batch._responses)):


May mentioned in the past - current_paths and current_batch._responses has different length. Actually, for copy the response has 4x of the length as the current_paths. This is because copy involves multiple operations.

python zip does not care length match and this would miss potential error code beyond first N response (N=batch size)

May consider zip(current_batch._target_objects, current_batch._responses) they should have the same size.

I'm pretty sure I'm testing for this already, but I've added an assertion just to be sure

I made a test like this

https://github.com/Abacn/beam/blob/4a84d1618ed23614ec663f2cda1758d280ed5da6/sdks/python/apache_beam/io/gcp/gcsio_test.py#L481-500

also added some print to gcsio.py

https://github.com/Abacn/beam/blob/4a84d1618ed23614ec663f2cda1758d280ed5da6/sdks/python/apache_beam/io/gcp/gcsio.py#L292

Output:

5 20 200 200 200 200 200 200 200 200 200 200 200 200 200 404 200 404 200 200 200 200 gs://clouddfe-yihu-test/temp1/gcs_0.txt gs://clouddfe-yihu-test/temp2/gcs_0.txt 200 gs://clouddfe-yihu-test/temp1/gcs_1.txt gs://clouddfe-yihu-test/temp2/gcs_1.txt 200 gs://clouddfe-yihu-test/temp1/gcs_2.txt gs://clouddfe-yihu-test/temp2/gcs_2.txt 200 gs://clouddfe-yihu-test/temp1/gcs_3.txt gs://clouddfe-yihu-test/temp2/gcs_3.txt 200 gs://clouddfe-yihu-test/temp1/gcs_4.txt gs://clouddfe-yihu-test/temp2/gcs_4.txt 200

As can be seen, the length current_pairs and current_batch._responses is different (4 fold), and the return value missed the 404 code

I see what you're saying, and running the tests with the assertion included has confirmed it. I'll dig into the client more to see what we can do about it.

BjornPrime · 2023-08-23T16:55:21Z

Run Python 3.11 PostCommit

BjornPrime · 2023-08-30T19:57:50Z

Run Python 3.11 PostCommit

BjornPrime · 2023-08-31T18:20:38Z

Run Python 3.11 PostCommit

BjornPrime · 2023-09-01T21:07:24Z

Run Python 3.11 PostCommit

BjornPrime · 2023-09-05T14:41:10Z

Run Python 3.11 PostCommit

BjornPrime · 2023-09-05T17:48:22Z

Run Python 3.11 PostCommit

BjornPrime · 2023-09-06T17:05:20Z

Run Python 3.11 PostCommit

BjornPrime · 2023-09-07T21:45:50Z

Run Python 3.11 PostCommit

BjornPrime · 2023-09-08T13:31:10Z

Run Python 3.11 PostCommit

BjornPrime · 2023-09-08T17:30:25Z

Run Python_Runners PreCommit 3.10

BjornPrime · 2023-09-11T15:44:51Z

Run Python 3.11 PostCommit

BjornPrime · 2023-09-11T22:44:09Z

Run Python 3.11 PostCommit

BjornPrime · 2023-09-12T15:17:42Z

Run Python 3.11 PostCommit

github-actions · 2023-09-21T19:35:09Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

Abacn

Thanks for persistent on this. I went through carefully and commented on potential issues I found.

Abacn · 2023-09-22T22:54:16Z

sdks/python/apache_beam/io/gcp/bigquery_tools_test.py

@@ -224,23 +223,6 @@ def test_delete_dataset_retries_for_timeouts(self, patched_time_sleep):
    wrapper._delete_dataset('', '')
    self.assertTrue(client.datasets.Delete.called)

-  @unittest.skipIf(


this test gets removed while the reason why it fails internally remains unknown. From a high level there might be still some regression regarding credential / anonymous credentials - to aware

I am fine with it for now but please do not remove the test until we understand why it fails in certain environment, could skip if for now

I discussed this with Pablo and we agreed that the test was no longer necessary. While its failure is curious, IIRC, we determined that the test was not ultimately testing something relevant to production, so we didn't actually need to worry about it.

This tests if user agent info is added to the request sent to bigquery. In various gcp component we use this info to track usage. This effort is active e.g. gcs counterpart #26770 #27211

That's possibly why it is saying not relevent to production, but does not mean it can be removed

We can make it "skipped" but please do not remove test

Okay, I'll skip it and open an issue for it, since you say it's important.

sdks/python/apache_beam/options/pipeline_options_validator_test.py

sdks/python/apache_beam/runners/dataflow/internal/apiclient.py

sdks/python/setup.py

sdks/python/apache_beam/io/gcp/gcsio.py

github-actions · 2023-09-25T19:25:12Z

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

R: @AnandInguva for label python.
R: @ahmedabu98 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).

Abacn · 2023-09-25T22:10:25Z

Run Python 3.11 PostCommit

Abacn · 2023-09-26T14:10:30Z

PreCommit Portable Python workflow fails during clean up unrelated. Opened actions/setup-java#543

This reverts commit 94812c0.

…ache#28721) This reverts commit 56b1259.

The two commits are merged into one: * Reapply "Replace StorageV1 client with GCS client (apache#28079)" (apache#28721) * added project parameter to apiclient

* Cherry pick two previous commits on migrating gcs client The two commits are merged into one: * Reapply "Replace StorageV1 client with GCS client (#28079)" (#28721) * added project parameter to apiclient * Initialze storage client with project from pipeline option. --------- Co-authored-by: Bjorn Pedersen <[email protected]>

* Cherry pick two previous commits on migrating gcs client The two commits are merged into one: * Reapply "Replace StorageV1 client with GCS client (apache#28079)" (apache#28721) * added project parameter to apiclient * Initialze storage client with project from pipeline option. --------- Co-authored-by: Bjorn Pedersen <[email protected]>

github-actions bot added python examples docker io gcp labels Aug 21, 2023

BjornPrime changed the title ~~Replace StorageV1 client with GCS client (take 3)~~ Replace StorageV1 client with GCS client (take 4) Aug 21, 2023

Abacn reviewed Aug 23, 2023

View reviewed changes

BjornPrime mentioned this pull request Aug 28, 2023

Batch requests generating multiple response codes for each request googleapis/python-storage#1112

Closed

liferoad mentioned this pull request Aug 31, 2023

[Bug]: GCS IO publishes uninformative error message #26769

Closed

15 tasks

BjornPrime force-pushed the storage-client-replacement branch from 6933f3e to a86bb2d Compare September 8, 2023 18:57

BjornPrime force-pushed the storage-client-replacement branch from fc594f5 to 208c369 Compare September 11, 2023 17:59

github-actions bot removed the docker label Sep 11, 2023

BjornPrime mentioned this pull request Sep 11, 2023

[Feature Request]: Decrease the number of GET requests GCSIO makes #28398

Closed

15 tasks

BjornPrime force-pushed the storage-client-replacement branch from 208c369 to d3023c8 Compare September 14, 2023 15:07

This was referenced Sep 21, 2023

Replace storage_v1_client with GCS client #25676

Closed

[Task]: Update the minor version of google-cloud-storage library prior to Beam release. #27326

Closed

BjornPrime marked this pull request as ready for review September 21, 2023 19:05

Abacn reviewed Sep 22, 2023

View reviewed changes

github-actions bot added the Next Action: Reviewers label Sep 25, 2023

Replace StorageV1 client with GCS client

904550e

BjornPrime force-pushed the storage-client-replacement branch from 93ab58e to 904550e Compare September 26, 2023 02:29

Abacn approved these changes Sep 26, 2023

View reviewed changes

Abacn merged commit 94812c0 into apache:master Sep 26, 2023
81 of 85 checks passed

damccorm added a commit that referenced this pull request Sep 28, 2023

Revert "Replace StorageV1 client with GCS client (#28079)"

8223674

This reverts commit 94812c0.

damccorm added a commit that referenced this pull request Sep 28, 2023

Revert "Replace StorageV1 client with GCS client (#28079)" (#28721)

56b1259

This reverts commit 94812c0.

BjornPrime added a commit to BjornPrime/my-beam that referenced this pull request Oct 17, 2023

Reapply "Replace StorageV1 client with GCS client (apache#28079)" (ap…

163a288

…ache#28721) This reverts commit 56b1259.

ghost mentioned this pull request Nov 8, 2023

Migrate gcs client #29351

Closed

3 tasks

jto mentioned this pull request Nov 17, 2023

Fix stuck nexmark streaming #29465

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace StorageV1 client with GCS client (take 4) #28079

Replace StorageV1 client with GCS client (take 4) #28079

BjornPrime commented Aug 21, 2023 •

edited

Loading

codecov bot commented Aug 21, 2023 •

edited

Loading

Abacn Aug 23, 2023

BjornPrime Aug 23, 2023

Abacn Aug 23, 2023

BjornPrime Aug 24, 2023

BjornPrime commented Aug 23, 2023

BjornPrime commented Aug 30, 2023

BjornPrime commented Aug 31, 2023

BjornPrime commented Sep 1, 2023

BjornPrime commented Sep 5, 2023

BjornPrime commented Sep 5, 2023

BjornPrime commented Sep 6, 2023

BjornPrime commented Sep 7, 2023

BjornPrime commented Sep 8, 2023

BjornPrime commented Sep 8, 2023

BjornPrime commented Sep 11, 2023

BjornPrime commented Sep 11, 2023

BjornPrime commented Sep 12, 2023

github-actions bot commented Sep 21, 2023

Abacn left a comment

Abacn Sep 22, 2023 •

edited

Loading

BjornPrime Sep 25, 2023

Abacn Sep 25, 2023

BjornPrime Sep 25, 2023

github-actions bot commented Sep 25, 2023

Abacn commented Sep 25, 2023

Abacn commented Sep 26, 2023


		return [(pair[0], pair[1], pair_to_status[pair]) for pair in src_dest_pairs]
		for pair, resp in list(zip(current_pairs, current_batch._responses)):

Replace StorageV1 client with GCS client (take 4) #28079

Replace StorageV1 client with GCS client (take 4) #28079

Conversation

BjornPrime commented Aug 21, 2023 • edited Loading

GitHub Actions Tests Status (on master branch)

codecov bot commented Aug 21, 2023 • edited Loading

Codecov Report

Abacn Aug 23, 2023

Choose a reason for hiding this comment

BjornPrime Aug 23, 2023

Choose a reason for hiding this comment

Abacn Aug 23, 2023

Choose a reason for hiding this comment

BjornPrime Aug 24, 2023

Choose a reason for hiding this comment

BjornPrime commented Aug 23, 2023

BjornPrime commented Aug 30, 2023

BjornPrime commented Aug 31, 2023

BjornPrime commented Sep 1, 2023

BjornPrime commented Sep 5, 2023

BjornPrime commented Sep 5, 2023

BjornPrime commented Sep 6, 2023

BjornPrime commented Sep 7, 2023

BjornPrime commented Sep 8, 2023

BjornPrime commented Sep 8, 2023

BjornPrime commented Sep 11, 2023

BjornPrime commented Sep 11, 2023

BjornPrime commented Sep 12, 2023

github-actions bot commented Sep 21, 2023

Abacn left a comment

Choose a reason for hiding this comment

Abacn Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

BjornPrime Sep 25, 2023

Choose a reason for hiding this comment

Abacn Sep 25, 2023

Choose a reason for hiding this comment

BjornPrime Sep 25, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 25, 2023

Abacn commented Sep 25, 2023

Abacn commented Sep 26, 2023

BjornPrime commented Aug 21, 2023 •

edited

Loading

codecov bot commented Aug 21, 2023 •

edited

Loading

Abacn Sep 22, 2023 •

edited

Loading