[FLINK-14405][runtime] Align ResourceProfile/ResourceSpec fields with the new TaskExecutor memory setups. #9910

xintongsong · 2019-10-16T10:30:11Z

What is the purpose of the change

This PR updates fields of ResourceProfile and ResourceSpec to align with FLIP-49 TaskExecutor resources.

Brief change log

1917ca8..11b699b: Hot-fixes and clean-ups that should not introduce behavior changes.
- 1917ca8: Introduce MemorySize constants of special values that are reused a lot.
- 69c49f3: Move SimpleSlotContext to test scope. There's no usage in production codes. This change will allow us to annotate ResourceProfile#ANY as for test purpose only.
- 2407804: Fix misusage of ResourceProfile#UNKNOWN and ResourceProfile#ANY.
- 265ebd9: Deduplicate operator resource / parallelism argument checks.
- 41be21c: Disable numerical comparing ResourceProfile#UNKNOWN.
- 11b699b: Make MemorySize Comparable.
02a54db: Update ResourceProfile and ResourceSpec fields to align with FLIP-49 TaskExecutor resources.
d2493bf: Restrict ResourceSpec to always have cpu cores and task heap memory size specified, unless is UNKNOWN.

Verifying this change

This change is already covered by existing tests, such as ResourceProfileTest and ResourceSpecTest. This PR updates the existing tests to align with the new fields.

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 · 2019-10-16T10:33:41Z

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 e3f278e (Wed Dec 04 15:06:07 UTC 2019)

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 · 2019-10-16T10:54:51Z

CI report:

8d1696b : FAILURE Build
bd54289 : SUCCESS Build
7df654a : CANCELED Build
90c4d2d : FAILURE Build
d2493bf : SUCCESS Build
0d09297 : FAILURE Build
8d1671e : SUCCESS Build
e34903f : SUCCESS Build
e3f278e : SUCCESS Build

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build

yanghua · 2019-10-16T11:21:54Z

Hi @xintongsong It seems the correct JIRA id of this PR should be FLINK-14405?

xintongsong · 2019-10-16T12:01:18Z

Thanks for the correction, @yanghua. Just fixed.

azagrebin

Thanks for the PR @xintongsong
I left small comments and one to discuss about the case where the operator does not care about the type of the requested managed memory.

flink-java/src/main/java/org/apache/flink/api/java/operators/DataSink.java

...k-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java

flink-core/src/main/java/org/apache/flink/api/common/operators/ResourceSpec.java

...k-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java

...blink/src/main/java/org/apache/flink/table/planner/plan/nodes/resource/NodeResourceUtil.java

xintongsong · 2019-10-17T11:58:48Z

@azagrebin,
I've updated the PR addresses some of your comments (of which conversations are resolved). For the rest of them, I need your feedback to proceed. Thanks

xintongsong · 2019-10-18T09:30:48Z

@azagrebin,
I've updated the PR. Could you please take another look.
Travis passed: https://travis-ci.org/xintongsong/flink/builds/599486641

azagrebin

Thanks for addressing the comments @xintongsong
I have left some smaller comments to resolve before merge.
For future, it would be also nice to have changes for spec and profile in separate commits in this kind of PR.

.../src/test/java/org/apache/flink/runtime/resourcemanager/slotmanager/SlotManagerImplTest.java

...c/main/java/org/apache/flink/api/common/operators/util/OperatorResourceParallelismUtils.java

...k-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java

azagrebin · 2019-10-22T09:14:57Z

...k-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java

@@ -416,56 +480,56 @@ public ResourceProfile subtract(ResourceProfile other) {

 		return new ResourceProfile(
 			subtractDoublesConsideringInf(cpuCores, other.cpuCores),


Can we actually subtract a resource with some bigger field?
Like, we can get negative CPU which we do not check in the constructor anymore

There's a checkArgument to make sure that this is larger than other, if this is what you are concerning.

azagrebin · 2019-10-22T09:30:22Z

flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskManagerServices.java

+			MemorySize.MAX_VALUE,
+			MemorySize.MAX_VALUE,
+			// TODO: before operators separate on-heap/off-heap managed memory, we use on-heap managed memory to denote total managed memory
+			MemorySize.parse(managedMemoryPerSlotMB + "m"),


This should basically be adjusted with the #9801, right?

flink-core/src/main/java/org/apache/flink/api/common/operators/ResourceSpec.java

...k-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java

flink-core/src/main/java/org/apache/flink/api/common/operators/ResourceSpec.java

KarmaGYZ

It generally LGTM.
One comment, is the usage of UNKNOWN correct in Dispatcher#isPartialResourceConfigured?

...k-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java

xintongsong · 2019-11-07T03:45:06Z

It generally LGTM.
One comment, is the usage of UNKNOWN correct in Dispatcher#isPartialResourceConfigured?

Yes, I think so.

...k-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java

xintongsong · 2019-11-07T05:25:22Z

Thanks for the review, @azagrebin, @zhuzhurk, @KarmaGYZ.
The comments are addressed.

xintongsong · 2019-11-07T07:42:35Z

Travis Passed: https://travis-ci.org/xintongsong/flink/builds/608570964

This class is for the legacy code, and there's no usage of this class in production codes.

- UNKNOWN should be used for describing: - A resource requirement (SlotRequest, SlotProfile, ExecutionJobVertex, etc.) that the exact amount of resource needed is not specified. - Remaining resource of a multi task slot (SlotInfoAndResources) that contains tasks with unknown resource profiles. - UNKNOWN should not be used for describing total resource of a slot / task executor (TaskSlot, SlotOffer, SlotReport, etc.), which should always be specific with FLIP-49. - ANY should be used for describing a set of resource (SlotOffer, SlotReport, TaskSlot, etc.) that is large enough to match any request. It's for testability purpose only. With FLIP-49, task executors / slots should always have specific resource profiles in production. This commit fix misusages of the two constants.

… checks.

…urce types

…esource types

…d task heap memory size, unless it UNKNOWN.

…urceSpec.

…notate constructors used only in testing codes as VisibleForTestting for ResourceProfile.

azagrebin

Thanks @xintongsong
LGTM, merging it when the Travis is green

azagrebin · 2019-11-12T12:52:36Z

merged into master by b872f19 .. 36d1b62

rmetzger added review=description? component=Runtime/Metrics component=Runtime/Task labels Oct 16, 2019

xintongsong changed the title ~~[FLINK-14406] Align ResourceProfile/ResourceSpec fields with the new TaskExecutor memory setups.~~ [FLINK-14406][runtime][core] Align ResourceProfile/ResourceSpec fields with the new TaskExecutor memory setups. Oct 16, 2019

xintongsong changed the title ~~[FLINK-14406][runtime][core] Align ResourceProfile/ResourceSpec fields with the new TaskExecutor memory setups.~~ [FLINK-14406][runtime] Align ResourceProfile/ResourceSpec fields with the new TaskExecutor memory setups. Oct 16, 2019

xintongsong marked this pull request as ready for review October 16, 2019 10:41

xintongsong changed the title ~~[FLINK-14406][runtime] Align ResourceProfile/ResourceSpec fields with the new TaskExecutor memory setups.~~ [FLINK-14405][runtime] Align ResourceProfile/ResourceSpec fields with the new TaskExecutor memory setups. Oct 16, 2019

rmetzger removed the component=Runtime/Metrics label Oct 16, 2019

azagrebin requested changes Oct 16, 2019

View reviewed changes

xintongsong force-pushed the FLINK-14405-flip49-resource-profile-spec branch from 8d1696b to bd54289 Compare October 17, 2019 11:55

xintongsong force-pushed the FLINK-14405-flip49-resource-profile-spec branch 3 times, most recently from 90c4d2d to d2493bf Compare October 18, 2019 06:24

xintongsong requested a review from azagrebin October 18, 2019 09:29

azagrebin requested changes Oct 22, 2019

View reviewed changes

zhuzhurk reviewed Oct 24, 2019

View reviewed changes

flink-core/src/main/java/org/apache/flink/api/common/operators/ResourceSpec.java Outdated Show resolved Hide resolved

azagrebin mentioned this pull request Oct 29, 2019

[FLINK-14400] Shrink scope of MemoryManager from TaskExecutor to slot #10034

Merged

zhuzhurk reviewed Oct 30, 2019

View reviewed changes

flink-core/src/main/java/org/apache/flink/api/common/operators/ResourceSpec.java Outdated Show resolved Hide resolved

KarmaGYZ reviewed Nov 6, 2019

View reviewed changes

KarmaGYZ mentioned this pull request Nov 6, 2019

[FLINK-14594] Fix matching logics of ResourceSpec/ResourceProfile/Resource considering double values #10079

Merged

zhuzhurk reviewed Nov 7, 2019

View reviewed changes

...k-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java Outdated Show resolved Hide resolved

xintongsong force-pushed the FLINK-14405-flip49-resource-profile-spec branch from d2493bf to 0d09297 Compare November 7, 2019 03:52

zhuzhurk reviewed Nov 7, 2019

View reviewed changes

...k-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/ResourceProfile.java Outdated Show resolved Hide resolved

xintongsong force-pushed the FLINK-14405-flip49-resource-profile-spec branch 2 times, most recently from 8d1671e to e34903f Compare November 7, 2019 05:22

xintongsong mentioned this pull request Nov 11, 2019

[FLINK-14188][runtime] TaskExecutor derive and register with default slot resource profile #10146

Closed

xintongsong added 12 commits November 11, 2019 15:31

[hotfix] Introduce constants MemorySize#ZERO and MemorySize#MAX_VALUE.

6730c36

[hotfix] Move SimpleSlotContext to test scope.

97d4a4f

This class is for the legacy code, and there's no usage of this class in production codes.

[hotfix] Deduplicate setting operator resource / parallelism argument…

8fa4a0a

… checks.

[hotfix] UNKNOWN ResourceSpec should not be numerically compared.

449d4cf

[hotfix] Make MemorySize comparable.

422d91e

[FLINK-14405][runtime] Update ResourceSpec to align with FLIP-49 reso…

08cd4db

…urce types

[FLINK-14405][runtime] Update ResourceProfile to align with FLIP-49 r…

180ce67

…esource types

[FLINK-14495][core] Limit ResourceSpec to always specify cpu cores an…

26636fd

…d task heap memory size, unless it UNKNOWN.

[hotfix] Remove unused constructors and unnecessary javadocs for Reso…

08166a8

…urceSpec.

[hotfix] Remove unused constructors and unnecessary javadoces, and an…

6afb300

…notate constructors used only in testing codes as VisibleForTestting for ResourceProfile.

[hotfix] Preserve the singleton property for ResourceProfile#ANY.

e3f278e

xintongsong force-pushed the FLINK-14405-flip49-resource-profile-spec branch from e34903f to e3f278e Compare November 11, 2019 15:27

azagrebin approved these changes Nov 11, 2019

View reviewed changes

azagrebin closed this Nov 12, 2019

azagrebin mentioned this pull request Dec 3, 2019

[FLINK-15023][core][runtime] Remove on-heap managed memory #10397

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-14405][runtime] Align ResourceProfile/ResourceSpec fields with the new TaskExecutor memory setups. #9910

[FLINK-14405][runtime] Align ResourceProfile/ResourceSpec fields with the new TaskExecutor memory setups. #9910

xintongsong commented Oct 16, 2019 •

edited

flinkbot commented Oct 16, 2019 •

edited

flinkbot commented Oct 16, 2019 •

edited

yanghua commented Oct 16, 2019

xintongsong commented Oct 16, 2019

azagrebin left a comment

xintongsong commented Oct 17, 2019

xintongsong commented Oct 18, 2019

azagrebin left a comment

azagrebin Oct 22, 2019

xintongsong Nov 7, 2019

azagrebin Oct 22, 2019

xintongsong Nov 7, 2019

KarmaGYZ left a comment

xintongsong commented Nov 7, 2019

xintongsong commented Nov 7, 2019

xintongsong commented Nov 7, 2019

azagrebin left a comment

azagrebin commented Nov 12, 2019

		@@ -416,56 +480,56 @@ public ResourceProfile subtract(ResourceProfile other) {

		return new ResourceProfile(
		subtractDoublesConsideringInf(cpuCores, other.cpuCores),

[FLINK-14405][runtime] Align ResourceProfile/ResourceSpec fields with the new TaskExecutor memory setups. #9910

[FLINK-14405][runtime] Align ResourceProfile/ResourceSpec fields with the new TaskExecutor memory setups. #9910

Conversation

xintongsong commented Oct 16, 2019 • edited

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 Oct 16, 2019 • edited

Automated Checks

Review Progress

flinkbot commented Oct 16, 2019 • edited

CI report:

yanghua commented Oct 16, 2019

xintongsong commented Oct 16, 2019

azagrebin left a comment

Choose a reason for hiding this comment

xintongsong commented Oct 17, 2019

xintongsong commented Oct 18, 2019

azagrebin left a comment

Choose a reason for hiding this comment

azagrebin Oct 22, 2019

Choose a reason for hiding this comment

xintongsong Nov 7, 2019

Choose a reason for hiding this comment

azagrebin Oct 22, 2019

Choose a reason for hiding this comment

xintongsong Nov 7, 2019

Choose a reason for hiding this comment

KarmaGYZ left a comment

Choose a reason for hiding this comment

xintongsong commented Nov 7, 2019

xintongsong commented Nov 7, 2019

xintongsong commented Nov 7, 2019

azagrebin left a comment

Choose a reason for hiding this comment

azagrebin commented Nov 12, 2019

xintongsong commented Oct 16, 2019 •

edited

flinkbot commented Oct 16, 2019 •

edited

flinkbot commented Oct 16, 2019 •

edited