Skip to content
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-16742][runtime][dist] Extend and use BashJavaUtils to start JM JVM process and pass JVM memory args #11545

Closed
wants to merge 6 commits into from

Conversation

xintongsong
Copy link
Contributor

@xintongsong xintongsong commented Mar 27, 2020

What is the purpose of the change

This PR is part of FLIP-116. It makes the start-up shell scripts to launch JM JVM process with the new memory configuration logics and utils.

Brief change log

  • 2dca2f2: Minor code clean-ups.
  • ee0a086: Make config.sh not assuming BashJavaUtis always return two lines of results. The assumption was true for calculating TM resource, but is not for calculating JM resource.
  • 56b965a: Extend BashJavaUtils for JM memory calculations, and use it in start-up scripts for standalone session cluster.
  • 2b3ea1a: Move BashJavaUtils to a separated module. This is for resolving resolving the dependency between BashJavaUtils and flink-container.
  • 18f44da: Extend BashJavaUtils for JM memory calculations, and use it in start-up scripts for standalone job cluster.
  • d20feb4: Remove common memory configuration / calculation codes, which are no longer used, from config.sh.

Verifying this change

  • Extended BashJavaUtilsITCase

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (yes)
  • 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): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: (yes)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

@flinkbot
Copy link
Collaborator

flinkbot commented Mar 27, 2020

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 788ae1e (Wed Apr 15 11:40:19 UTC 2020)

Warnings:

  • 1 pom.xml files were touched: Check for build and licensing issues.
  • No documentation files were touched! Remember to keep the Flink docs up to date!

Mention the bot in a comment to re-run the automated checks.

Review Progress

  • ❓ 1. The [description] looks good.
  • ❓ 2. There is [consensus] that the contribution should go into to Flink.
  • ❓ 3. Needs [attention] from.
  • ❓ 4. The change fits into the overall [architecture].
  • ❓ 5. Overall code [quality] is good.

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 commands
The @flinkbot bot supports the following commands:

  • @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
  • @flinkbot approve all to approve all aspects
  • @flinkbot approve-until architecture to approve everything until architecture
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval you gave earlier

@flinkbot
Copy link
Collaborator

flinkbot commented Mar 27, 2020

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run travis re-run the last Travis build
  • @flinkbot run azure re-run the last Azure build

@xintongsong
Copy link
Contributor Author

cc @azagrebin

Copy link
Contributor

@azagrebin azagrebin left a 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 @xintongsong
It looks quite good to me. I left couple of comments to discuss.
In particular, I am not sure about the need to support various ways to parse options for different Flink processes.

@xintongsong
Copy link
Contributor Author

Thanks for the review @azagrebin.
I've addressed your comments.

Copy link
Contributor

@azagrebin azagrebin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @xintongsong for addressing the comments.
The PR looks good to me. I left only minor comments and a question about the contents of the bash util jar.

@xintongsong
Copy link
Contributor Author

@azagrebin Thanks for the review. Comments addressed.

@xintongsong xintongsong force-pushed the FLINK-16742-jm-mem-sh branch 2 times, most recently from 0c1d7ba to 7e5cae7 Compare April 9, 2020 05:44
@xintongsong
Copy link
Contributor Author

@azagrebin
I've addressed your comments. Please take a look at your convenience.

Copy link
Contributor

@azagrebin azagrebin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @xintongsong, I think the last change simplified a lot and the PR looks good overall.
I left only one concern about the -D option specifics.

@xintongsong xintongsong force-pushed the FLINK-16742-jm-mem-sh branch 2 times, most recently from 46b64a4 to ea1ef51 Compare April 15, 2020 03:48
@xintongsong
Copy link
Contributor Author

Thanks @azagrebin, comments addressed.

Copy link
Contributor

@azagrebin azagrebin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @xintongsong , LGTM
I left only a small refactoring suggestion

@xintongsong
Copy link
Contributor Author

Thanks @azagrebin, I've addressed the last comment.

xintongsong added a commit to xintongsong/flink that referenced this pull request Apr 17, 2020
@xintongsong
Copy link
Contributor Author

Rebased onto #11577.
A few changes during the rebasing:

  • Pass dynamic properties to BashJavaUtils for JM of standalone job clusters. (JM of standalone session cluster does not support dynamic properties ATM.)
  • Add test case for getting JM resource parameters from BashJavaUtils with dynamic properties.

azagrebin pushed a commit to azagrebin/flink that referenced this pull request Apr 17, 2020
azagrebin pushed a commit that referenced this pull request Apr 20, 2020
Comment on lines 642 to 505
IFS=$'\n' execution_results=($(echo "${output}" | grep ${EXECUTION_PREFIX}))
if [[ ${#execution_results[@]} != ${expected_lines} ]]; then
echo "[ERROR] The execution results has unexpected number of lines, expected: ${expected_lines}, actual: ${#execution_results[@]}." 1>&2
echo "[ERROR] An execution result line is expected following the prefix '${EXECUTION_PREFIX}'" 1>&2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like, we have to rebase this part on master because of FLINK-17023. Btw, wc -l solution there for counting lines looks a bit simpler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extractExecutionResults() {
    local output="$1"
    local expected_lines="$2"
    local EXECUTION_PREFIX="BASH_JAVA_UTILS_EXEC_RESULT:"
    local execution_results
    local num_lines

    execution_results=$(echo "${output}" | grep ${EXECUTION_PREFIX})
    num_lines=$(echo "${execution_results}" | wc -l)
    if [[ ${num_lines} -ne ${expected_lines} ]]; then
        echo "[ERROR] The execution result has unexpected number of lines, expected: ${expected_lines}, actual: ${num_lines}." 1>&2
        echo "[ERROR] An execution result line is expected to have the prefix '${EXECUTION_PREFIX}'" 1>&2
        echo "$output" 1>&2
        exit 1
    fi

    echo "${execution_results//${EXECUTION_PREFIX}/}"
}

azagrebin pushed a commit to azagrebin/flink that referenced this pull request Apr 21, 2020
@xintongsong
Copy link
Contributor Author

@azagrebin Sorry for the late response, and thanks for providing the solution for the conflict resolving. I've rebased the PR.

@azagrebin
Copy link
Contributor

@flinkbot run travis

@azagrebin
Copy link
Contributor

@flinkbot run azure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants