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-15488] Obtain the JVM and TM param correctly #10804
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 1bdc001 (Thu Jan 09 05:40:15 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:
|
CI report:
Bot commandsThe @flinkbot bot supports the following commands:
|
bc2b1f3
to
3a660ec
Compare
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 for opening the PR, @KarmaGYZ.
I left some comments.
flink-runtime/src/main/java/org/apache/flink/runtime/util/BashJavaUtils.java
Outdated
Show resolved
Hide resolved
3a660ec
to
84651f7
Compare
Thanks for the review @xintongsong . PR updated. |
84651f7
to
7ba78be
Compare
6c0d003
to
a3a5ebe
Compare
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.
LGTM. I have left some minor suggestions.
sb.append(s); | ||
} | ||
return sb.toString(); | ||
return IOUtils.toString(process.getInputStream()); |
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.
Should have been a separate commit but you don't need to fix it.
a3a5ebe
to
fbcc36f
Compare
LGTM. Merging |
This avoids that log messages to stdout intermingle with BashJavaUtils' computation result which is also printed to stdout. This closes apache#10804.
This avoids that log messages to stdout intermingle with BashJavaUtils' computation result which is also printed to stdout. This closes apache#10804.
This avoids that log messages to stdout intermingle with BashJavaUtils' computation result which is also printed to stdout. This closes apache#10804.
This avoids that log messages to stdout intermingle with BashJavaUtils' computation result which is also printed to stdout. This closes apache#10804.
This avoids that log messages to stdout intermingle with BashJavaUtils' computation result which is also printed to stdout. This closes apache#10804.
This avoids that log messages to stdout intermingle with BashJavaUtils' computation result which is also printed to stdout. This closes #10804.
What is the purpose of the change
When using logback it is not possible to start the taskmanager using taskamanger.sh scripts. To fix this issue, we need to make a contract that the calculation result is always outputted in the last line and in specific format. Then, we can fetch and verify it even when the log is redirected to console.
Brief change log
BashJavaUtils
classVerifying this change
Using logback, you could follow this guide
Start a TaskExecutor by
${FLINK_DIR}/bin/taskmanager.sh
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation