[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mjsax commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-110096269 No worries :) I am used to it. And I think it's good to ensure high code quality! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-110076740 Thank you! Sorry for giving you a hard time. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/796 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mjsax commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-110004370 See your point. But we need to come to a conclusion. - I like my single line comment better (of course ;)) -- I thinks it's easier to read than any regex -- and I don't think, that the cut-process in an issue - the last command you suggested is taking the first matching token, thus, there are still multiple cut-off characters Should we start a vote? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/796#issuecomment-109997007
No. The variable $SLAVE in unchanged... The comment line is not removed (I
just drop the else here and print $SLAVE after the if-then):
mjsax@T420s-dbis-mjsax:~$ SLAVE=#comment; if [[ $SLAVE =~
^([0-9a-zA-Z/.-]+).*$ ]] ; then echo ${BASH_REMATCH[1]}; fi; echo $SLAVE
#comment
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-110005201 No need for a vote. I agree with you that it is better to have only one comment character. The cut command is also ok. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-110002485 It can be a backwards-compatibility problem if users used some custom comment character. Probably negligible. Concerning the second if statement: Apparently, there was some feature along the lines of domain/hostname. This is not documented at all and can be removed. We can create a separate JIRA for that. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mjsax commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-110005377 Ok. So I remove the first if-then and update this PR. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mjsax commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-110001000 And I don't think it a compatibility problem. I think, '#' is no valid characters for host names. Thus, nobody can have it in any slave file anyway. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/796#issuecomment-109998768
My bad. Sorry for bothering. I had slightly modified the example. We could,
however, just remove the if statement and it would work without creating an
external process:
```bash
[[ hello bli blub 123 353 =~ ^([0-9a-zA-Z/.-]+).*$ ]]
SLAVE=${BASH_REMATCH[1]}
```
I agree that it is cleaner to have only one comment character. Only
disadvantage I see is that it breaks backwards-compatibility. The whole thing
needs a bit of cleanup. For example, the network topology seems a bit weird
and is probably a legacy feature.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-109988749 Are you sure? For me, it works in bash. Just tested under MacOS, Debian Stable, and Ubuntu. It doesn't work in other shells which implement regexp matching differently. For example, in zsh the matching array is called `match` instead of `BASH_REMATCH`. However, the script is meant to be executed in bash anyways and therefore has a `#!/usr/bin/env bash` shebang. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mjsax commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-109982446 This does not work on my system... and obviously not on other system either. Otherwise, the feature would not have been requested. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/796#issuecomment-109994289
mjsax@T420s-dbis-mjsax:~$ SLAVE=#comment; if [[ $SLAVE =~
^([0-9a-zA-Z/.-]+).$ ]] ; then echo ${BASH_REMATCH[1]}; else echo
missmatch; fi
missmatch
Exactly. So, it works as expected. It returns an empty string in case the
host is commented out. With your changes, it will then not be considered as a
host name.
mjsax@T420s-dbis-mjsax:~$ SLAVE=host#comment; if [[ $SLAVE =~
^([0-9a-zA-Z/.-]+).$ ]] ; then echo ${BASH_REMATCH[1]}; else echo
missmatch; fi
host
That's also fine because it should remove the comment.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mjsax commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-10868 I did not understand the purpose of the second if-then at all... But the first does not remove comment lines. My introduces code works perfectly fine. IMHO, it's cleaner code and I would remove the first if-then completely and replace it with my single-line-cut command. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mjsax commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-109992966 The regex matches everything up to the first character that is not '0-9a-zA-Z/.-' Thus, any non matching character is uses as cut-off (e.g. blank, tab, @, |, ...). I think we should remove the if-then completely. The 'cut' command does exactly what we need and used '#' as comment only. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mbalassi commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-109564993 LGTM, quite convenient. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mjsax commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-109572648 Just realized, that it is better to put it into extractHostName function within bin/config.sh. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
GitHub user mjsax opened a pull request: https://github.com/apache/flink/pull/796 [FLINK-2174] Allow comments in 'slaves' file added skipping of #-comments in slaves file in start/stop scripts You can merge this pull request into a Git repository by running: $ git pull https://github.com/mjsax/flink slavesFile Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/796.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 #796 commit f3d3bd7b5ab4241371642f5d0da3f06f7f56d92c Author: mjsax mj...@informatik.hu-berlin.de Date: 2015-06-05T18:35:45Z added skipping of #-comments in slaves file --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2174] Allow comments in 'slaves' file
Github user mjsax commented on the pull request: https://github.com/apache/flink/pull/796#issuecomment-109480303 My Travis is green (https://travis-ci.org/mjsax/flink/builds/65612778). Any comments? Can be merged from my point of view. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
