Gcsio migration by shunping · Pull Request #29360 · apache/beam

shunping · 2023-11-09T01:38:08Z

This is the continuation of gcsio migration work. The majority work has been done by @BjornPrime, and here I am fixing a few edge cases and trying to make sure all tests are passed before submission.

fixes #25676

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.

github-actions · 2023-11-09T03:05:45Z

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

github-actions · 2023-11-10T00:49:38Z

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

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

shunping · 2023-11-10T00:57:25Z

I am going to do some more tests before it is merged. Will let you know

shunping · 2023-11-10T18:28:31Z

stop reviewer notifications

github-actions · 2023-11-10T18:30:05Z

Stopping reviewer notifications for this pull request: requested by reviewer

codecov · 2023-11-11T05:41:31Z

Codecov Report

Attention: 174 lines in your changes are missing coverage. Please review.

Comparison is base (729c4de) 38.32% compared to head (acb57f3) 37.85%.
Report is 3 commits behind head on master.

Files	Patch %	Lines
sdks/python/apache_beam/io/gcp/gcsio.py	10.31%	113 Missing ⚠️
...apache_beam/runners/dataflow/internal/apiclient.py	0.00%	22 Missing ⚠️
..._beam/runners/portability/sdk_container_builder.py	0.00%	15 Missing ⚠️
sdks/python/apache_beam/io/gcp/gcsfilesystem.py	0.00%	13 Missing ⚠️
...ks/python/apache_beam/runners/interactive/utils.py	0.00%	10 Missing ⚠️
sdks/python/apache_beam/internal/gcp/auth.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #29360      +/-   ##
==========================================
- Coverage   38.32%   37.85%   -0.47%     
==========================================
  Files         694      690       -4     
  Lines      102373   101305    -1068     
==========================================
- Hits        39235    38354     -881     
+ Misses      61546    61359     -187     
  Partials     1592     1592

Flag	Coverage Δ
python	`29.01% <7.44%> (-0.87%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

johnjcasey

Approved! lets hope this attempt and our safeguards are enough!

* 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 <bjornpedersen@google.com>

AnandInguva · 2023-11-17T19:13:58Z

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

@@ -352,27 +352,20 @@ def delete(self, paths):
    Args:


Is line 350 comment still relevant? The directories are not getting deleted recursively.

GCS is currently a flat system. I think it will match and delete all the files with that prefix, which is equivalent to delete the files recursively.

Hm..I am running few example and still the files within the matched prefix are not getting deleted.

For example,

I have path=gs://anandinguva-test/artifacts/53b617/ and when I call GCSFileSystem(options).delete([path]), I expect it to delete the directories/buckets and files/objects within the path. Maybe we can clarify this in comments.

https://github.com/apache/beam/pull/29477/files - this follows a pattern to local filesystem. Would this work?

Have you debugged the original code? I am curious of which part doesn't work. I guess it is the file matching part?

Yes, the matching part. When a directory is provided, we append *.

expected - all the dirs and objects should be deleted.

Actual - only objects are getting deleted.

Solution - let's use bucket.list_buckets() and match the path with the provided dirs and delete them. I can spin up a PR if this sounds good.

That makes sense to me. Thanks for working on this! Please go ahead to create a PR.

This value was changed in PR apache#29360 to 1000, which led to internal test failure.

This value was changed in PR #29360 to 1000, which led to internal test failure.

This value was changed in PR apache#29360 to 1000, which led to internal test failure.

github-actions bot added python examples io gcp labels Nov 9, 2023

shunping force-pushed the gcsio-migration branch from a94b514 to 7b5367d Compare November 9, 2023 23:55

github-actions bot added the Next Action: Reviewers label Nov 10, 2023

liferoad approved these changes Nov 10, 2023

View reviewed changes

shunping force-pushed the gcsio-migration branch from ec18fcd to 1d66d36 Compare November 10, 2023 18:27

BjornPrime and others added 2 commits November 16, 2023 09:18

Cherry pick two previous commits on migrating gcs client

9e885c0

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.

acb57f3

shunping force-pushed the gcsio-migration branch from 1d66d36 to acb57f3 Compare November 16, 2023 14:20

shunping requested a review from liferoad November 16, 2023 15:14

johnjcasey approved these changes Nov 16, 2023

View reviewed changes

johnjcasey merged commit 8ac8b20 into apache:master Nov 16, 2023

Abacn pushed a commit that referenced this pull request Nov 17, 2023

Gcsio migration (#29360) (#29465)

65ddc34

AnandInguva reviewed Nov 17, 2023

View reviewed changes

shunping added a commit to shunping/beam that referenced this pull request Nov 21, 2023

Revert the value of MAX_BATCH_OPERATION_SIZE in gcsio

374105f

This value was changed in PR apache#29360 to 1000, which led to internal test failure.

shunping mentioned this pull request Nov 21, 2023

Revert the value of MAX_BATCH_OPERATION_SIZE in gcsio #29503

Merged

3 tasks

johnjcasey pushed a commit that referenced this pull request Nov 21, 2023

Revert the value of MAX_BATCH_OPERATION_SIZE in gcsio (#29503)

392a4a1

This value was changed in PR #29360 to 1000, which led to internal test failure.

riteshghorse pushed a commit to riteshghorse/beam that referenced this pull request Nov 21, 2023

Revert the value of MAX_BATCH_OPERATION_SIZE in gcsio (apache#29503)

87a86d6

This value was changed in PR apache#29360 to 1000, which led to internal test failure.

damccorm mentioned this pull request Dec 6, 2023

Replace storage_v1_client with GCS client #25676

Closed

15 tasks

shunping mentioned this pull request Dec 14, 2023

Add user-agent string for Beam in the GCS request header. #29765

Merged

3 tasks

shunping mentioned this pull request Aug 13, 2024

[Bug]: [Python SDK] Data Corruption on GCS read in 2.53.0 - 2.58.0 SDKs. #32169

Closed

17 tasks

Conversation

shunping commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GitHub Actions Tests Status (on master branch)

Uh oh!

github-actions bot commented Nov 9, 2023

Uh oh!

github-actions bot commented Nov 10, 2023

Uh oh!

shunping commented Nov 10, 2023

Uh oh!

shunping commented Nov 10, 2023

Uh oh!

github-actions bot commented Nov 10, 2023

Uh oh!

codecov bot commented Nov 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

johnjcasey left a comment

Choose a reason for hiding this comment

Uh oh!

AnandInguva Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

shunping Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

AnandInguva Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

AnandInguva Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

shunping Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

AnandInguva Nov 19, 2023

Choose a reason for hiding this comment

Uh oh!

shunping Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shunping commented Nov 9, 2023 •

edited

Loading

codecov bot commented Nov 11, 2023 •

edited

Loading