You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "clairemcginty (via GitHub)" <gi...@apache.org> on 2024/10/14 14:52:05 UTC
[PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
clairemcginty opened a new pull request, #32769:
URL: https://github.com/apache/beam/pull/32769
Context: I was reading GCS Parquet files via SplittableDoFn and noticed that `ReadableFile#openSeekable` does not propagate any of the [gcs-connector options](https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/v2.2.25/gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemConfiguration.java#L66-L562) specified in my `core-site.xml` file. Particularly, I wanted to turn off [fs.gs.inputstream.fast.fail.on.not.found.enable](https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/v2.2.25/gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemConfiguration.java#L330), which is redundant in a SDF with default empty-match treatment, and tweak [fs.gs.inputstream.fadvise](https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/v2.2.25/gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemConfiguration.java#L350). It looks like these `GoogleCloudStorageReadOptions` options need to be set explicitly in GcsUtil, and passe
d to any GoogleCloudStorage#open calls (see [reference](https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/v2.2.25/gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageImpl.java#L802-L846)).
The big downside of this PR is of course, pulling in Hadoop :( The alternative is to manually copy-paste all the Configuration keys manually into `GcsUtil`, which seems harder to maintain. Or, I could omit the `GcsReadOptionsFactory` factory logic entirely and leave it 100% up to the user to construct `GoogleCloudStorageReadOptions` instances.
------------------------
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](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #32769:
URL: https://github.com/apache/beam/pull/32769#discussion_r1856941717
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java:
##########
@@ -123,6 +125,58 @@ public static GcsCountersOptions create(
}
}
+ public static class GcsReadOptionsFactory
+ implements DefaultValueFactory<GoogleCloudStorageReadOptions> {
+ @Override
+ public GoogleCloudStorageReadOptions create(PipelineOptions options) {
+ try {
+ // Check if gcs-connector-hadoop is loaded into classpath
+ Class.forName("com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration");
+ Configuration config = new Configuration();
+ return GoogleCloudStorageReadOptions.builder()
+ .setFastFailOnNotFound(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_FAST_FAIL_ON_NOT_FOUND_ENABLE
+ .get(config, config::getBoolean))
+ .setSupportGzipEncoding(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_SUPPORT_GZIP_ENCODING_ENABLE
+ .get(config, config::getBoolean))
+ .setInplaceSeekLimit(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_INPLACE_SEEK_LIMIT.get(
+ config, config::getLong))
+ .setFadvise(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_FADVISE.get(
+ config, config::getEnum))
+ .setMinRangeRequestSize(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_MIN_RANGE_REQUEST_SIZE.get(
+ config, config::getInt))
+ .setGrpcChecksumsEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_CHECKSUMS_ENABLE.get(
+ config, config::getBoolean))
+ .setGrpcReadTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadMessageTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_MESSAGE_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadMetadataTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_METADATA_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadZeroCopyEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_ZEROCOPY_ENABLE.get(
+ config, config::getBoolean))
+ .setTraceLogEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_TRACE_LOG_ENABLE.get(
+ config, config::getBoolean))
+ .setTraceLogTimeThreshold(
+ GoogleHadoopFileSystemConfiguration.GCS_TRACE_LOG_TIME_THRESHOLD_MS.get(
+ config, config::getLong))
+ .build();
+ } catch (ClassNotFoundException e) {
Review Comment:
Sounds good! I removed the Hadoop dep and this parsing block; this leaves it up to the user to pass in a `GoogleCloudStorageReadOptions` constructed however they prefer 👍
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2476659801
@shunping , whenever you have a chance I'd appreciate any feedback on this!
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #32769:
URL: https://github.com/apache/beam/pull/32769#discussion_r1799664884
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java:
##########
@@ -123,6 +125,58 @@ public static GcsCountersOptions create(
}
}
+ public static class GcsReadOptionsFactory
+ implements DefaultValueFactory<GoogleCloudStorageReadOptions> {
+ @Override
+ public GoogleCloudStorageReadOptions create(PipelineOptions options) {
+ try {
+ // Check if gcs-connector-hadoop is loaded into classpath
+ Class.forName("com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration");
+ Configuration config = new Configuration();
+ return GoogleCloudStorageReadOptions.builder()
+ .setFastFailOnNotFound(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_FAST_FAIL_ON_NOT_FOUND_ENABLE
+ .get(config, config::getBoolean))
+ .setSupportGzipEncoding(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_SUPPORT_GZIP_ENCODING_ENABLE
+ .get(config, config::getBoolean))
+ .setInplaceSeekLimit(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_INPLACE_SEEK_LIMIT.get(
+ config, config::getLong))
+ .setFadvise(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_FADVISE.get(
+ config, config::getEnum))
+ .setMinRangeRequestSize(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_MIN_RANGE_REQUEST_SIZE.get(
+ config, config::getInt))
+ .setGrpcChecksumsEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_CHECKSUMS_ENABLE.get(
+ config, config::getBoolean))
+ .setGrpcReadTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadMessageTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_MESSAGE_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadMetadataTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_METADATA_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadZeroCopyEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_ZEROCOPY_ENABLE.get(
+ config, config::getBoolean))
+ .setTraceLogEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_TRACE_LOG_ENABLE.get(
+ config, config::getBoolean))
+ .setTraceLogTimeThreshold(
+ GoogleHadoopFileSystemConfiguration.GCS_TRACE_LOG_TIME_THRESHOLD_MS.get(
+ config, config::getLong))
+ .build();
+ } catch (ClassNotFoundException e) {
Review Comment:
Copy-pasted from here: https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/v2.2.25/gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemConfiguration.java#L656-L677
I think we could make a case to make that method public in a future release so we're not pulling in Hadoop explicitly here.
Or, I could omit this if/else branch entirely and always return `GoogleCloudStorageReadOptions.DEFAULT`, and leave it up to the user to supply a `GoogleCloudStorageReadOptions` instance (thus passing the Hadoop dependency down to the user-end).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2449705220
Reminder, please take a look at this pr: @damondouglas
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #32769:
URL: https://github.com/apache/beam/pull/32769#discussion_r1813224098
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java:
##########
@@ -249,17 +307,20 @@ public static boolean isWildcard(GcsPath spec) {
Credentials credentials,
@Nullable Integer uploadBufferSizeBytes,
@Nullable Integer rewriteDataOpBatchLimit,
- GcsCountersOptions gcsCountersOptions) {
+ GcsCountersOptions gcsCountersOptions,
+ GoogleCloudStorageReadOptions gcsReadOptions) {
this.storageClient = storageClient;
this.httpRequestInitializer = httpRequestInitializer;
this.uploadBufferSizeBytes = uploadBufferSizeBytes;
this.executorService = executorService;
this.credentials = credentials;
this.maxBytesRewrittenPerCall = null;
this.numRewriteTokensUsed = null;
+ this.googleCloudStorageReadOptions = gcsReadOptions;
googleCloudStorageOptions =
GoogleCloudStorageOptions.builder()
.setAppName("Beam")
+ .setReadChannelOptions(this.googleCloudStorageReadOptions)
Review Comment:
yeah `GoogleCloudStorageImpl` set up in a slightly strange way -- it's constructed with a GoogleCloudStorageOptions arg but also accepts a separate GoogleCloudStorageOptions as an argument to `open`, while ignoring the former instance variable. Dropping the separate member variable here makes sense, will do 👍
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java:
##########
@@ -249,17 +307,20 @@ public static boolean isWildcard(GcsPath spec) {
Credentials credentials,
@Nullable Integer uploadBufferSizeBytes,
@Nullable Integer rewriteDataOpBatchLimit,
- GcsCountersOptions gcsCountersOptions) {
+ GcsCountersOptions gcsCountersOptions,
+ GoogleCloudStorageReadOptions gcsReadOptions) {
this.storageClient = storageClient;
this.httpRequestInitializer = httpRequestInitializer;
this.uploadBufferSizeBytes = uploadBufferSizeBytes;
this.executorService = executorService;
this.credentials = credentials;
this.maxBytesRewrittenPerCall = null;
this.numRewriteTokensUsed = null;
+ this.googleCloudStorageReadOptions = gcsReadOptions;
googleCloudStorageOptions =
GoogleCloudStorageOptions.builder()
.setAppName("Beam")
+ .setReadChannelOptions(this.googleCloudStorageReadOptions)
Review Comment:
yeah `GoogleCloudStorageImpl` is set up in a slightly strange way -- it's constructed with a GoogleCloudStorageOptions arg but also accepts a separate GoogleCloudStorageOptions as an argument to `open`, while ignoring the former instance variable. Dropping the separate member variable here makes sense, will do 👍
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle commented on code in PR #32769:
URL: https://github.com/apache/beam/pull/32769#discussion_r1812224493
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java:
##########
@@ -123,6 +125,58 @@ public static GcsCountersOptions create(
}
}
+ public static class GcsReadOptionsFactory
+ implements DefaultValueFactory<GoogleCloudStorageReadOptions> {
+ @Override
+ public GoogleCloudStorageReadOptions create(PipelineOptions options) {
+ try {
+ // Check if gcs-connector-hadoop is loaded into classpath
+ Class.forName("com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration");
+ Configuration config = new Configuration();
+ return GoogleCloudStorageReadOptions.builder()
+ .setFastFailOnNotFound(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_FAST_FAIL_ON_NOT_FOUND_ENABLE
+ .get(config, config::getBoolean))
+ .setSupportGzipEncoding(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_SUPPORT_GZIP_ENCODING_ENABLE
+ .get(config, config::getBoolean))
+ .setInplaceSeekLimit(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_INPLACE_SEEK_LIMIT.get(
+ config, config::getLong))
+ .setFadvise(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_FADVISE.get(
+ config, config::getEnum))
+ .setMinRangeRequestSize(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_MIN_RANGE_REQUEST_SIZE.get(
+ config, config::getInt))
+ .setGrpcChecksumsEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_CHECKSUMS_ENABLE.get(
+ config, config::getBoolean))
+ .setGrpcReadTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadMessageTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_MESSAGE_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadMetadataTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_METADATA_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadZeroCopyEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_ZEROCOPY_ENABLE.get(
+ config, config::getBoolean))
+ .setTraceLogEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_TRACE_LOG_ENABLE.get(
+ config, config::getBoolean))
+ .setTraceLogTimeThreshold(
+ GoogleHadoopFileSystemConfiguration.GCS_TRACE_LOG_TIME_THRESHOLD_MS.get(
+ config, config::getLong))
+ .build();
+ } catch (ClassNotFoundException e) {
Review Comment:
> Or, I could omit this if/else branch entirely and always return GoogleCloudStorageReadOptions.DEFAULT, and leave it up to the user to supply a GoogleCloudStorageReadOptions instance (thus passing the Hadoop dependency down to the user-end).
I think this would be preferable to avoid having to pull in the other packages but asking for some other Beam maintainers more familiar with dep management etc to take a look as well.
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java:
##########
@@ -249,17 +307,20 @@ public static boolean isWildcard(GcsPath spec) {
Credentials credentials,
@Nullable Integer uploadBufferSizeBytes,
@Nullable Integer rewriteDataOpBatchLimit,
- GcsCountersOptions gcsCountersOptions) {
+ GcsCountersOptions gcsCountersOptions,
+ GoogleCloudStorageReadOptions gcsReadOptions) {
this.storageClient = storageClient;
this.httpRequestInitializer = httpRequestInitializer;
this.uploadBufferSizeBytes = uploadBufferSizeBytes;
this.executorService = executorService;
this.credentials = credentials;
this.maxBytesRewrittenPerCall = null;
this.numRewriteTokensUsed = null;
+ this.googleCloudStorageReadOptions = gcsReadOptions;
googleCloudStorageOptions =
GoogleCloudStorageOptions.builder()
.setAppName("Beam")
+ .setReadChannelOptions(this.googleCloudStorageReadOptions)
Review Comment:
This seems like a bug in `GoogleCloudStorageImpl.open` method that it doesn't use these options but you can remove the separate member variable and use `googleCloudStorageOptions.getReadChannelOptions()` in `open` below below.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Propagate gcs-connector options to GcsUtil [beam]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2514981119
## [Codecov](https://app.codecov.io/gh/apache/beam/pull/32769?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 57.39%. Comparing base [(`160ffd5`)](https://app.codecov.io/gh/apache/beam/commit/160ffd53ce0f5ac2b91f917abba42dda635841d9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`27672ab`)](https://app.codecov.io/gh/apache/beam/commit/27672abb10c9878870694714c916aa9919fee304?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 4 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #32769 +/- ##
=========================================
Coverage 57.39% 57.39%
Complexity 1474 1474
=========================================
Files 970 970
Lines 154426 154426
Branches 1076 1076
=========================================
Hits 88637 88637
Misses 63585 63585
Partials 2204 2204
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/beam/pull/32769?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2456671869
R: @shunping (XQ suggested you to help review this)
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on code in PR #32769:
URL: https://github.com/apache/beam/pull/32769#discussion_r1850391193
##########
sdks/java/extensions/google-cloud-platform-core/src/test/resources/test-hadoop-conf.xml:
##########
@@ -0,0 +1,14 @@
+<configuration>
Review Comment:
Could you add license information here? Example: https://github.com/apache/beam/blob/e1e093455acaa9a6d5b131cf5103165a661c7615/sdks/java/build-tools/src/main/resources/beam/checkstyle/checkstyle.xml#L2
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2457710979
> R: @shunping (XQ suggested you to help review this)
ack. will take a look today
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Propagate gcs-connector options to GcsUtil [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2501280641
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #32769:
URL: https://github.com/apache/beam/pull/32769#discussion_r1857111318
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java:
##########
@@ -249,17 +307,20 @@ public static boolean isWildcard(GcsPath spec) {
Credentials credentials,
@Nullable Integer uploadBufferSizeBytes,
@Nullable Integer rewriteDataOpBatchLimit,
- GcsCountersOptions gcsCountersOptions) {
+ GcsCountersOptions gcsCountersOptions,
+ GoogleCloudStorageReadOptions gcsReadOptions) {
this.storageClient = storageClient;
this.httpRequestInitializer = httpRequestInitializer;
this.uploadBufferSizeBytes = uploadBufferSizeBytes;
this.executorService = executorService;
this.credentials = credentials;
this.maxBytesRewrittenPerCall = null;
this.numRewriteTokensUsed = null;
+ this.googleCloudStorageReadOptions = gcsReadOptions;
googleCloudStorageOptions =
GoogleCloudStorageOptions.builder()
.setAppName("Beam")
+ .setReadChannelOptions(this.googleCloudStorageReadOptions)
Review Comment:
updated!
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2429127295
bumping this PR -- cc @scwhittle since I saw you recently made changes to GcsUtil?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Propagate gcs-connector options to GcsUtil [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on code in PR #32769:
URL: https://github.com/apache/beam/pull/32769#discussion_r1859695413
##########
sdks/java/extensions/google-cloud-platform-core/src/test/java/org/apache/beam/sdk/extensions/gcp/GcpCoreApiSurfaceTest.java:
##########
@@ -55,6 +55,7 @@ public void testGcpCoreApiSurface() throws Exception {
classesInPackage("com.google.api.services.storage"),
classesInPackage("com.google.auth"),
classesInPackage("com.fasterxml.jackson.annotation"),
+ classesInPackage("com.google.cloud.hadoop.gcsio"),
Review Comment:
Looks like this break a test:
https://ge.apache.org/s/5ubkbvlvrjzgi/console-log/task/:sdks:java:extensions:google-cloud-platform-core:test?anchor=5&page=1
##########
sdks/java/extensions/google-cloud-platform-core/src/test/java/org/apache/beam/sdk/extensions/gcp/GcpCoreApiSurfaceTest.java:
##########
@@ -55,6 +55,7 @@ public void testGcpCoreApiSurface() throws Exception {
classesInPackage("com.google.api.services.storage"),
classesInPackage("com.google.auth"),
classesInPackage("com.fasterxml.jackson.annotation"),
+ classesInPackage("com.google.cloud.hadoop.gcsio"),
Review Comment:
Looks like this breaks a test:
https://ge.apache.org/s/5ubkbvlvrjzgi/console-log/task/:sdks:java:extensions:google-cloud-platform-core:test?anchor=5&page=1
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Propagate gcs-connector options to GcsUtil [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2511859451
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #32769:
URL: https://github.com/apache/beam/pull/32769#discussion_r1799664884
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java:
##########
@@ -123,6 +125,58 @@ public static GcsCountersOptions create(
}
}
+ public static class GcsReadOptionsFactory
+ implements DefaultValueFactory<GoogleCloudStorageReadOptions> {
+ @Override
+ public GoogleCloudStorageReadOptions create(PipelineOptions options) {
+ try {
+ // Check if gcs-connector-hadoop is loaded into classpath
+ Class.forName("com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration");
+ Configuration config = new Configuration();
+ return GoogleCloudStorageReadOptions.builder()
+ .setFastFailOnNotFound(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_FAST_FAIL_ON_NOT_FOUND_ENABLE
+ .get(config, config::getBoolean))
+ .setSupportGzipEncoding(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_SUPPORT_GZIP_ENCODING_ENABLE
+ .get(config, config::getBoolean))
+ .setInplaceSeekLimit(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_INPLACE_SEEK_LIMIT.get(
+ config, config::getLong))
+ .setFadvise(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_FADVISE.get(
+ config, config::getEnum))
+ .setMinRangeRequestSize(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_MIN_RANGE_REQUEST_SIZE.get(
+ config, config::getInt))
+ .setGrpcChecksumsEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_CHECKSUMS_ENABLE.get(
+ config, config::getBoolean))
+ .setGrpcReadTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadMessageTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_MESSAGE_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadMetadataTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_METADATA_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadZeroCopyEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_ZEROCOPY_ENABLE.get(
+ config, config::getBoolean))
+ .setTraceLogEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_TRACE_LOG_ENABLE.get(
+ config, config::getBoolean))
+ .setTraceLogTimeThreshold(
+ GoogleHadoopFileSystemConfiguration.GCS_TRACE_LOG_TIME_THRESHOLD_MS.get(
+ config, config::getLong))
+ .build();
+ } catch (ClassNotFoundException e) {
Review Comment:
Copy-pasted from here: https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/v2.2.25/gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemConfiguration.java#L656-L677
I think we could make a case to make that method public in a future release so we're not pulling in Hadoop.
Or, I could omit this branch entirely and always return `GoogleCloudStorageReadOptions.DEFAULT`, and leave it up to the user to supply a `GoogleCloudStorageReadOptions` instance (thus passing the Hadoop dependency down to the user-end).
##########
sdks/java/extensions/google-cloud-platform-core/src/test/java/org/apache/beam/sdk/extensions/gcp/GcpCoreApiSurfaceTest.java:
##########
@@ -55,6 +55,8 @@ public void testGcpCoreApiSurface() throws Exception {
classesInPackage("com.google.api.services.storage"),
classesInPackage("com.google.auth"),
classesInPackage("com.fasterxml.jackson.annotation"),
+ classesInPackage("com.google.cloud.hadoop.gcsio"),
+ classesInPackage("com.google.common.collect"), // via GoogleCloudStorageReadOptions
Review Comment:
this is unfortunate 😓 GoogleCloudStorageReadOptions has one property that's strictly an ImmutableSet: https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/v2.2.25/gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemConfiguration.java#L519
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2411804658
assigned reviewers since at first glance, the GHA failures did not look related/might be transient? could be wrong though.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on code in PR #32769:
URL: https://github.com/apache/beam/pull/32769#discussion_r1850378211
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java:
##########
@@ -123,6 +125,58 @@ public static GcsCountersOptions create(
}
}
+ public static class GcsReadOptionsFactory
+ implements DefaultValueFactory<GoogleCloudStorageReadOptions> {
+ @Override
+ public GoogleCloudStorageReadOptions create(PipelineOptions options) {
+ try {
+ // Check if gcs-connector-hadoop is loaded into classpath
+ Class.forName("com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration");
+ Configuration config = new Configuration();
+ return GoogleCloudStorageReadOptions.builder()
+ .setFastFailOnNotFound(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_FAST_FAIL_ON_NOT_FOUND_ENABLE
+ .get(config, config::getBoolean))
+ .setSupportGzipEncoding(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_SUPPORT_GZIP_ENCODING_ENABLE
+ .get(config, config::getBoolean))
+ .setInplaceSeekLimit(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_INPLACE_SEEK_LIMIT.get(
+ config, config::getLong))
+ .setFadvise(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_FADVISE.get(
+ config, config::getEnum))
+ .setMinRangeRequestSize(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_MIN_RANGE_REQUEST_SIZE.get(
+ config, config::getInt))
+ .setGrpcChecksumsEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_CHECKSUMS_ENABLE.get(
+ config, config::getBoolean))
+ .setGrpcReadTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadMessageTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_MESSAGE_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadMetadataTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_METADATA_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadZeroCopyEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_ZEROCOPY_ENABLE.get(
+ config, config::getBoolean))
+ .setTraceLogEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_TRACE_LOG_ENABLE.get(
+ config, config::getBoolean))
+ .setTraceLogTimeThreshold(
+ GoogleHadoopFileSystemConfiguration.GCS_TRACE_LOG_TIME_THRESHOLD_MS.get(
+ config, config::getLong))
+ .build();
+ } catch (ClassNotFoundException e) {
Review Comment:
Agree with @scwhittle. It is also easier for maintainers and if new options are added, we don't need to change the code to support that.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Propagate gcs-connector options to GcsUtil [beam]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #32769:
URL: https://github.com/apache/beam/pull/32769#discussion_r1857315840
##########
sdks/java/extensions/google-cloud-platform-core/src/test/resources/test-hadoop-conf.xml:
##########
@@ -0,0 +1,14 @@
+<configuration>
Review Comment:
Ended up removing the file
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2456674874
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment `assign set of reviewers`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #32769:
URL: https://github.com/apache/beam/pull/32769#discussion_r1799664884
##########
sdks/java/extensions/google-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java:
##########
@@ -123,6 +125,58 @@ public static GcsCountersOptions create(
}
}
+ public static class GcsReadOptionsFactory
+ implements DefaultValueFactory<GoogleCloudStorageReadOptions> {
+ @Override
+ public GoogleCloudStorageReadOptions create(PipelineOptions options) {
+ try {
+ // Check if gcs-connector-hadoop is loaded into classpath
+ Class.forName("com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystemConfiguration");
+ Configuration config = new Configuration();
+ return GoogleCloudStorageReadOptions.builder()
+ .setFastFailOnNotFound(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_FAST_FAIL_ON_NOT_FOUND_ENABLE
+ .get(config, config::getBoolean))
+ .setSupportGzipEncoding(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_SUPPORT_GZIP_ENCODING_ENABLE
+ .get(config, config::getBoolean))
+ .setInplaceSeekLimit(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_INPLACE_SEEK_LIMIT.get(
+ config, config::getLong))
+ .setFadvise(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_FADVISE.get(
+ config, config::getEnum))
+ .setMinRangeRequestSize(
+ GoogleHadoopFileSystemConfiguration.GCS_INPUT_STREAM_MIN_RANGE_REQUEST_SIZE.get(
+ config, config::getInt))
+ .setGrpcChecksumsEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_CHECKSUMS_ENABLE.get(
+ config, config::getBoolean))
+ .setGrpcReadTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadMessageTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_MESSAGE_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadMetadataTimeoutMillis(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_METADATA_TIMEOUT_MS.get(
+ config, config::getLong))
+ .setGrpcReadZeroCopyEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_GRPC_READ_ZEROCOPY_ENABLE.get(
+ config, config::getBoolean))
+ .setTraceLogEnabled(
+ GoogleHadoopFileSystemConfiguration.GCS_TRACE_LOG_ENABLE.get(
+ config, config::getBoolean))
+ .setTraceLogTimeThreshold(
+ GoogleHadoopFileSystemConfiguration.GCS_TRACE_LOG_TIME_THRESHOLD_MS.get(
+ config, config::getLong))
+ .build();
+ } catch (ClassNotFoundException e) {
Review Comment:
Copy-pasted from here: https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/v2.2.25/gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemConfiguration.java#L656-L677
I think we could make a case to make that method public in a future release so we're not pulling in Hadoop explicitly here.
Or, I could omit this branch entirely and always return `GoogleCloudStorageReadOptions.DEFAULT`, and leave it up to the user to supply a `GoogleCloudStorageReadOptions` instance (thus passing the Hadoop dependency down to the user-end).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2411719059
assign set of reviewers
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2411721337
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @damondouglas for label java.
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).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Proposal: Propagate gcs-connector options to GcsUtil [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2411684082
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Propagate gcs-connector options to GcsUtil [beam]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #32769:
URL: https://github.com/apache/beam/pull/32769#discussion_r1859912269
##########
sdks/java/extensions/google-cloud-platform-core/src/test/java/org/apache/beam/sdk/extensions/gcp/GcpCoreApiSurfaceTest.java:
##########
@@ -55,6 +55,7 @@ public void testGcpCoreApiSurface() throws Exception {
classesInPackage("com.google.api.services.storage"),
classesInPackage("com.google.auth"),
classesInPackage("com.fasterxml.jackson.annotation"),
+ classesInPackage("com.google.cloud.hadoop.gcsio"),
Review Comment:
thanks for tracking that down! Pushed a fix. one of the precommit tests is still failing though:
```
org.apache.beam.runners.dataflow.worker.streaming.harness.FanOutStreamingEngineWorkerHarnessTest > testOnNewWorkerMetadata_correctlyRemovesStaleWindmillServers FAILED
java.lang.AssertionError at GrpcCleanupRule.java:201
```
Not sure if/how this could be related to my PR
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Propagate gcs-connector options to GcsUtil [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on PR #32769:
URL: https://github.com/apache/beam/pull/32769#issuecomment-2501285748
Running the failed precommit test again, though the failure seems unrelated to the code change here.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Propagate gcs-connector options to GcsUtil [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #32769:
URL: https://github.com/apache/beam/pull/32769#discussion_r1812715906
##########
CHANGES.md:
##########
@@ -69,6 +69,7 @@
* [Managed Iceberg] Now available in Python SDK ([#31495](https://github.com/apache/beam/pull/31495))
* [Managed Iceberg] Add support for TIMESTAMP, TIME, and DATE types ([#32688](https://github.com/apache/beam/pull/32688))
* BigQuery CDC writes are now available in Python SDK, only supported when using StorageWrite API at least once mode ([#32527](https://github.com/apache/beam/issues/32527))
+* gcs-connector config options can be set via GcsOptions; otherwise will be automatically loaded from default Configuration (Java)
Review Comment:
Nit: Could we link to this PR here
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Propagate gcs-connector options to GcsUtil [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm merged PR #32769:
URL: https://github.com/apache/beam/pull/32769
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org