You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by alkagin <gi...@git.apache.org> on 2016/06/23 09:56:17 UTC
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
GitHub user alkagin opened a pull request:
https://github.com/apache/flink/pull/2149
[FLINK-4084] Add configDir parameter to CliFrontend and flink shell script
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
In addition to going through the list, please provide a meaningful description of your changes.
- [ X] General
- The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
- [X ] Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
- [X ] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
Modified flink shell script and CliFrontend to provide an option `configDir` to specify a custom configuration folder.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/alkagin/flink FLINK-4084-b
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2149.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 #2149
----
commit 29bfc57df566593678bf99df826412cd44a07589
Author: Andrea Sella <an...@radicalbit.io>
Date: 2016-06-21T12:13:58Z
[FLINK-4084] --configDir shell script
commit fa65db88207d4c8d8319a075d5c05debf8a278e2
Author: Andrea Sella <an...@radicalbit.io>
Date: 2016-06-21T15:07:50Z
[FLINK-4084] Add configDir in CliFrontendParser
commit c00970d581aeda1eff9800dadf1c9505f4809338
Author: Andrea Sella <an...@radicalbit.io>
Date: 2016-06-22T11:07:03Z
[FLINK-4084] check if configDir value is a directory
commit 0e15c068fe691bdcb79ae214892430129f5588d4
Author: Andrea Sella <an...@radicalbit.io>
Date: 2016-06-22T12:50:06Z
[FLINK-4084] add abspath to bin/flink, renamed CONFIGURATION to CONFIGDIR
commit 4b35798d9bc989296ee862101b04537efc67521c
Author: Andrea Sella <an...@radicalbit.io>
Date: 2016-06-22T13:26:45Z
[FLINK-4084] typo in CliFrontendRunTest
commit 3dfe7a56904191948d1b5b30a17198a7201294f4
Author: Andrea Sella <an...@radicalbit.io>
Date: 2016-06-22T14:59:12Z
[FLINK-4084] Read configDir before CliFrontend's initialization
commit a838ebca1af00b2e7d541b113f0450fe18eb8463
Author: Andrea Sella <an...@radicalbit.io>
Date: 2016-06-23T08:12:56Z
[FLINK-4084] --configDir enabled just for run action
commit 047ecc17d61189f065141f12d1a391e9bb0f63c0
Author: Andrea Sella <an...@radicalbit.io>
Date: 2016-06-23T08:59:39Z
[FLINK-4084] format issue: add space after if
commit bb9203923edeb02258455712db579d09dd3e7e62
Author: Andrea Sella <an...@radicalbit.io>
Date: 2016-06-23T09:51:15Z
[FLINK-4084] Add configDir in docs
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by alkagin <gi...@git.apache.org>.
Github user alkagin commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68937366
--- Diff: flink-clients/src/main/java/org/apache/flink/client/CliFrontend.java ---
@@ -132,9 +132,9 @@
- private final Configuration config;
+ private Configuration config;
--- End diff --
> Could we make the configDir parameter a general parameter that goes before run, list, cancel, etc.?
Sounds good. `configDir` must be positional after `flink` or I may do something like this `flink run --configDir /my/custom/dir -p 10 /path/to/jar`?
> You could parse the config parameter in the constructor and initialize the config only once. Then continue with the normal parsing.
Now `configDir` is traversal on all actions, i can do it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68550234
--- Diff: flink-clients/src/main/java/org/apache/flink/client/CliFrontend.java ---
@@ -132,9 +132,9 @@
- private final Configuration config;
+ private Configuration config;
- private final FiniteDuration clientTimeout;
+ private FiniteDuration clientTimeout;
--- End diff --
Same as above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68550224
--- Diff: flink-clients/src/main/java/org/apache/flink/client/CliFrontend.java ---
@@ -132,9 +132,9 @@
- private final Configuration config;
+ private Configuration config;
--- End diff --
Why did you remove the `final` flag here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r71354207
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontendParser.java ---
@@ -451,4 +479,25 @@ public static InfoOptions parseInfoCommand(String[] args) throws CliArgsExceptio
}
}
+ public static MainOptions parseMainCommand(String[] args) throws CliArgsException {
+
+ // drop all arguments after an action
+ final List<String> params= Arrays.asList(args);
+ for (String action: CliFrontend.ACTIONS) {
+ int index = params.indexOf(action);
+ if(index != -1) {
+ args = Arrays.copyOfRange(args, 0, index);
--- End diff --
I think dropping the args is not necessary if you use the parsing below.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68742191
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,20 +17,41 @@
# limitations under the License.
################################################################################
-target="$0"
# For the case, the executable has been directly symlinked, figure out
# the correct bin path by following its symlink up to an upper bound.
# Note: we can't use the readlink utility here if we want to be POSIX
# compatible.
-iteration=0
-while [ -L "$target" ]; do
- if [ "$iteration" -gt 100 ]; then
- echo "Cannot resolve path: You have a cyclic symlink in $target."
+followSymLink() {
+ local iteration=0
+ local bar=$1
+ while [ -L "$bar" ]; do
+ if [ "$iteration" -gt 100 ]; then
+ echo "Cannot resolve path: You have a cyclic symlink in $bar."
+ break
+ fi
+ ls=`ls -ld -- "$bar"`
+ bar=`expr "$ls" : '.* -> \(.*\)$'`
+ iteration=$((iteration + 1))
+ done
+
+ echo "$bar"
+}
+
+target=$(followSymLink "$0")
+
+#Search --configDir into the parameters and set it as FLINK_CONF_DIR
+args=("$@")
+for i in "${!args[@]}"; do
+ if [ ${args[$i]} = "--configDir" ]; then
+ dir=$(followSymLink "${args[$(($i+1))]}" )
--- End diff --
Please add a space after `$(` or remove the space after `}"`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68550807
--- Diff: flink-clients/src/test/java/org/apache/flink/client/CliFrontendRunTest.java ---
@@ -98,6 +98,12 @@ public void testRun() {
RunOptions options = CliFrontendParser.parseRunCommand(parameters);
assertEquals("expectedSavepointPath", options.getSavepointPath());
}
+ // test configure configDir
+ {
+ String[] parameters = {"-D", "expectedConfigDirectory", getTestJarPath()};
--- End diff --
Both long and short option should be tested (though I think we can remove the short option).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r69123278
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontendParser.java ---
@@ -459,4 +476,14 @@ public static InfoOptions parseInfoCommand(String[] args) throws CliArgsExceptio
}
}
+ public static MainOptions parseMainCommand(String[] args) throws CliArgsException {
+ try {
+ DefaultParser parser = new DefaultParser();
+ CommandLine line = parser.parse(MAIN_OPTIONS, args, false);
--- End diff --
You're parsing the entire arguments here. What if the user jar has `--configDir` as parameter?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68550160
--- Diff: docs/apis/cli.md ---
@@ -187,6 +187,8 @@ Action "run" compiles and runs a program.
java.net.URLClassLoader}.
-d,--detached If present, runs the job in detached
mode
+ -D,--configDir </path/to/confdir> The configuration directory with which
--- End diff --
`-D`? I think that can be easily confused. We don't have to have a short option.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2149: [FLINK-4084] Add configDir parameter to CliFrontend and f...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the issue:
https://github.com/apache/flink/pull/2149
Thanks for the update. We're getting there :) Great work so far!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2149: [FLINK-4084] Add configDir parameter to CliFrontend and f...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the issue:
https://github.com/apache/flink/pull/2149
Thanks for the pull request! I've made some comments. The pull requests also needs to be rebased against the latest master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r69101922
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,20 +17,41 @@
# limitations under the License.
################################################################################
-target="$0"
# For the case, the executable has been directly symlinked, figure out
# the correct bin path by following its symlink up to an upper bound.
# Note: we can't use the readlink utility here if we want to be POSIX
# compatible.
-iteration=0
-while [ -L "$target" ]; do
- if [ "$iteration" -gt 100 ]; then
- echo "Cannot resolve path: You have a cyclic symlink in $target."
+followSymLink() {
+ local iteration=0
+ local bar=$1
+ while [ -L "$bar" ]; do
+ if [ "$iteration" -gt 100 ]; then
+ echo "Cannot resolve path: You have a cyclic symlink in $bar."
+ break
+ fi
+ ls=`ls -ld -- "$bar"`
+ bar=`expr "$ls" : '.* -> \(.*\)$'`
+ iteration=$((iteration + 1))
+ done
+
+ echo "$bar"
+}
+
+target=$(followSymLink "$0")
+
+#Search --configDir into the parameters and set it as FLINK_CONF_DIR
+args=("$@")
+for i in "${!args[@]}"; do
+ if [ ${args[$i]} = "--configDir" ]; then
--- End diff --
You're right, it's not necessary.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r79157293
--- Diff: docs/apis/cli.md ---
@@ -187,6 +187,8 @@ Action "run" compiles and runs a program.
java.net.URLClassLoader}.
-d,--detached If present, runs the job in detached
mode
+ --configDir </path/to/confdir> The configuration directory with which
--- End diff --
Ah ok. Looks good. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r69101854
--- Diff: flink-clients/src/main/java/org/apache/flink/client/CliFrontend.java ---
@@ -132,9 +132,9 @@
- private final Configuration config;
+ private Configuration config;
--- End diff --
Let's make it positional to prevent confusion.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68742325
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,20 +17,41 @@
# limitations under the License.
################################################################################
-target="$0"
# For the case, the executable has been directly symlinked, figure out
# the correct bin path by following its symlink up to an upper bound.
# Note: we can't use the readlink utility here if we want to be POSIX
# compatible.
-iteration=0
-while [ -L "$target" ]; do
- if [ "$iteration" -gt 100 ]; then
- echo "Cannot resolve path: You have a cyclic symlink in $target."
+followSymLink() {
+ local iteration=0
+ local bar=$1
+ while [ -L "$bar" ]; do
+ if [ "$iteration" -gt 100 ]; then
+ echo "Cannot resolve path: You have a cyclic symlink in $bar."
+ break
+ fi
+ ls=`ls -ld -- "$bar"`
+ bar=`expr "$ls" : '.* -> \(.*\)$'`
+ iteration=$((iteration + 1))
+ done
+
+ echo "$bar"
+}
+
+target=$(followSymLink "$0")
+
+#Search --configDir into the parameters and set it as FLINK_CONF_DIR
+args=("$@")
+for i in "${!args[@]}"; do
+ if [ ${args[$i]} = "--configDir" ]; then
+ dir=$(followSymLink "${args[$(($i+1))]}" )
+ if [ -d "$dir" ]; then
+ FLINK_CONF_DIR=`cd "${dir}"; pwd -P`
--- End diff --
Could we replace this with `FLINK_CONF_DIR=$(cd "${dir}" && pwd -P)`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r71355731
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,21 +17,39 @@
# limitations under the License.
################################################################################
-target="$0"
# For the case, the executable has been directly symlinked, figure out
# the correct bin path by following its symlink up to an upper bound.
# Note: we can't use the readlink utility here if we want to be POSIX
# compatible.
-iteration=0
-while [ -L "$target" ]; do
- if [ "$iteration" -gt 100 ]; then
- echo "Cannot resolve path: You have a cyclic symlink in $target."
- break
+followSymLink() {
+ local iteration=0
+ local target=$1
+ while [ -L "$target" ]; do
+ if [ "$iteration" -gt 100 ]; then
+ echo "Cannot resolve path: You have a cyclic symlink in $target."
+ break
+ fi
+ ls=`ls -ld -- "$target"`
+ target=`expr "$ls" : '.* -> \(.*\)$'`
+ iteration=$((iteration + 1))
+ done
+
+ echo "$target"
+}
+
+target=$(followSymLink "$0")
+
+#Check if --configDir is present and set is value as FLINK_CONF_DIR
+if [ "$1" = "--configDir" ]; then
--- End diff --
Here you assume that `--configDir` is the first parameter but later on you parse for it until you find the action command. Why do you set both the environment variable and pass it on as a parameter?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68741918
--- Diff: flink-clients/src/test/java/org/apache/flink/client/CliFrontendRunTest.java ---
@@ -100,15 +100,22 @@ public void testRun() {
}
// test jar arguments
+ {
+ String[] parameters =
+ {"-m", "localhost:6123", getTestJarPath(), "-arg1", "value1", "justavalue", "--arg2", "value2"};
+ RunOptions options = CliFrontendParser.parseRunCommand(parameters);
+ assertEquals("-arg1", options.getProgramArgs()[0]);
+ assertEquals("value1", options.getProgramArgs()[1]);
+ assertEquals("justavalue", options.getProgramArgs()[2]);
+ assertEquals("--arg2", options.getProgramArgs()[3]);
+ assertEquals("value2", options.getProgramArgs()[4]);
--- End diff --
Tabs have been converted to spaces here. That's why the diff looks odd. Please use tabs for indention.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r71353752
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontendParser.java ---
@@ -451,4 +479,25 @@ public static InfoOptions parseInfoCommand(String[] args) throws CliArgsExceptio
}
}
+ public static MainOptions parseMainCommand(String[] args) throws CliArgsException {
+
+ // drop all arguments after an action
+ final List<String> params= Arrays.asList(args);
--- End diff --
space missing
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2149: [FLINK-4084] Add configDir parameter to CliFrontend and f...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the issue:
https://github.com/apache/flink/pull/2149
@alkagin I've been on vacations. Getting back to you soon!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r79157764
--- Diff: docs/apis/cli.md ---
@@ -187,6 +187,8 @@ Action "run" compiles and runs a program.
java.net.URLClassLoader}.
-d,--detached If present, runs the job in detached
mode
+ --configDir </path/to/confdir> The configuration directory with which
--- End diff --
Actually, it should be aligned and `configDir` is not part of the run options.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68550677
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontendParser.java ---
@@ -76,6 +76,9 @@
static final Option SAVEPOINT_DISPOSE_OPTION = new Option("d", "dispose", true,
"Disposes an existing savepoint.");
+ static final Option CONFIGDIR_OPTION = new Option("D", "configDir", true,
--- End diff --
I would be for removing the short option because it can easily be confused with `-d`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2149: [FLINK-4084] Add configDir parameter to CliFrontend and f...
Posted by alkagin <gi...@git.apache.org>.
Github user alkagin commented on the issue:
https://github.com/apache/flink/pull/2149
I am on vacations, I will address your comments as soon as I get back.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r69123467
--- Diff: flink-clients/src/test/java/org/apache/flink/client/CliFrontendMainTest.java ---
@@ -0,0 +1,35 @@
+package org.apache.flink.client;
+
+
+import org.apache.flink.client.cli.CliArgsException;
+import org.apache.flink.client.cli.CliFrontendParser;
+import org.apache.flink.client.cli.MainOptions;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.FileNotFoundException;
+import java.net.MalformedURLException;
+
+import static org.apache.flink.client.CliFrontendTestUtils.getTestJarPath;
+import static org.junit.Assert.assertEquals;
+
+public class CliFrontendMainTest {
+
+
+ @BeforeClass
+ public static void init() {
+ CliFrontendTestUtils.pipeSystemOutToNull();
+ CliFrontendTestUtils.clearGlobalConfiguration();
+ }
+
+ @Test
+ public void testMain() throws CliArgsException, FileNotFoundException, MalformedURLException {
+ // test configure configDir
+ {
+ String[] parameters = {"--configDir", "expectedConfigDirectory", getTestJarPath()};
--- End diff --
i.e. there is no "action" before the jar.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68746527
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,20 +17,41 @@
# limitations under the License.
################################################################################
-target="$0"
# For the case, the executable has been directly symlinked, figure out
# the correct bin path by following its symlink up to an upper bound.
# Note: we can't use the readlink utility here if we want to be POSIX
# compatible.
-iteration=0
-while [ -L "$target" ]; do
- if [ "$iteration" -gt 100 ]; then
- echo "Cannot resolve path: You have a cyclic symlink in $target."
+followSymLink() {
+ local iteration=0
+ local bar=$1
+ while [ -L "$bar" ]; do
+ if [ "$iteration" -gt 100 ]; then
+ echo "Cannot resolve path: You have a cyclic symlink in $bar."
+ break
+ fi
+ ls=`ls -ld -- "$bar"`
+ bar=`expr "$ls" : '.* -> \(.*\)$'`
+ iteration=$((iteration + 1))
+ done
+
+ echo "$bar"
+}
+
+target=$(followSymLink "$0")
+
+#Search --configDir into the parameters and set it as FLINK_CONF_DIR
+args=("$@")
+for i in "${!args[@]}"; do
+ if [ ${args[$i]} = "--configDir" ]; then
+ dir=$(followSymLink "${args[$(($i+1))]}" )
+ if [ -d "$dir" ]; then
+ FLINK_CONF_DIR=`cd "${dir}"; pwd -P`
--- End diff --
I suppose it doesn't matter much but I like the chained version better.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68741598
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,20 +17,41 @@
# limitations under the License.
################################################################################
-target="$0"
# For the case, the executable has been directly symlinked, figure out
# the correct bin path by following its symlink up to an upper bound.
# Note: we can't use the readlink utility here if we want to be POSIX
# compatible.
-iteration=0
-while [ -L "$target" ]; do
- if [ "$iteration" -gt 100 ]; then
- echo "Cannot resolve path: You have a cyclic symlink in $target."
+followSymLink() {
+ local iteration=0
+ local bar=$1
+ while [ -L "$bar" ]; do
+ if [ "$iteration" -gt 100 ]; then
+ echo "Cannot resolve path: You have a cyclic symlink in $bar."
+ break
+ fi
+ ls=`ls -ld -- "$bar"`
+ bar=`expr "$ls" : '.* -> \(.*\)$'`
+ iteration=$((iteration + 1))
+ done
+
+ echo "$bar"
+}
+
+target=$(followSymLink "$0")
+
+#Search --configDir into the parameters and set it as FLINK_CONF_DIR
+args=("$@")
+for i in "${!args[@]}"; do
+ if [[ ${args[$i]} = "--configDir" || ${args[$i]} = "-D" ]]; then
+ dir=$(followSymLink "${args[$(($i+1))]}" )
+ if [ -d "$dir" ]; then
+ FLINK_CONF_DIR=`cd "${dir}"; pwd -P`
--- End diff --
Can we make this `FLINK_CONF_DIR=$(cd "${dir}" && pwd -P)`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by chobeat <gi...@git.apache.org>.
Github user chobeat commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r77777389
--- Diff: docs/apis/cli.md ---
@@ -187,6 +187,8 @@ Action "run" compiles and runs a program.
java.net.URLClassLoader}.
-d,--detached If present, runs the job in detached
mode
+ --configDir </path/to/confdir> The configuration directory with which
--- End diff --
This is a space, I checked. Do you want it to be aligned to the beginning of the other lines or is it ok like it is?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r71354132
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontendParser.java ---
@@ -451,4 +479,25 @@ public static InfoOptions parseInfoCommand(String[] args) throws CliArgsExceptio
}
}
+ public static MainOptions parseMainCommand(String[] args) throws CliArgsException {
+
+ // drop all arguments after an action
+ final List<String> params= Arrays.asList(args);
+ for (String action: CliFrontend.ACTIONS) {
+ int index = params.indexOf(action);
+ if(index != -1) {
+ args = Arrays.copyOfRange(args, 0, index);
+ break;
+ }
+ }
+
+ try {
+ DefaultParser parser = new DefaultParser();
+ CommandLine line = parser.parse(MAIN_OPTIONS, args, false);
--- End diff --
Actually you can use `CommandLine line = parser.parse(MAIN_OPTIONS, args, true);` to halt once you reach a non-arg.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by alkagin <gi...@git.apache.org>.
Github user alkagin commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68568030
--- Diff: flink-clients/src/main/java/org/apache/flink/client/CliFrontend.java ---
@@ -132,9 +132,9 @@
- private final Configuration config;
+ private Configuration config;
--- End diff --
I removed final from clientTimeout and config because I need to reassign them in [CliFrontend.java#L219](https://github.com/alkagin/flink/blob/bb9203923edeb02258455712db579d09dd3e7e62/flink-clients/src/main/java/org/apache/flink/client/CliFrontend.java#L219), as far as I understand `--configDir` option must be available just in `run` mode. Is it correct? Otherwise I can read it in the main and instantiate correctly CliFrontend passing the configDir.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by alkagin <gi...@git.apache.org>.
Github user alkagin commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68743195
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,20 +17,41 @@
# limitations under the License.
################################################################################
-target="$0"
# For the case, the executable has been directly symlinked, figure out
# the correct bin path by following its symlink up to an upper bound.
# Note: we can't use the readlink utility here if we want to be POSIX
# compatible.
-iteration=0
-while [ -L "$target" ]; do
- if [ "$iteration" -gt 100 ]; then
- echo "Cannot resolve path: You have a cyclic symlink in $target."
+followSymLink() {
+ local iteration=0
+ local bar=$1
+ while [ -L "$bar" ]; do
+ if [ "$iteration" -gt 100 ]; then
+ echo "Cannot resolve path: You have a cyclic symlink in $bar."
+ break
+ fi
+ ls=`ls -ld -- "$bar"`
+ bar=`expr "$ls" : '.* -> \(.*\)$'`
+ iteration=$((iteration + 1))
+ done
+
+ echo "$bar"
+}
+
+target=$(followSymLink "$0")
+
+#Search --configDir into the parameters and set it as FLINK_CONF_DIR
+args=("$@")
+for i in "${!args[@]}"; do
+ if [ ${args[$i]} = "--configDir" ]; then
+ dir=$(followSymLink "${args[$(($i+1))]}" )
+ if [ -d "$dir" ]; then
+ FLINK_CONF_DIR=`cd "${dir}"; pwd -P`
--- End diff --
I found a similar command [here](https://github.com/alkagin/flink/blob/76283d3f0fbf5454b208ba1887d847acea09640d/flink-dist/src/main/flink-bin/bin/config.sh#L132).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r69123415
--- Diff: flink-clients/src/test/java/org/apache/flink/client/CliFrontendMainTest.java ---
@@ -0,0 +1,35 @@
+package org.apache.flink.client;
+
+
+import org.apache.flink.client.cli.CliArgsException;
+import org.apache.flink.client.cli.CliFrontendParser;
+import org.apache.flink.client.cli.MainOptions;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.FileNotFoundException;
+import java.net.MalformedURLException;
+
+import static org.apache.flink.client.CliFrontendTestUtils.getTestJarPath;
+import static org.junit.Assert.assertEquals;
+
+public class CliFrontendMainTest {
+
+
+ @BeforeClass
+ public static void init() {
+ CliFrontendTestUtils.pipeSystemOutToNull();
+ CliFrontendTestUtils.clearGlobalConfiguration();
+ }
+
+ @Test
+ public void testMain() throws CliArgsException, FileNotFoundException, MalformedURLException {
+ // test configure configDir
+ {
+ String[] parameters = {"--configDir", "expectedConfigDirectory", getTestJarPath()};
--- End diff --
This is not a valid invocation of the CliFrontend.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by alkagin <gi...@git.apache.org>.
Github user alkagin commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68750010
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,20 +17,41 @@
# limitations under the License.
################################################################################
-target="$0"
# For the case, the executable has been directly symlinked, figure out
# the correct bin path by following its symlink up to an upper bound.
# Note: we can't use the readlink utility here if we want to be POSIX
# compatible.
-iteration=0
-while [ -L "$target" ]; do
- if [ "$iteration" -gt 100 ]; then
- echo "Cannot resolve path: You have a cyclic symlink in $target."
+followSymLink() {
+ local iteration=0
+ local bar=$1
+ while [ -L "$bar" ]; do
+ if [ "$iteration" -gt 100 ]; then
+ echo "Cannot resolve path: You have a cyclic symlink in $bar."
+ break
+ fi
+ ls=`ls -ld -- "$bar"`
+ bar=`expr "$ls" : '.* -> \(.*\)$'`
+ iteration=$((iteration + 1))
+ done
+
+ echo "$bar"
+}
+
+target=$(followSymLink "$0")
+
+#Search --configDir into the parameters and set it as FLINK_CONF_DIR
+args=("$@")
+for i in "${!args[@]}"; do
+ if [ ${args[$i]} = "--configDir" ]; then
+ dir=$(followSymLink "${args[$(($i+1))]}" )
+ if [ -d "$dir" ]; then
+ FLINK_CONF_DIR=`cd "${dir}"; pwd -P`
--- End diff --
Added chained version, the second command runs only if the first was successful; i think it is a more correct behaviour.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68910391
--- Diff: flink-clients/src/main/java/org/apache/flink/client/CliFrontend.java ---
@@ -132,9 +132,9 @@
- private final Configuration config;
+ private Configuration config;
--- End diff --
Could we make the `configDir` parameter a general parameter that goes before `run`, `list`, `cancel`, etc.? Don't know if that is too tricky but it would make sense because it is a general configuration key.
`flink --configDir /my/custom/dir run -p 10 /path/to/jar`
That way the config parameter wouldn't have to be listed for every action. Would make the help look less verbose.
You could parse the config parameter in the constructor and initialize the config only once. Then continue with the normal parsing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r71354329
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/ProgramOptions.java ---
@@ -148,4 +148,5 @@ public boolean getDetachedMode() {
public String getSavepointPath() {
return savepointPath;
}
+
--- End diff --
The changes in this file are not necessary. Could you revert?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r69123376
--- Diff: flink-clients/src/test/java/org/apache/flink/client/CliFrontendMainTest.java ---
@@ -0,0 +1,35 @@
+package org.apache.flink.client;
+
+
+import org.apache.flink.client.cli.CliArgsException;
+import org.apache.flink.client.cli.CliFrontendParser;
+import org.apache.flink.client.cli.MainOptions;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.FileNotFoundException;
+import java.net.MalformedURLException;
+
+import static org.apache.flink.client.CliFrontendTestUtils.getTestJarPath;
+import static org.junit.Assert.assertEquals;
+
+public class CliFrontendMainTest {
+
+
+ @BeforeClass
+ public static void init() {
+ CliFrontendTestUtils.pipeSystemOutToNull();
+ CliFrontendTestUtils.clearGlobalConfiguration();
+ }
+
+ @Test
+ public void testMain() throws CliArgsException, FileNotFoundException, MalformedURLException {
+ // test configure configDir
+ {
+ String[] parameters = {"--configDir", "expectedConfigDirectory", getTestJarPath()};
+ MainOptions options = CliFrontendParser.parseMainCommand(parameters);
+ assertEquals("expectedConfigDirectory", options.getConfigDir());
+ }
+ }
--- End diff --
We need more tests here to assure the parameter is correctly parsed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r79156877
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontendParser.java ---
@@ -241,8 +255,11 @@ private static Options getSavepointOptionsWithoutDeprecatedOptions(Options optio
* Prints the help for the client.
*/
public static void printHelp() {
- System.out.println("./flink <ACTION> [OPTIONS] [ARGUMENTS]");
+ System.out.println("./flink [CONFIGDIR] <ACTION> [ACTION-OPTIONS] [ARGUMENTS]");
--- End diff --
Shouldn't this be `./flink [--configDir <configDir>] <ACTION> [ACTION-OPTIONS] [ARGUMENTS]`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r71354777
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,21 +17,39 @@
# limitations under the License.
--- End diff --
You change the mode from 644 to 755.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by alkagin <gi...@git.apache.org>.
Github user alkagin commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68567354
--- Diff: docs/apis/cli.md ---
@@ -187,6 +187,8 @@ Action "run" compiles and runs a program.
java.net.URLClassLoader}.
-d,--detached If present, runs the job in detached
mode
+ -D,--configDir </path/to/confdir> The configuration directory with which
--- End diff --
I totally agree, I've added a short option 'cause I think it was required. I will remove asap
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68551023
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,6 +17,20 @@
# limitations under the License.
################################################################################
+function abspath() {
+ if [ -d "$1" ]; then
+ # dir
+ (cd "$1"; pwd)
+ elif [ -f "$1" ]; then
+ # file
+ if [[ $1 == */* ]]; then
+ echo "$(cd "${1%/*}"; pwd)/${1##*/}"
+ else
+ echo "$(pwd)/$1"
--- End diff --
Have you looked in `config.sh`? There is a function for resolving symbolic links. What happens in case of cyclic symbolic links?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r71332501
--- Diff: docs/apis/cli.md ---
@@ -187,6 +187,8 @@ Action "run" compiles and runs a program.
java.net.URLClassLoader}.
-d,--detached If present, runs the job in detached
mode
+ --configDir </path/to/confdir> The configuration directory with which
--- End diff --
I think there is a tab, we use only spaces for formatting here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68741402
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,20 +17,41 @@
# limitations under the License.
################################################################################
-target="$0"
# For the case, the executable has been directly symlinked, figure out
# the correct bin path by following its symlink up to an upper bound.
# Note: we can't use the readlink utility here if we want to be POSIX
# compatible.
-iteration=0
-while [ -L "$target" ]; do
- if [ "$iteration" -gt 100 ]; then
- echo "Cannot resolve path: You have a cyclic symlink in $target."
+followSymLink() {
+ local iteration=0
+ local bar=$1
+ while [ -L "$bar" ]; do
+ if [ "$iteration" -gt 100 ]; then
+ echo "Cannot resolve path: You have a cyclic symlink in $bar."
+ break
+ fi
+ ls=`ls -ld -- "$bar"`
+ bar=`expr "$ls" : '.* -> \(.*\)$'`
+ iteration=$((iteration + 1))
+ done
+
+ echo "$bar"
+}
+
+target=$(followSymLink "$0")
+
+#Search --configDir into the parameters and set it as FLINK_CONF_DIR
+args=("$@")
+for i in "${!args[@]}"; do
+ if [[ ${args[$i]} = "--configDir" || ${args[$i]} = "-D" ]]; then
--- End diff --
We still have the old `-D` parameter here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by alkagin <gi...@git.apache.org>.
Github user alkagin commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68742339
--- Diff: flink-clients/src/test/java/org/apache/flink/client/CliFrontendRunTest.java ---
@@ -100,15 +100,22 @@ public void testRun() {
}
// test jar arguments
+ {
+ String[] parameters =
+ {"-m", "localhost:6123", getTestJarPath(), "-arg1", "value1", "justavalue", "--arg2", "value2"};
+ RunOptions options = CliFrontendParser.parseRunCommand(parameters);
+ assertEquals("-arg1", options.getProgramArgs()[0]);
+ assertEquals("value1", options.getProgramArgs()[1]);
+ assertEquals("justavalue", options.getProgramArgs()[2]);
+ assertEquals("--arg2", options.getProgramArgs()[3]);
+ assertEquals("value2", options.getProgramArgs()[4]);
--- End diff --
Damn intellij upgrade, it should reset config style.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r71354529
--- Diff: flink-clients/src/test/java/org/apache/flink/client/CliFrontendRunTest.java ---
@@ -110,6 +110,8 @@ public void testRun() {
assertEquals("--arg2", options.getProgramArgs()[3]);
assertEquals("value2", options.getProgramArgs()[4]);
}
+
+
--- End diff --
Not necessary change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68551210
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -33,6 +47,22 @@ while [ -L "$target" ]; do
iteration=$((iteration + 1))
done
+
+#Search --configDir into the parameters and set it as FLINK_CONF_DIR
+args=("$@")
+for i in "${!args[@]}"; do
+ if [[ ${args[$i]} = "--configDir" || ${args[$i]} = "-D" ]]; then
+ dir=${args[$(($i+1))]}
+ if [ -d "$dir" ]; then
+ FLINK_CONF_DIR= abspath ${dir}
--- End diff --
Should never assign to the variable because of the space after `=`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r71354413
--- Diff: flink-clients/src/test/java/org/apache/flink/client/CliFrontendMainTest.java ---
@@ -0,0 +1,39 @@
+package org.apache.flink.client;
+
+
+import org.apache.flink.client.cli.CliArgsException;
+import org.apache.flink.client.cli.CliFrontendParser;
+import org.apache.flink.client.cli.MainOptions;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.FileNotFoundException;
+import java.net.MalformedURLException;
+
+import static org.apache.flink.client.CliFrontendTestUtils.getTestJarPath;
+import static org.junit.Assert.assertEquals;
+
+public class CliFrontendMainTest {
--- End diff --
Maybe `CliFrontendMainArgsTest`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by alkagin <gi...@git.apache.org>.
Github user alkagin commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68743480
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,20 +17,41 @@
# limitations under the License.
################################################################################
-target="$0"
# For the case, the executable has been directly symlinked, figure out
# the correct bin path by following its symlink up to an upper bound.
# Note: we can't use the readlink utility here if we want to be POSIX
# compatible.
-iteration=0
-while [ -L "$target" ]; do
- if [ "$iteration" -gt 100 ]; then
- echo "Cannot resolve path: You have a cyclic symlink in $target."
+followSymLink() {
+ local iteration=0
+ local bar=$1
+ while [ -L "$bar" ]; do
+ if [ "$iteration" -gt 100 ]; then
+ echo "Cannot resolve path: You have a cyclic symlink in $bar."
+ break
+ fi
+ ls=`ls -ld -- "$bar"`
+ bar=`expr "$ls" : '.* -> \(.*\)$'`
+ iteration=$((iteration + 1))
+ done
+
+ echo "$bar"
+}
+
+target=$(followSymLink "$0")
+
+#Search --configDir into the parameters and set it as FLINK_CONF_DIR
+args=("$@")
+for i in "${!args[@]}"; do
+ if [ ${args[$i]} = "--configDir" ]; then
--- End diff --
There is already one, we need two?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68742138
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,20 +17,41 @@
# limitations under the License.
################################################################################
-target="$0"
# For the case, the executable has been directly symlinked, figure out
# the correct bin path by following its symlink up to an upper bound.
# Note: we can't use the readlink utility here if we want to be POSIX
# compatible.
-iteration=0
-while [ -L "$target" ]; do
- if [ "$iteration" -gt 100 ]; then
- echo "Cannot resolve path: You have a cyclic symlink in $target."
+followSymLink() {
+ local iteration=0
+ local bar=$1
+ while [ -L "$bar" ]; do
+ if [ "$iteration" -gt 100 ]; then
+ echo "Cannot resolve path: You have a cyclic symlink in $bar."
+ break
+ fi
+ ls=`ls -ld -- "$bar"`
+ bar=`expr "$ls" : '.* -> \(.*\)$'`
+ iteration=$((iteration + 1))
+ done
+
+ echo "$bar"
+}
+
+target=$(followSymLink "$0")
+
+#Search --configDir into the parameters and set it as FLINK_CONF_DIR
+args=("$@")
+for i in "${!args[@]}"; do
+ if [ ${args[$i]} = "--configDir" ]; then
--- End diff --
I think this needs another space after `]`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2149: [FLINK-4084] Add configDir parameter to CliFronten...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2149#discussion_r68742023
--- Diff: flink-dist/src/main/flink-bin/bin/flink ---
@@ -17,20 +17,41 @@
# limitations under the License.
################################################################################
-target="$0"
# For the case, the executable has been directly symlinked, figure out
# the correct bin path by following its symlink up to an upper bound.
# Note: we can't use the readlink utility here if we want to be POSIX
# compatible.
-iteration=0
-while [ -L "$target" ]; do
- if [ "$iteration" -gt 100 ]; then
- echo "Cannot resolve path: You have a cyclic symlink in $target."
+followSymLink() {
+ local iteration=0
+ local bar=$1
+ while [ -L "$bar" ]; do
--- End diff --
+1 for local variables but please don't call them `bar` \U0001f604 How about `local target=$1`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---