Support gcs-connector 3.x in GcsUtil #33368

clairemcginty · 2024-12-12T19:55:34Z

Rationale:

I would like to use gcs-connector 3.x, which supports the new Parquet VectorIO feature. However, gcs-connector 3.x also drops Java 8 and targets Java 11, which blocks us from upgrading it directly in Beam, since Beam is still targeting 8 (see #31678).

Additionally, as a Beam user, I can't just upgrade gcs-connector on my end, due to breaking changes in how GoogleCloudStorageImpl is instantiated: in 2.x it has public constructors, but in 3.x it drops the public constructors and enforces a Builder pattern.

Therefore, when running on gcs-connector 3.x, my pipeline throws a NoSuchMethodError from org.apache.beam.sdk.extensions.gcp.util.GcsUtil when it tries to invoke the 2.x constructor: https://github.com/apache/beam/blob/v2.61.0/sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java#L727

This PR adds a pipeline option for a GoogleCloudStorage Provider, so that users who want to use gcs-connector 3.x can be unblocked from doing so. It defaults to invoking the gcs-connector 2.x public constructor, but 3.x users can override it to use the Builder.

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.

clairemcginty · 2024-12-12T20:28:36Z

...cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/options/GcsOptions.java

+    GoogleCloudStorage get(
+        GoogleCloudStorageOptions options,
+        Storage storage,
+        Credentials credentials,
+        HttpRequestInitializer httpRequestInitializer);


These 4 params should cover both the 2.x constructor and the 3.x Builder

github-actions · 2024-12-12T21:06:11Z

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

clairemcginty · 2024-12-12T22:05:41Z

cc @Abacn - wdyt of this workaround for gcs-connector 3.x?

Abacn · 2024-12-13T23:48:16Z

Hi, thanks for the investigation. Is the builder constructor also supported on 2.x ? If so we can just change to use it in all case and no need extra options exposed to user

clairemcginty · 2024-12-16T13:52:38Z

Hi, thanks for the investigation. Is the builder constructor also supported on 2.x ? If so we can just change to use it in all case and no need extra options exposed to user

Unfortunately it isn't :/ There is no way to construct a GoogleCloudStorageImpl that works for both gcs-connector 2.x and 3.x (at least as far as I've been able to find).

Abacn

understand, thanks!

clairemcginty · 2024-12-22T21:19:14Z

understand, thanks!

thanks @Abacn ! should I update CHANGES.md for the 2.62.0 release or the next one?

Abacn · 2024-12-23T12:22:07Z

java precommit tineout for several rerun. It's passing on HEAD, could you please taking a look if it is related to this change?

Abacn · 2024-12-27T13:15:24Z

after multiple rerun there is indeed a related test failure:

https://github.com/apache/beam/runs/34888346578

testGcpCoreApiSurface (org.apache.beam.sdk.extensions.gcp.GcpCoreApiSurfaceTest) failed

java.lang.AssertionError: 
Expected: API surface to include only:

...

 but: The following disallowed classes appeared on the API surface:
	class com.google.protobuf.AbstractMessage exposed via:
...
interface org.apache.beam.sdk.extensions.gcp.options.GcsOptions$GoogleCloudStorageProvider

looks like it is coded in test to prevent the exposure of unwanted classes. Refactor in a way that does not leak these may fix

clairemcginty · 2024-12-27T16:53:18Z

looks like it is coded in test to prevent the exposure of unwanted classes. Refactor in a way that does not leak these may fix

thanks for looking into it @Abacn ! hmm, this seems challenging to refactor since the exposure is coming from interface com.google.cloud.hadoop.gcsio.GoogleCloudStorage itself... and there's no superclass/subclass that would be appropriate to substitute in here. I guess instead of adding a provider for a GoogleCloudStorage instance, we could add a provider for a BiFunction<StorageResourceId, CreateObjectOptions, WritableByteChannel> ? Although that would leak more implementation details to the user.

clairemcginty · 2025-01-10T15:11:25Z

looks like it is coded in test to prevent the exposure of unwanted classes. Refactor in a way that does not leak these may fix

hmm, this seems challenging to refactor since the exposure is coming from interface com.google.cloud.hadoop.gcsio.GoogleCloudStorage itself... and there's no superclass/subclass that would be appropriate to substitute in here. I guess instead of adding a provider for a GoogleCloudStorage instance, we could add a provider for a BiFunction<StorageResourceId, CreateObjectOptions, WritableByteChannel> ? Although that would leak more implementation details to the user.

hey @Abacn ! just wanted to ping this thread before the PR goes stale :)

Abacn · 2025-01-10T15:43:27Z

a last resort would be using reflection to handle both constructor or builder. Say, use a try to encapsulate the constictor call, when using 3.x dependency ut will throw an error, catch that Exception and construct the instance using Reflection. Is that possible to do?

clairemcginty · 2025-01-10T16:08:05Z

a last resort would be using reflection to handle both constructor or builder. Say, use a try to encapsulate the constictor call, when using 3.x dependency ut will throw an error, catch that Exception and construct the instance using Reflection. Is that possible to do?

I think that makes sense... so we'd keep the official Beam dep on 2.x. but try to reflectively construct a 3.x instance first?

…nnector 3.x

…onstructor

clairemcginty · 2025-01-10T19:32:02Z

seems to work! tested it on 2.x and 3.x 👍

Abacn · 2025-01-10T19:36:48Z

...oogle-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java

-    return new GoogleCloudStorageImpl(options, storage, credentials);
+    try {
+      // Attempt to construct gcs-connector 3.x-style GoogleCloudStorage, which is created
+      // exclusively via Builder method; this can be replaced once Java 8 is dropped and


could add a // TODO tag like

// TODO eliminate reflection once Beam drops Java 8 support and upgrade to use gcsio 3.x

good idea- updated 👍

clairemcginty · 2025-01-13T15:42:03Z

there is an integration failure but it doesn't look related:

org.apache.beam.sdk.io.gcp.bigquery.StorageApiSinkSchemaUpdateIT > testExactlyOnceWithAutoSchemaUpdate[1] FAILED
    org.apache.beam.sdk.Pipeline$PipelineExecutionException at StorageApiSinkSchemaUpdateIT.java:431
        Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException at GoogleJsonResponseException.java:146

Abacn · 2025-01-13T21:41:23Z

Sorry for another late request

I've been a little bit worry about relying on the try {} catch {} to determine the code path, which happened to bring out performance regression in #33575

since using Reflection is already a hack, I do not see a good solution to this either. But could we try {return new GoogleCloudStorageImpl(...)} first? So in the default dependency versions (gcs 2.x), it won't throw Exception (even though it is handled) after this change

clairemcginty · 2025-01-14T14:12:51Z

that makes sense @Abacn ! Updated so that we try to the 2.x constructor first and attempt the 3.x Builder only if that throws a NoSuchMethodError.

clairemcginty · 2025-01-14T14:53:18Z

looks like there was a spotbugs error - looking into it now..

Abacn · 2025-01-14T15:14:01Z

yeah, can be found in "SpotBugs Results": https://github.com/apache/beam/actions/runs/12768890245?pr=33368


Code | Warning
-- | --
REC | Exception is caught when Exception is not thrown in org.apache.beam.sdk.extensions.gcp.util.GcsUtil.createGoogleCloudStorage(GoogleCloudStorageOptions, Storage, Credentials)
  | Bug type REC_CATCH_EXCEPTION (click for details)In class org.apache.beam.sdk.extensions.gcp.util.GcsUtilIn method org.apache.beam.sdk.extensions.gcp.util.GcsUtil.createGoogleCloudStorage(GoogleCloudStorageOptions, Storage, Credentials)At GcsUtil.java:[line 780]

Code	Warning
REC	Exception is caught when Exception is not thrown in org.apache.beam.sdk.extensions.gcp.util.GcsUtil.createGoogleCloudStorage(GoogleCloudStorageOptions, Storage, Credentials)
[Bug type REC_CATCH_EXCEPTION (click for details)](https://github.com/apache/beam/pull/33368#REC_CATCH_EXCEPTION)
In class org.apache.beam.sdk.extensions.gcp.util.GcsUtil
In method org.apache.beam.sdk.extensions.gcp.util.GcsUtil.createGoogleCloudStorage(GoogleCloudStorageOptions, Storage, Credentials)
At GcsUtil.java:[line 780]

Abacn · 2025-01-14T15:15:14Z

~~catch only the checked reflection Exceptions should fix the issue~~ I see what the check means now, see below

...oogle-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java

* Parameterize GoogleCloudStorage provider in GcsUtil to unblock gcs-connector 3.x * Use Reflection to attempt 3.x Builder, and fall back to 2.x Constructor * Attempt constructing 3.x-style GCS via reflection; fall back to 2.x constructor * Update CHANGES.md * Add TODO note on reflection block * Try non-reflected construction first * Fix SpotBugs error

github-actions bot added java extensions gcp labels Dec 12, 2024

clairemcginty commented Dec 12, 2024

View reviewed changes

Abacn approved these changes Dec 21, 2024

View reviewed changes

clairemcginty changed the title ~~Parameterize GoogleCloudStorage provider in GcsUtil to unblock gcs-co…~~ Support gcs-connector 3.x in GcsUtil Jan 10, 2025

clairemcginty added 4 commits January 10, 2025 14:30

Parameterize GoogleCloudStorage provider in GcsUtil to unblock gcs-co…

fd96c32

…nnector 3.x

Use Reflection to attempt 3.x Builder, and fall back to 2.x Constructor

45eff10

Attempt constructing 3.x-style GCS via reflection; fall back to 2.x c…

b829452

…onstructor

Update CHANGES.md

ca8f9c0

clairemcginty force-pushed the gcs-connector-compat branch from f343641 to ca8f9c0 Compare January 10, 2025 19:31

Abacn reviewed Jan 10, 2025

View reviewed changes

Add TODO note on reflection block

7c5e744

Try non-reflected construction first

72bca6b

Abacn reviewed Jan 14, 2025

View reviewed changes

...oogle-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java Outdated Show resolved Hide resolved

Fix SpotBugs error

e4e272a

Abacn approved these changes Jan 14, 2025

View reviewed changes

Abacn merged commit ff8b6a1 into apache:master Jan 14, 2025
18 of 19 checks passed

clairemcginty deleted the gcs-connector-compat branch January 14, 2025 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support gcs-connector 3.x in GcsUtil #33368

Support gcs-connector 3.x in GcsUtil #33368

clairemcginty commented Dec 12, 2024 •

edited

Loading

clairemcginty Dec 12, 2024

github-actions bot commented Dec 12, 2024

clairemcginty commented Dec 12, 2024

Abacn commented Dec 13, 2024

clairemcginty commented Dec 16, 2024

Abacn left a comment

clairemcginty commented Dec 22, 2024

Abacn commented Dec 23, 2024

Abacn commented Dec 27, 2024

clairemcginty commented Dec 27, 2024 •

edited

Loading

clairemcginty commented Jan 10, 2025

Abacn commented Jan 10, 2025

clairemcginty commented Jan 10, 2025

clairemcginty commented Jan 10, 2025

Abacn Jan 10, 2025

clairemcginty Jan 10, 2025

clairemcginty commented Jan 13, 2025

Abacn commented Jan 13, 2025

clairemcginty commented Jan 14, 2025

clairemcginty commented Jan 14, 2025

Abacn commented Jan 14, 2025

Abacn commented Jan 14, 2025 •

edited

Loading

Support gcs-connector 3.x in GcsUtil #33368

Support gcs-connector 3.x in GcsUtil #33368

Conversation

clairemcginty commented Dec 12, 2024 • edited Loading

GitHub Actions Tests Status (on master branch)

clairemcginty Dec 12, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 12, 2024

clairemcginty commented Dec 12, 2024

Abacn commented Dec 13, 2024

clairemcginty commented Dec 16, 2024

Abacn left a comment

Choose a reason for hiding this comment

clairemcginty commented Dec 22, 2024

Abacn commented Dec 23, 2024

Abacn commented Dec 27, 2024

clairemcginty commented Dec 27, 2024 • edited Loading

clairemcginty commented Jan 10, 2025

Abacn commented Jan 10, 2025

clairemcginty commented Jan 10, 2025

clairemcginty commented Jan 10, 2025

Abacn Jan 10, 2025

Choose a reason for hiding this comment

clairemcginty Jan 10, 2025

Choose a reason for hiding this comment

clairemcginty commented Jan 13, 2025

Abacn commented Jan 13, 2025

clairemcginty commented Jan 14, 2025

clairemcginty commented Jan 14, 2025

Abacn commented Jan 14, 2025

Abacn commented Jan 14, 2025 • edited Loading

clairemcginty commented Dec 12, 2024 •

edited

Loading

clairemcginty commented Dec 27, 2024 •

edited

Loading

Abacn commented Jan 14, 2025 •

edited

Loading