[FLINK-22683][runtime] Fix the null or incorrect value of total Flink… #15936

KarmaGYZ · 2021-05-17T10:06:30Z

…/process memory in creating TaskExecutorMemoryConfiguration

What is the purpose of the change

(For example: This pull request makes task deployment go through the blob server, rather than through RPC. That way we avoid re-transferring them on each deployment (during recovery).)

Brief change log

(for example:)

The TaskInfo is stored in the blob store on job creation time as a persistent artifact
Deployments RPC transmits only the blob storage reference
TaskManagers retrieve the TaskInfo from the blob cache

Verifying this change

(Please pick either of the following options)

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

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (100MB)
Extended integration test for recovery after master (JobManager) failure
Added test that validates that TaskInfo is transferred only once across recoveries
Manually verified the change by running a 4 node cluser with 2 JobManagers and 4 TaskManagers, a stateful streaming program, and killing one JobManager and two TaskManagers during the execution, verifying that recovery happens correctly.

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

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

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

flinkbot · 2021-05-17T10:11:49Z

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 7b395b2 (Sat Aug 28 12:14:49 UTC 2021)

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 · 2021-05-17T10:34:22Z

CI report:

74bf5ad Azure: SUCCESS
7b395b2 Azure: PENDING

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 · 2021-06-01T08:21:40Z

...k-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskExecutorResourceUtils.java

+    public static long calculateTotalFlinkMemoryFromFactors(Configuration config) {
+        return config.get(TaskManagerOptions.TASK_HEAP_MEMORY)
+                .add(config.get(TaskManagerOptions.TASK_OFF_HEAP_MEMORY))
+                .add(config.get(TaskManagerOptions.NETWORK_MEMORY_MAX))
+                .add(config.get(TaskManagerOptions.MANAGED_MEMORY_SIZE))
+                .add(config.get(TaskManagerOptions.FRAMEWORK_HEAP_MEMORY))
+                .add(config.get(TaskManagerOptions.FRAMEWORK_OFF_HEAP_MEMORY))
+                .getBytes();
+    }
+
+    public static long calculateTotalProcessMemoryFromFactors(Configuration config) {
+        return calculateTotalFlinkMemoryFromFactors(config)
+                + config.get(TaskManagerOptions.JVM_METASPACE)
+                        .add(config.get(TaskManagerOptions.JVM_OVERHEAD_MAX))
+                        .getBytes();
+    }


Better to add some assertions.

memory sizes explicitly configured

max == min for network & jvm-overhead

xintongsong · 2021-06-01T08:22:39Z

...k-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskExecutorResourceUtils.java

        return resourceSpecFromConfig(adjustForLocalExecution(config));
    }

+    public static long calculateTotalFlinkMemoryFromFactors(Configuration config) {


Suggested change

public static long calculateTotalFlinkMemoryFromFactors(Configuration config) {

public static long calculateTotalFlinkMemoryFromComponents(Configuration config) {

xintongsong · 2021-06-01T08:23:05Z

...k-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskExecutorResourceUtils.java

+                .getBytes();
+    }
+
+    public static long calculateTotalProcessMemoryFromFactors(Configuration config) {


Suggested change

public static long calculateTotalProcessMemoryFromFactors(Configuration config) {

public static long calculateTotalProcessMemoryFromComponents(Configuration config) {

xintongsong · 2021-06-01T08:27:18Z

flink-runtime/src/test/java/org/apache/flink/runtime/taskexecutor/TaskExecutorTest.java

+        configuration.set(TaskManagerOptions.TASK_HEAP_MEMORY, MemorySize.ofMebiBytes(10));
+        configuration.set(TaskManagerOptions.MANAGED_MEMORY_SIZE, MemorySize.ofMebiBytes(10));


I think the assumption is that memory sizes for all components should be explicitly configured before starting the TM process. Having only these two options set and the test doesn't fail is an indicator that we do not check the assumption strictly.

KarmaGYZ · 2021-06-02T06:46:29Z

Thanks for the comments @xintongsong . PR updated.

…/process memory in creating TaskExecutorMemoryConfiguration

xintongsong

LGTM. Let's wait for the AZP green light before merging this.

…/process memory in creating TaskExecutorMemoryConfiguration This closes #15936

…/process memory in creating TaskExecutorMemoryConfiguration This closes apache#15936

rmetzger added the review=description? label May 17, 2021

rmetzger added the component=Runtime/Metrics label May 17, 2021

KarmaGYZ force-pushed the FLINK-22683 branch 2 times, most recently from e46df61 to 259e9d4 Compare May 21, 2021 04:56

xintongsong reviewed Jun 1, 2021

View reviewed changes

KarmaGYZ force-pushed the FLINK-22683 branch 4 times, most recently from 0221799 to 74bf5ad Compare June 2, 2021 06:45

KarmaGYZ force-pushed the FLINK-22683 branch from 74bf5ad to 69ff412 Compare June 3, 2021 02:38

[FLINK-22683][runtime] Fix the null or incorrect value of total Flink…

7b395b2

…/process memory in creating TaskExecutorMemoryConfiguration

KarmaGYZ force-pushed the FLINK-22683 branch from 69ff412 to 7b395b2 Compare June 3, 2021 02:50

xintongsong approved these changes Jun 3, 2021

View reviewed changes

xintongsong closed this in 9e8c551 Jun 3, 2021

xintongsong pushed a commit that referenced this pull request Jun 3, 2021

[FLINK-22683][runtime] Fix the null or incorrect value of total Flink…

2ea2f9f

…/process memory in creating TaskExecutorMemoryConfiguration This closes #15936

xintongsong pushed a commit to xintongsong/flink that referenced this pull request Jun 3, 2021

[FLINK-22683][runtime] Fix the null or incorrect value of total Flink…

02105e5

…/process memory in creating TaskExecutorMemoryConfiguration This closes apache#15936

xintongsong mentioned this pull request Jun 3, 2021

[FLINK-22683][runtime] Fix the null or incorrect value of total flink/process memory in creating TaskExecutorMemoryConfiguration #16067

Closed

xintongsong pushed a commit to xintongsong/flink that referenced this pull request Jun 3, 2021

[FLINK-22683][runtime] Fix the null or incorrect value of total Flink…

2954bb2

…/process memory in creating TaskExecutorMemoryConfiguration This closes apache#15936

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[FLINK-22683][runtime] Fix the null or incorrect value of total Flink… #15936

[FLINK-22683][runtime] Fix the null or incorrect value of total Flink… #15936

Uh oh!

KarmaGYZ commented May 17, 2021

Uh oh!

flinkbot commented May 17, 2021 •

edited

Loading

Uh oh!

flinkbot commented May 17, 2021 •

edited

Loading

Uh oh!

xintongsong Jun 1, 2021

Uh oh!

xintongsong Jun 1, 2021

Uh oh!

xintongsong Jun 1, 2021

Uh oh!

xintongsong Jun 1, 2021

Uh oh!

KarmaGYZ commented Jun 2, 2021

Uh oh!

xintongsong left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	public static long calculateTotalFlinkMemoryFromFactors(Configuration config) {
	public static long calculateTotalFlinkMemoryFromComponents(Configuration config) {

	public static long calculateTotalProcessMemoryFromFactors(Configuration config) {
	public static long calculateTotalProcessMemoryFromComponents(Configuration config) {

		configuration.set(TaskManagerOptions.TASK_HEAP_MEMORY, MemorySize.ofMebiBytes(10));
		configuration.set(TaskManagerOptions.MANAGED_MEMORY_SIZE, MemorySize.ofMebiBytes(10));

Uh oh!

[FLINK-22683][runtime] Fix the null or incorrect value of total Flink… #15936

[FLINK-22683][runtime] Fix the null or incorrect value of total Flink… #15936

Uh oh!

Conversation

KarmaGYZ commented May 17, 2021

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

Uh oh!

flinkbot commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automated Checks

Review Progress

Uh oh!

flinkbot commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

xintongsong Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

xintongsong Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

xintongsong Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

xintongsong Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

KarmaGYZ commented Jun 2, 2021

Uh oh!

xintongsong left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

flinkbot commented May 17, 2021 •

edited

Loading

flinkbot commented May 17, 2021 •

edited

Loading