gemini-code-assist[bot] commented on code in PR #38583:
URL: https://github.com/apache/beam/pull/38583#discussion_r3281312875
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtilV1.java:
##########
@@ -265,12 +267,8 @@ public boolean shouldRetry(IOException e) {
.setReadChannelOptions(gcsReadOptions)
.setGrpcEnabled(shouldUseGrpc)
.build();
- try {
- googleCloudStorage =
- createGoogleCloudStorage(googleCloudStorageOptions, storageClient,
credentials);
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
+ googleCloudStorage =
+ createGoogleCloudStorage(googleCloudStorageOptions, storageClient,
credentials);
Review Comment:
The `createGoogleCloudStorage` method still needs to handle or declare
`IOException` because the `GoogleCloudStorageImpl` constructor in `gcsio` 2.x
throws it. The try-catch block should be restored here to maintain proper error
handling and ensure the code compiles.
```java
try {
googleCloudStorage =
createGoogleCloudStorage(googleCloudStorageOptions, storageClient,
credentials);
} catch (IOException e) {
throw new RuntimeException(e);
}
```
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtilV1.java:
##########
@@ -727,16 +725,48 @@ public WritableByteChannel create(GcsPath path,
CreateOptions options) throws IO
}
GoogleCloudStorage createGoogleCloudStorage(
- GoogleCloudStorageOptions options, Storage storage, Credentials
credentials)
- throws IOException {
- return GoogleCloudStorageImpl.builder()
- .setOptions(options)
- .setHttpTransport(storage.getRequestFactory().getTransport())
- .setCredentials(credentials)
- // gcsio 3 expects httpRequestInitializer to be either absent or
- // com.google.cloud.hadoop.util.RetryHttpInitializer when credentials
not provided
- .setHttpRequestInitializer(credentials != null ?
httpRequestInitializer : null)
- .build();
+ GoogleCloudStorageOptions options, Storage storage, Credentials
credentials) {
+ try {
+ return new GoogleCloudStorageImpl(options, storage, credentials);
+ } catch (NoSuchMethodError e) {
+ // gcs-connector 3.x drops the direct constructor and exclusively uses
Builder
+ // TODO eliminate reflection once Beam drops Java 8 support and upgrades
to gcsio 3.x
+ try {
+ final Method builderMethod =
GoogleCloudStorageImpl.class.getMethod("builder");
+ Object builder = builderMethod.invoke(null);
+ final Class<?> builderClass =
+ Class.forName(
+
"com.google.cloud.hadoop.gcsio.AutoBuilder_GoogleCloudStorageImpl_Builder");
+
+ final Method setOptionsMethod =
+ builderClass.getMethod("setOptions",
GoogleCloudStorageOptions.class);
+ setOptionsMethod.setAccessible(true);
+ builder = setOptionsMethod.invoke(builder, options);
+
+ final Method setHttpTransportMethod =
+ builderClass.getMethod("setHttpTransport", HttpTransport.class);
+ setHttpTransportMethod.setAccessible(true);
+ builder =
+ setHttpTransportMethod.invoke(builder,
storage.getRequestFactory().getTransport());
+
+ final Method setCredentialsMethod =
+ builderClass.getMethod("setCredentials", Credentials.class);
+ setCredentialsMethod.setAccessible(true);
+ builder = setCredentialsMethod.invoke(builder, credentials);
+
+ final Method setHttpRequestInitializerMethod =
+ builderClass.getMethod("setHttpRequestInitializer",
HttpRequestInitializer.class);
+ setHttpRequestInitializerMethod.setAccessible(true);
+ builder = setHttpRequestInitializerMethod.invoke(builder,
httpRequestInitializer);
Review Comment:
The reflection logic for `gcsio` 3.x should preserve the original behavior
of passing `null` for the `httpRequestInitializer` when `credentials` are null.
This was specifically required for `gcsio` 3.x as noted in the previous
implementation's comments.
```suggestion
final Method setHttpRequestInitializerMethod =
builderClass.getMethod("setHttpRequestInitializer",
HttpRequestInitializer.class);
setHttpRequestInitializerMethod.setAccessible(true);
builder = setHttpRequestInitializerMethod.invoke(
builder, credentials != null ? httpRequestInitializer : null);
```
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtilV1.java:
##########
@@ -727,16 +725,48 @@ public WritableByteChannel create(GcsPath path,
CreateOptions options) throws IO
}
GoogleCloudStorage createGoogleCloudStorage(
- GoogleCloudStorageOptions options, Storage storage, Credentials
credentials)
- throws IOException {
- return GoogleCloudStorageImpl.builder()
- .setOptions(options)
- .setHttpTransport(storage.getRequestFactory().getTransport())
- .setCredentials(credentials)
- // gcsio 3 expects httpRequestInitializer to be either absent or
- // com.google.cloud.hadoop.util.RetryHttpInitializer when credentials
not provided
- .setHttpRequestInitializer(credentials != null ?
httpRequestInitializer : null)
- .build();
+ GoogleCloudStorageOptions options, Storage storage, Credentials
credentials) {
+ try {
+ return new GoogleCloudStorageImpl(options, storage, credentials);
+ } catch (NoSuchMethodError e) {
+ // gcs-connector 3.x drops the direct constructor and exclusively uses
Builder
+ // TODO eliminate reflection once Beam drops Java 8 support and upgrades
to gcsio 3.x
+ try {
+ final Method builderMethod =
GoogleCloudStorageImpl.class.getMethod("builder");
+ Object builder = builderMethod.invoke(null);
+ final Class<?> builderClass =
+ Class.forName(
+
"com.google.cloud.hadoop.gcsio.AutoBuilder_GoogleCloudStorageImpl_Builder");
Review Comment:
Hardcoding the internal `AutoBuilder` class name is fragile and depends on
implementation details of the `gcsio` library. It is safer to obtain the class
directly from the builder instance returned by
`GoogleCloudStorageImpl.builder()`.
```suggestion
final Class<?> builderClass = builder.getClass();
```
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtilV1.java:
##########
@@ -727,16 +725,48 @@ public WritableByteChannel create(GcsPath path,
CreateOptions options) throws IO
}
GoogleCloudStorage createGoogleCloudStorage(
- GoogleCloudStorageOptions options, Storage storage, Credentials
credentials)
- throws IOException {
- return GoogleCloudStorageImpl.builder()
- .setOptions(options)
- .setHttpTransport(storage.getRequestFactory().getTransport())
- .setCredentials(credentials)
- // gcsio 3 expects httpRequestInitializer to be either absent or
- // com.google.cloud.hadoop.util.RetryHttpInitializer when credentials
not provided
- .setHttpRequestInitializer(credentials != null ?
httpRequestInitializer : null)
- .build();
+ GoogleCloudStorageOptions options, Storage storage, Credentials
credentials) {
Review Comment:
The `GoogleCloudStorageImpl` constructor in `gcsio` 2.x throws a checked
`IOException`. Removing `throws IOException` from this method signature will
cause a compilation error when using the 2.x library. Please restore the
exception declaration.
```java
GoogleCloudStorage createGoogleCloudStorage(
GoogleCloudStorageOptions options, Storage storage, Credentials
credentials)
throws IOException {
```
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/IcebergScanConfig.java:
##########
@@ -19,7 +19,7 @@
import static
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions.checkArgument;
import static
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions.checkNotNull;
-import static
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.Sets.newHashSet;
+import static org.apache.hadoop.util.Sets.newHashSet;
Review Comment:
Beam prefers using its vendored Guava dependencies
(`org.apache.beam.vendor.guava...`) over external utility classes like
`org.apache.hadoop.util.Sets` to ensure classpath stability and avoid version
conflicts with user-provided Hadoop environments.
```suggestion
import static
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.Sets.newHashSet;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
