FLINK-2380: allow to specify the default filesystem scheme in the flink configuration file. #1524
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -52,6 +52,14 @@ The configuration files for the TaskManagers can be different, Flink does not as | |||
| - `parallelism.default`: The default parallelism to use for programs that have no parallelism specified. (DEFAULT: 1). For setups that have no concurrent jobs running, setting this value to NumTaskManagers * NumSlotsPerTaskManager will cause the system to use all available execution resources for the program's execution. **Note**: The default parallelism can be overwriten for an entire job by calling `setParallelism(int parallelism)` on the `ExecutionEnvironment` or by passing `-p <parallelism>` to the Flink Command-line frontend. It can be overwritten for single transformations by calling `setParallelism(int | |||
| parallelism)` on an operator. See the [programming guide]({{site.baseurl}}/apis/programming_guide.html#parallel-execution) for more information about the parallelism. | |||
|
|
|||
| - `fs.default-scheme`: The default filesystem scheme to be used, with the necessary authority (if needed). | |||
There was a problem hiding this comment.
What do you mean with necessary authority?
|
Hey Klou! Nice addition. A couple of users have asked for this. I guess they will be happy. I made some minor comments inline. The changes look good, I haven't tested it yet manually, but I've noticed that there are no test cases. I think we should add a simple test that ensures the basic expected behaviour (use default scheme is file, use default scheme if specified, don't use default scheme if explicit in URI etc.) After adding the tests and trying it out manually, I think it will be good to merge. :) |
kl0u
force-pushed
the
fs_param
branch
6 times, most recently
from
9aefa6f
to
2fc305b
Compare
| @@ -19,13 +19,19 @@ | |||
| package org.apache.flink.configuration; | |||
|
|
|||
| import static org.junit.Assert.assertEquals; | |||
| import static org.junit.Assert.assertTrue; | |||
There was a problem hiding this comment.
These added imports are probably not necessary
|
Thanks for picking up this issue. This goes into a very good direction.
|
|
Could you explain what more tests do you have in mind? So far I am testing 1) if the scheme provided in the configuration is used when one is not explicitly provided, 2) if an explicit scheme overrides the configuration one, and 3) if a scheme from the configuration overrides the default one. |
kl0u
force-pushed
the
fs_param
branch
4 times, most recently
from
656b17d
to
7be180c
Compare
kl0u
force-pushed
the
fs_param
branch
2 times, most recently
from
3119123
to
8233a5b
Compare
|
I'm testing the change on a cluster (with YARN) to see if everything is working as expected. |
|
I identified the following issues:
|
|
Setting the value to is good: |
|
Thanks @rmetzger for the comment. Will fix it! |
|
I have updated the PR with the new comments. |
kl0u
force-pushed
the
fs_param
branch
2 times, most recently
from
0da5fbe
to
5e20f40
Compare
|
Thank you.
I don't think putting three question marks there is a good idea. I think it implicates that the user is too stupid to properly specify a file path. the error reporting for the default scheme: I would expect a similar error reporting for |
… the flink configuration file.
|
Thanks for the comment @rmetzger. I changed the error message. Please review and let me know. |
| @@ -176,6 +184,8 @@ abstract class ApplicationMasterBase { | |||
| jobManagerPort, webServerPort, slots, taskManagerCount, | |||
| dynamicPropertiesEncodedString) | |||
|
|
|||
| //todo should I also set the FS default here???? | |||
There was a problem hiding this comment.
No. I'll remove this TODO when merging
There was a problem hiding this comment.
@rmetzger Yes I know. That comment was forgotten since earlier.
|
I tested the change again on a cluster. Everything is working nicely with YARN. I'll merge the PR. |
|
Perfect! Thanks a lot @rmetzger |
subhankarb
pushed a commit
to subhankarb/flink
that referenced
this pull request
Mar 17, 2016
… the flink configuration file. This closes apache#1524
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.