[Java] Track pipeline options revision for idempotent initialization of file systems #26694
Conversation
mosche
force-pushed
the
pipeline_opts_revision
branch
from
02022b7 to
ecc7ee6
Compare
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
mosche
force-pushed
the
pipeline_opts_revision
branch
from
ecc7ee6 to
d017628
Compare
mosche
changed the title
|
Run Java_IOs_Direct PreCommit |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @robertwb for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Run Java PreCommit |
2 similar comments
|
Run Java PreCommit |
|
Run Java PreCommit |
mosche
force-pushed
the
pipeline_opts_revision
branch
from
d017628 to
d7872b9
Compare
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
|
CC: @robertwb |
There was a problem hiding this comment.
LGTM
Before merging, I'd ask @robertwb to take a look for additional comments
Also, it would be helpful to add some unit tests for this new "revision" functionality.
sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java
Show resolved
Hide resolved
mosche
force-pushed
the
pipeline_opts_revision
branch
from
d7872b9 to
f3bdc8d
Compare
| @@ -103,6 +103,18 @@ public class PipelineOptionsFactoryTest { | |||
| @Rule public TestRule restoreSystemProperties = new RestoreSystemProperties(); | |||
| @Rule public ExpectedLogs expectedLogs = ExpectedLogs.none(PipelineOptionsFactory.class); | |||
|
|
|||
| @Test | |||
| public void testRevision() { | |||
There was a problem hiding this comment.
@aromanenko-dev I've added a basic unit test
|
Run Java PreCommit |
|
Failed test of |
|
Thanks @aromanenko-dev |
Current initialization of FileSystems through
FileSystems.setDefaultPipelineOptionsis problematic and prone to race conditions, especially when triggered on deserialization ofSerializablePipelineOptions(see #18430, BEAM-14465, BEAM-14355).Particularly with
S3FileSystemthis is bad as it can easily leak resources (threads!) if used this way (see #26321).Observations
FileSystems.setDefaultPipelineOptionsfrom deserialization in SerializablePipelineOptions (SerializablePipelineOptions should not call FileSystems.setDefaultPipelineOptions. #18430) is unlikely to happen any time soon as it requires a coordinated push across various runners and it’s not obvious where and when initialization is supposed to happen for each runner. As a consequence we must expectFileSystems.setDefaultPipelineOptionsto be called any number of times.Changes in this PR
Closes #27535, fixes #26321
(cc @robertwb Thanks for your reply on the email thread)
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.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.