[FLINK-16746] Deprecate legacy heap memory option for JM and expose the new ones in docs #11787

azagrebin · 2020-04-17T07:51:54Z

What is the purpose of the change

Add default 'jobmanager.memory.process.size: 1472m' to flink-conf.yaml.

Deprecate legacy heap options: jobmanager.heap.size (and update deprecation of jobmanager.heap.mb) to use jobmanager.memory.heap.size for standalone and jobmanager.memory.process.size for containerized (Yarn/K8s) environments.

Expose the new JM memory options in user docs.

Verifying this change

Trivial change. Check docs.

azagrebin · 2020-04-17T07:53:04Z

The PR is based on #11577, #11545 and #11675 atm.
Only two last commits are to review.
cc @xintongsong

flinkbot · 2020-04-17T07:55:37Z

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 74f7619 (Fri Apr 17 07:55:37 UTC 2020)

Warnings:

1 pom.xml files were touched: Check for build and licensing issues.

_{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 · 2020-04-17T08:12:35Z

CI report:

e6af843 Travis: SUCCESS Azure: SUCCESS

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

Thanks for opening this PR, @azagrebin. The changes look good to me in general.
I left a few questions regarding the configuration value changes.
Besides, I have only two minor comments.

I would suggest to give the last commit a more descriptive message. It's not only deprecating the legacy configuration options, but also activating the new ones.
There are links in the mem_setup.md and mem_setup.zh.md pointing to config.html#jobmanager-heap-size, which I believe is removed.

xintongsong · 2020-04-20T07:09:47Z

flink-dist/src/main/resources/flink-conf.yaml


-jobmanager.heap.size: 1024m
+jobmanager.memory.process.size: 1472m


Could you explain why is the default 1472m?

I added a tab for JM in the calculation spreadsheet. The changes aim to align the new derived heap size with the previous default heap size for standalone. As a result, the container size will increase a bit. 1472m was miscalculated. It should be 1600m.

xintongsong · 2020-04-20T07:18:16Z

flink-jepsen/src/jepsen/flink/db.clj

@@ -41,7 +41,7 @@
 {:high-availability "zookeeper"
 :high-availability.zookeeper.quorum (zookeeper-quorum test)
 :high-availability.storageDir "hdfs:https:///flink/ha"
- :jobmanager.heap.size  "2048m"
+ :jobmanager.memory.process.size "2496m"


Same here? Why is the value change "2048 -> 2496"?

same here, it should be 2702m. Though, as discussed with @GJL, it does not matter too much in this case. I will revert this.

azagrebin · 2020-04-20T15:15:46Z

Thanks for the review @xintongsong
I addressed the comments.

The last thing:

There are links in the mem_setup.md and mem_setup.zh.md pointing to config.html#jobmanager-heap-size, which I believe is removed.

we can address in FLINK-16946. It is already planned to do so in the doc draft when the JM chapter becomes available. For now, this link will just go to the config page which should be ok.

xintongsong

Thanks for the explanations. LGTM.

…turn two lines of results.

…vaUtils.

…uration in start-up scripts. This closes apache#11545.

…nfig.sh.

…age and extract configuration loading logics.

… into ClusterSpecification

This closes apache#11675.

…getLegacyTaskManagerHeapMemoryIfExplicitlyConfigured

…uiredConfigOptions to run also for TM

…flink-conf.yaml The resulting default JVM heap size for standalone deployment is 1024Mb as before FLIP-116. The requested container size (Yarn and Kubernetes) will increase by default from 1024Mb to 1600Mb.

…ory options Deprecate legacy heap options: jobmanager.heap.size (and update deprecation of jobmanager.heap.mb) to use jobmanager.memory.heap.size for standalone and jobmanager.memory.process.size for containerized (Yarn/K8s) environments. Expose the new JM memory options of FLIP-116 in user docs. This closes apache#11787.

rmetzger added the review=description? label Apr 17, 2020

rmetzger added component=Runtime/Coordination component=Runtime/Configuration labels Apr 17, 2020

xintongsong reviewed Apr 20, 2020

View reviewed changes

azagrebin force-pushed the FLINK-16746 branch from 74f7619 to b4b0fac Compare April 20, 2020 15:11

xintongsong approved these changes Apr 21, 2020

View reviewed changes

azagrebin force-pushed the FLINK-16746 branch from b4b0fac to 3ba8e78 Compare April 21, 2020 08:04

xintongsong added 6 commits April 21, 2020 16:01

[hotfix][runtime] Code clean-ups in BashJavaUtils and its test cases.

00f8bf5

[FLINK-16742][dist] config.sh does not assume BashJavaUtils always re…

d2c6d9b

…turn two lines of results.

[FLINK-16742][runtime] Ignore unknown command line options for BashJa…

49d1214

…vaUtils.

[FLINK-16742][dist] Extend and use BashJavaUtils for JM memory config…

9a921b1

…uration in start-up scripts. This closes apache#11545.

[FLINK-16472][dist] Remove unused memory configuration logics from co…

a1b4d49

…nfig.sh.

[FLINK-16742][runtime] Refactor BashJavaUtils, move to separated pack…

a237df3

…age and extract configuration loading logics.

azagrebin force-pushed the FLINK-16746 branch from 3ba8e78 to eebcb0f Compare April 21, 2020 13:11

azagrebin added 7 commits April 22, 2020 17:02

[FLINK-16745][clients] Parse JobManagerProcessSpec from Configuration…

4ba10ff

… into ClusterSpecification

[FLINK-16745][yarn] Start Yarn JM with FLIP-116 JVM memory args

b077b7d

[FLINK-16745][k8s] Start Kubernetes JM with FLIP-116 JVM memory args

24e929f

This closes apache#11675.

[FLINK-16745][coordination] Remove unused container cutoff

145f624

[hotfix] Remove 'TaskManager' from MemoryBackwardsCompatibilityUtils#…

6e29344

…getLegacyTaskManagerHeapMemoryIfExplicitlyConfigured

[hotfix] Fix ProcessMemoryUtilsTestBase#testExceptionShouldContainReq…

c56b295

…uiredConfigOptions to run also for TM

[FLINK-16746] Add default 'jobmanager.memory.process.size: 1600m' to …

2ef0e88

…flink-conf.yaml The resulting default JVM heap size for standalone deployment is 1024Mb as before FLIP-116. The requested container size (Yarn and Kubernetes) will increase by default from 1024Mb to 1600Mb.

azagrebin force-pushed the FLINK-16746 branch from eebcb0f to b7bed0e Compare April 22, 2020 14:07

azagrebin force-pushed the FLINK-16746 branch from b7bed0e to e6af843 Compare April 22, 2020 14:15

azagrebin closed this in b020aaf Apr 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-16746] Deprecate legacy heap memory option for JM and expose the new ones in docs #11787

[FLINK-16746] Deprecate legacy heap memory option for JM and expose the new ones in docs #11787

azagrebin commented Apr 17, 2020

azagrebin commented Apr 17, 2020

flinkbot commented Apr 17, 2020

flinkbot commented Apr 17, 2020 •

edited

Loading

xintongsong left a comment

xintongsong Apr 20, 2020

azagrebin Apr 20, 2020

xintongsong Apr 20, 2020

azagrebin Apr 20, 2020 •

edited

Loading

azagrebin commented Apr 20, 2020 •

edited

Loading

xintongsong left a comment


		jobmanager.heap.size: 1024m
		jobmanager.memory.process.size: 1472m

[FLINK-16746] Deprecate legacy heap memory option for JM and expose the new ones in docs #11787

[FLINK-16746] Deprecate legacy heap memory option for JM and expose the new ones in docs #11787

Conversation

azagrebin commented Apr 17, 2020

What is the purpose of the change

Verifying this change

azagrebin commented Apr 17, 2020

flinkbot commented Apr 17, 2020

Automated Checks

Review Progress

flinkbot commented Apr 17, 2020 • edited Loading

CI report:

xintongsong left a comment

Choose a reason for hiding this comment

xintongsong Apr 20, 2020

Choose a reason for hiding this comment

azagrebin Apr 20, 2020

Choose a reason for hiding this comment

xintongsong Apr 20, 2020

Choose a reason for hiding this comment

azagrebin Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

azagrebin commented Apr 20, 2020 • edited Loading

xintongsong left a comment

Choose a reason for hiding this comment

flinkbot commented Apr 17, 2020 •

edited

Loading

azagrebin Apr 20, 2020 •

edited

Loading

azagrebin commented Apr 20, 2020 •

edited

Loading