-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
[FLINK-17264][scripts] Fix broken taskmanager.sh by using -eq to compare num_lines #11820
Conversation
cc @zentol Could you help to review this simple PR? |
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit a75e89f (Mon Apr 20 10:33:30 UTC 2020) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
a75e89f
to
56a0362
Compare
I did mean My suggestion was |
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.
Thanks @wangyang0918
if [[ ${num_lines} -ne 1 ]]
is better
56a0362
to
bb53e79
Compare
Thanks @aljoscha @leonardBang, i have updated the PR to use |
Sorry for introducing this bug in the code. I discover that there are also other scripts (mostly e2e test scripts, for example |
The root cause is that tabs alignment https://apple.stackexchange.com/questions/370366/why-is-wc-c-printing-spaces-before-the-number |
I am not sure how many users are running e2e tests on the Mac. And how many incompatible commands do we have in e2e tests. So maybe we need to create another ticket and have the discussion there. |
Thanks for creating this PR @wangyang0918. LGTM. +1 for merging this PR. |
Merging this PR now. |
@wangyang0918 I regularly run e2e tests on my mac when debugging issues, so it would be good to also fix those usages. Do you want to open an issue and cut a PR for that? Please ping me if you do. |
What is the purpose of the change
Start a taskmanager on Mac via taskmanager.sh will get the following error.
The root cause is FLINK-17023 introduce the following change and it could not work as expected.
On linux environment, the output is "1". However, on Mac, it is " 1". Maybe we need to add
tr -d '[:space:]'
afterwc
.Brief change log
tr -d '[:space:]'
Verifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation