New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BEAM-2005] Move getScheme from FileSystemRegistrar to FileSystem #2757
Conversation
* | ||
* <p>The scheme is required to be unique among all | ||
* <p>The {@link FileSystem#getScheme() scheme} is required to be unique among all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each scheme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -413,19 +408,11 @@ private static String parseScheme(String spec) { | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for scheme
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
SCHEME_TO_REGISTRAR.clear(); | ||
public static void setDefaultConfigInWorkers(PipelineOptions options) { | ||
checkNotNull(options, "options"); | ||
SCHEME_TO_FILESYSTEM.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may need locks around this now -- there's a race condition if setDefaultConfigInWorkers
is called while anything else is happening.
FWICT the Dataflow worker is the only one that initializes once at the beginning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it synchronized.
Eventually the plan is for HadoopFileSystem to return customizable schemes? |
Note that I needed to update FileSystems to instantiate the FileSystem(s) upfront instead of remembering the mapping from scheme to registrar.
Yes, the plan is for HadoopFileSystem to return customizable schemes, its just hardcoded right now since its an empty implementation. |
*/ | ||
private static void loadFileSystemRegistrars() { | ||
SCHEME_TO_REGISTRAR.clear(); | ||
public static synchronized void setDefaultConfigInWorkers(PipelineOptions options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to synchronize accessors to SCHEME_TO_FILESYSTEM
-- specifically, #getFileSystemInternal
.
…tion, remove double lookup within map
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM
Note that I needed to update FileSystems to instantiate the FileSystem(s) upfront instead of remembering the mapping from scheme to registrar during "registration" of the PipelineOptions.
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull request
mvn clean verify
. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
<Jira issue #>
in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.