[FLINK-15145][config] Change TM memory configuration default values for FLIP-49. #10860

xintongsong · 2020-01-15T09:37:22Z

What is the purpose of the change

This PR updates FLIP-49 TM memory sizes configuration default values according to the outcome of tuning with real jobs.

Brief change log

JVM overhead min: 128MB -> 192MB
JVM metaspace: 128MB -> 96MB
Total process size (in default flink-conf.yaml): 1024MB -> 1568MB

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

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

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
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 · 2020-01-15T09:40:04Z

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 5f9cd22 (Wed Jan 15 09:40:04 UTC 2020)

Warnings:

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 · 2020-01-15T10:14:25Z

CI report:

5f9cd22 Travis: SUCCESS Azure: SUCCESS
f57ca41 Travis: SUCCESS Azure: FAILURE
3ac114c Travis: SUCCESS Azure: FAILURE

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

azagrebin

Thanks for the PR @xintongsong
It looks good to me, just added suggestions for a bit more details in comments
and reorganising process and Flink memory sizes.

EDIT: we probably want to mention the legacy option taskmanager.heap.size only in migration guide.

azagrebin · 2020-01-15T10:01:35Z

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

+# The total process memory size for the TaskManager.
+#
+# Note this accounts for all memory usages of a TaskManager process, including JVM metaspace and other overheads.
+# To exclude JVM metaspace and other overheads, please use total flink memory size (taskmanager.memory.flink.size) instead.


Suggested change

# To exclude JVM metaspace and other overheads, please use total flink memory size (taskmanager.memory.flink.size) instead.

azagrebin · 2020-01-15T10:02:27Z

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

@@ -42,9 +42,12 @@ jobmanager.rpc.port: 6123
 jobmanager.heap.size: 1024m


-# The heap size for the TaskManager JVM
+# The total process memory size for the TaskManager.


Suggested change

# The total process memory size for the TaskManager.

# The total process memory size for the TaskExecutor.

I think this the most recent term for task manager after FLIP-6

In the code base it is correct that we call it TaskExecutor. However, we never fully transitioned from TaskManager to TaskExecutor in our documentation. Given that the memory config options are prefixed with taskmanager, I would suggest to stick with TaskManager for the time being.

azagrebin · 2020-01-15T10:02:58Z

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

-# The heap size for the TaskManager JVM
+# The total process memory size for the TaskManager.
+#
+# Note this accounts for all memory usages of a TaskManager process, including JVM metaspace and other overheads.


Suggested change

# Note this accounts for all memory usages of a TaskManager process, including JVM metaspace and other overheads.

# Note this accounts for all memory usage within the TaskExecutor process, including JVM metaspace and other overhead.

taskmanager.memory.process.size: 1568m

Same here with TaskExecutor -> TaskManager.

…in default flink-conf.yaml. - Correct the description of process memory size. - Provide the alternative flink memory size.

…fault flink-conf.yaml to 1568MB.

…ault value to 192MB.

…t value to 96MB.

xintongsong · 2020-01-15T11:12:38Z

Thanks for the review, @azagrebin.
Comments addressed.

tillrohrmann

Thanks a lot for providing this PR @xintongsong. The changes look good to me. The only thing I would suggest to change is the term TaskExecutor into TaskManager because that is also the prefix of the newly introduced memory configuration options.

tillrohrmann · 2020-01-15T16:59:27Z

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

@@ -42,10 +42,16 @@ jobmanager.rpc.port: 6123
 jobmanager.heap.size: 1024m


-# The heap size for the TaskManager JVM
+# The total process memory size for the TaskExecutor.


I would suggest to use the term TaskExecutor for time being because we have never really changed the name in the documetation.

tillrohrmann · 2020-01-15T16:59:58Z

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

-# The heap size for the TaskManager JVM
+# The total process memory size for the TaskExecutor.
+#
+# Note this accounts for all memory usage within the TaskExecutor process, including JVM metaspace and other overhead.


Same here with TaskExecutor and TaskManager.

…y size in default flink-conf.yaml.

xintongsong · 2020-01-16T01:48:05Z

Thank you @tillrohrmann.
I've addressed your comments.

tillrohrmann · 2020-01-16T08:00:36Z

Thanks @xintongsong for addressing my comments so quickly. Merging this PR now.

…t value to 96MB. This closes #10860.

xintongsong requested a review from azagrebin January 15, 2020 09:37

rmetzger added the review=description? label Jan 15, 2020

rmetzger added the component=Runtime/Configuration label Jan 15, 2020

azagrebin reviewed Jan 15, 2020

View reviewed changes

azagrebin requested review from azagrebin and tillrohrmann January 15, 2020 10:32

xintongsong added 4 commits January 15, 2020 19:10

[hotfix][dist][config] Update the comments of TM process memory size …

14bf4ae

…in default flink-conf.yaml. - Correct the description of process memory size. - Provide the alternative flink memory size.

[FLINK-15145][dist][config] Increase the TM process memory size in de…

071f799

…fault flink-conf.yaml to 1568MB.

[FLINK-15145][core][config] Increase the TM JVM overhead min size def…

c03845d

…ault value to 192MB.

[FLINK-15145][core][config] Decrease the TM JVM metaspace size defaul…

f57ca41

…t value to 96MB.

xintongsong force-pushed the FLINK-15145-default-changes branch from 5f9cd22 to f57ca41 Compare January 15, 2020 11:12

tillrohrmann approved these changes Jan 15, 2020

View reviewed changes

fixup! [hotfix][dist][config] Update the comments of TM process memor…

3ac114c

…y size in default flink-conf.yaml.

tillrohrmann closed this in d5272ec Jan 16, 2020

tillrohrmann pushed a commit that referenced this pull request Jan 16, 2020

[FLINK-15145][core][config] Decrease the TM JVM metaspace size defaul…

6dfa0a7

…t value to 96MB. This closes #10860.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-15145][config] Change TM memory configuration default values for FLIP-49. #10860

[FLINK-15145][config] Change TM memory configuration default values for FLIP-49. #10860

xintongsong commented Jan 15, 2020

flinkbot commented Jan 15, 2020

flinkbot commented Jan 15, 2020 •

edited

Loading

azagrebin left a comment •

edited

Loading

azagrebin Jan 15, 2020

azagrebin Jan 15, 2020

tillrohrmann Jan 15, 2020

azagrebin Jan 15, 2020 •

edited

Loading

tillrohrmann Jan 15, 2020

xintongsong commented Jan 15, 2020

tillrohrmann left a comment

tillrohrmann Jan 15, 2020

tillrohrmann Jan 15, 2020

xintongsong commented Jan 16, 2020

tillrohrmann commented Jan 16, 2020

	# The total process memory size for the TaskManager.
	# The total process memory size for the TaskExecutor.

[FLINK-15145][config] Change TM memory configuration default values for FLIP-49. #10860

[FLINK-15145][config] Change TM memory configuration default values for FLIP-49. #10860

Conversation

xintongsong commented Jan 15, 2020

What is the purpose of the change

Brief change log

Verifying this change

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

Documentation

flinkbot commented Jan 15, 2020

Automated Checks

Review Progress

flinkbot commented Jan 15, 2020 • edited Loading

CI report:

azagrebin left a comment • edited Loading

Choose a reason for hiding this comment

azagrebin Jan 15, 2020

Choose a reason for hiding this comment

azagrebin Jan 15, 2020

Choose a reason for hiding this comment

tillrohrmann Jan 15, 2020

Choose a reason for hiding this comment

azagrebin Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

tillrohrmann Jan 15, 2020

Choose a reason for hiding this comment

xintongsong commented Jan 15, 2020

tillrohrmann left a comment

Choose a reason for hiding this comment

tillrohrmann Jan 15, 2020

Choose a reason for hiding this comment

tillrohrmann Jan 15, 2020

Choose a reason for hiding this comment

xintongsong commented Jan 16, 2020

tillrohrmann commented Jan 16, 2020

flinkbot commented Jan 15, 2020 •

edited

Loading

azagrebin left a comment •

edited

Loading

azagrebin Jan 15, 2020 •

edited

Loading