You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by zhangminglei <gi...@git.apache.org> on 2018/01/27 13:29:43 UTC
[GitHub] flink pull request #5375: [FLINK-7095] [TaskManager] Add Command line parsin...
GitHub user zhangminglei opened a pull request:
https://github.com/apache/flink/pull/5375
[FLINK-7095] [TaskManager] Add Command line parsing tool to TaskManag…
## What is the purpose of the change
*Add a proper command line parsing tool to the entry point of the TaskManagerRunner#main.*
## Brief change log
- *Use Commons CLI as tool to implement this functionality.*
- *Remove the original way of ParameterTool.*
## Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (don't know)
- The S3 file system connector: (no)
## Documentation
- Does this pull request introduce a new feature? (yes)
- If yes, how is the feature documented? (JavaDocs)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zhangminglei/flink flink-7095
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5375.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #5375
----
commit 7db40095c16ab4f6330343efb3a9703674b194ed
Author: zhangminglei <zm...@...>
Date: 2018-01-27T13:23:37Z
[FLINK-7095] [TaskManager] Add Command line parsing tool to TaskManagerRunner.main
----
---
[GitHub] flink pull request #5375: [FLINK-7095] [TaskManager] Add Command line parsin...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/5375#discussion_r164473535
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskManagerRunner.java ---
@@ -213,10 +226,11 @@ public static void main(String[] args) throws Exception {
LOG.info("Cannot determine the maximum number of open file descriptors");
}
- ParameterTool parameterTool = ParameterTool.fromArgs(args);
-
- final String configDir = parameterTool.get("configDir");
+ // try to parse the command line arguments
+ CommandLineParser parser = new DefaultParser();
+ CommandLine cmd = parser.parse(ALL_OPTIONS, args);
+ final String configDir = cmd.getOptionValue("configDir");
--- End diff --
Have you tried whether this actually works? It looks to me as if this will always return `null`. Please write a test for it.
---
[GitHub] flink issue #5375: [FLINK-7095] [TaskManager] Add Command line parsing tool ...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/5375
I have updated the code and write a test. @tillrohrmann Thanks in advance to review those codes ~
---
[GitHub] flink issue #5375: [FLINK-7095] [TaskManager] Add Command line parsing tool ...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/5375
Hi, @tillrohrmann . You are welcome ~ I still have a lot of other flink jira will be addressed by me in the future.
---
[GitHub] flink issue #5375: [FLINK-7095] [TaskManager] Add Command line parsing tool ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/5375
Sorry for not getting back to you earlier @zhangminglei. I accidentally addressed this issue with #6318. I think we can therefore close this PR.
Sorry for the bad PR management. This won't happen again.
---
[GitHub] flink issue #5375: [FLINK-7095] [TaskManager] Add Command line parsing tool ...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/5375
Thanks @tillrohrmann . I will take a look based on your suggestions.
---
[GitHub] flink pull request #5375: [FLINK-7095] [TaskManager] Add Command line parsin...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei closed the pull request at:
https://github.com/apache/flink/pull/5375
---
[GitHub] flink issue #5375: [FLINK-7095] [TaskManager] Add Command line parsing tool ...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/5375
@tillrohrmann It seems that now there is not any ```Option``` be registered for later parse the ```configDir ``` parameter I just checked. I either register the ```configDir``` parameter in somewhere, such as in ```CliFrontendParser``` class or directly use ```TaskManager.parseArgsAndLoadConfig(args);``` to get ```Configuration```. What do you think of this ? @tillrohrmann Thanks.
---