[FLINK-20850][runtime] Removing usage of CoLocationConstraints #14584

XComp · 2021-01-08T07:56:21Z

What is the purpose of the change

The CoLocationConstraint class became obsolete when introducing FLINK-18690.

This PR depends on PR #14580 covering FLINK-20892.

Brief change log

The CoLocationConstraint was removed from the code clearing any existing member fields and updating/removing related test cases.

Verifying this change

CI Build

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): 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: 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 · 2021-01-08T07:58:03Z

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 51e4355 (Fri May 28 07:06:42 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-01-08T08:04:55Z

CI report:

51e4355 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

XComp · 2021-01-11T07:27:40Z

The refactoring is ready to be reviewed. Please also have a look at the docs changes that were necessary due to the refactoring.
@zhuzhurk It would be helpful for you to have a look as well.

zhuzhurk · 2021-01-11T09:04:56Z

@XComp how about we get the base PRs merged before continuing with this PR? I can see that all the base PRs are approved already.

Removed DefaultExecutionTopology.ensureCoLocatedVerticesInSameRegion(..) as CoLocationConstraint is not used anymore. FLINK-20589

Remove CoLocationConstraint from ExeuctionVertexSchedulingRequirements as it is not used anymore. FLINK-20589

This test verifies that the CoLocationConstraint is reset when recovering the task. We still need to verify whether it should be replaced. FLINK-20589

This test verifies that the CoLocationConstraints are created which is not the case anymore. FLINK-20589

…rtex The CoLocation information isn't processed through the ExecutionVertex anymore but is calculated statically by the LocalInputPreferredSlotSharingStrategy. FLINK-20589

The CoLocationConstraints are not used anymore but replaced by LocalInputPreferredSlotSharingStrategy. FLINK-20589

The CoLocationConstraints is not used anymore and, therefore, can be removed from the CoLocationGroup. FLINK-20589

CoLocationConstraint is deleted. Its JavaDoc is copied over to CoLocationConstraintDesc and adjusted accordingly. FLINK-20589

The CoLocationGroup only needs to be adapted within JobVertex. Any other locations can rely on CoLocationGroupDesc's read-only methods. FLINK-20589

Minor cleanup to follow Intellij suggestions. FLINK-20589

Previously, the CoLocationConstraints have been reset in this method. The CoLocationGroup handling is not needed anymore after the reset functionality was removed. FLINK-20589

…face The interface CoLocationGroupDesc was introduced wherever possible. FLINK-20589

We need to ensure that co-located tasks are executed in the same pipeline region. Hence, 8b1510c was reverted. FLINK-20589

CoLocationGroupDesc.getVertices was renamed: The refactoring changed the return type and, therefore, made the renaming necessary. FLINK-20589

The old implementation relied on the CoLocationConstraint only being created once. Switching to CoLocationConstraintDesc changed that behavior. Hence, we have to switch from IdentityHashMap to HashMap. FLINK-20589

After finalizing the refactoring of removing CoLocationConstraint and making CoLocationGroupDesc an interface, it is now reasonable to rename the affected classes and interfaces accordingly: * CoLocationGroup -> CoLocationGroupImpl * CoLocationGroupDesc -> CoLocationGroup * CoLocationConstraintDesc -> CoLocationConstraint FLINK-20589

FLINK-20589

XComp · 2021-01-11T15:16:41Z

@XComp how about we get the base PRs merged before continuing with this PR? I can see that all the base PRs are approved already.

Ok, @rmetzger merged the previous PRs. I rebased master...

zhuzhurk

Thanks for opening this PR. The changes look quite good to me. There are just several minor comments.
+1 to merge it once the minor comments are addressed.

flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/JobGraph.java

flink-runtime/src/main/java/org/apache/flink/runtime/jobmanager/scheduler/CoLocationGroup.java

flink-runtime/src/test/java/org/apache/flink/runtime/jobgraph/JobGraphTest.java

Minor renamings are applied that were brought up during the review. Additionally, missing JavaDoc was added.

rmetzger

Except for one comment, the change seems mostly fine.

rmetzger · 2021-01-13T07:17:13Z

flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/JobVertex.java

-        CoLocationGroup thisGroup = this.coLocationGroup;
-        CoLocationGroup otherGroup = strictlyCoLocatedWith.coLocationGroup;
+        CoLocationGroupImpl thisGroup = this.coLocationGroup;
+        CoLocationGroupImpl otherGroup = strictlyCoLocatedWith.coLocationGroup;


Wouldn't it be cleaner to add the addVertex() method to the CoLocationGroup interface?

The idea was to have a readonly interface as the colocation groups do not need to be changed after initialization.

I see, this is only the initialization.

rmetzger · 2021-01-13T07:31:31Z

...ming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGenerator.java

                        coLocationGroups.computeIfAbsent(
                                coLocationGroupKey,
-                                k -> new Tuple2<>(sharingGroup, new CoLocationGroup()));
+                                k -> new Tuple2<>(sharingGroup, new CoLocationGroupImpl()));


Adding the addVertex method to the interface would not require touching this test

See my answer above.

rmetzger · 2021-01-13T10:41:23Z

I'll merge the change now! Thanks for working on this!

rmetzger added the review=description? label Jan 8, 2021

tillrohrmann changed the title ~~[FLINK-20580][runtime] Removing usage of CoLocationConstraints~~ [FLINK-20850][runtime] Removing usage of CoLocationConstraints Jan 8, 2021

rmetzger added the component=Runtime/Coordination label Jan 8, 2021

XComp force-pushed the feature/FLINK-20850 branch 3 times, most recently from a722171 to 4393af4 Compare January 8, 2021 16:06

XComp added 17 commits January 11, 2021 16:09

[FLINK-20850][runtime] Remove ensureCoLocatedVerticesInSameRegion

d2cd2fc

Removed DefaultExecutionTopology.ensureCoLocatedVerticesInSameRegion(..) as CoLocationConstraint is not used anymore. FLINK-20589

[FLINK-20850][runtime] Remove CoLocationConstraint

e676e69

Remove CoLocationConstraint from ExeuctionVertexSchedulingRequirements as it is not used anymore. FLINK-20589

[FLINK-20850][runtime] Remove CoLocationConstraint-related test

d606bd9

This test verifies that the CoLocationConstraint is reset when recovering the task. We still need to verify whether it should be replaced. FLINK-20589

[FLINK-20850][runtime] Remove CoLocationConstraint-related test

343d47f

This test verifies that the CoLocationConstraints are created which is not the case anymore. FLINK-20589

[FLINK-20850][runtime] Remove CoLocation information from ExecutionVe…

5bd6cd9

…rtex The CoLocation information isn't processed through the ExecutionVertex anymore but is calculated statically by the LocalInputPreferredSlotSharingStrategy. FLINK-20589

[FLINK-20850][runtime] Remove CoLocationConstraintTest

f4dcfd1

The CoLocationConstraints are not used anymore but replaced by LocalInputPreferredSlotSharingStrategy. FLINK-20589

[FLINK-20850][runtime] Remove CoLocationConstraint from CoLocationGroup

3645bcd

The CoLocationConstraints is not used anymore and, therefore, can be removed from the CoLocationGroup. FLINK-20589

[FLINK-20850][runtime] Remove CoLocationConstraint

22d847b

CoLocationConstraint is deleted. Its JavaDoc is copied over to CoLocationConstraintDesc and adjusted accordingly. FLINK-20589

[FLINK-20850][runtime] Make CoLocationGroupDesc an interface

b062aa5

The CoLocationGroup only needs to be adapted within JobVertex. Any other locations can rely on CoLocationGroupDesc's read-only methods. FLINK-20589

[FLINK-20850][runtime] Minor cleanup

af31e07

Minor cleanup to follow Intellij suggestions. FLINK-20589

[FLINK-20850][runtime] Cleans up resetForNewExecution

ea4f9d4

Previously, the CoLocationConstraints have been reset in this method. The CoLocationGroup handling is not needed anymore after the reset functionality was removed. FLINK-20589

[FLINK-20850][runtime] Replace CoLocationGroup by corresponding inter…

ad854e3

…face The interface CoLocationGroupDesc was introduced wherever possible. FLINK-20589

[FLINK-20850][runtime] Revert 8b1510c

ddfcfed

We need to ensure that co-located tasks are executed in the same pipeline region. Hence, 8b1510c was reverted. FLINK-20589

[FLINK-20850][runtime] Renames method

f1b2df2

CoLocationGroupDesc.getVertices was renamed: The refactoring changed the return type and, therefore, made the renaming necessary. FLINK-20589

[FLINK-20850][runtime] Fix failing test

55c1b41

The old implementation relied on the CoLocationConstraint only being created once. Switching to CoLocationConstraintDesc changed that behavior. Hence, we have to switch from IdentityHashMap to HashMap. FLINK-20589

[FLINK-20850][runtime] Typo fix

7dea123

FLINK-20589

XComp force-pushed the feature/FLINK-20850 branch from daed89f to 7dea123 Compare January 11, 2021 15:12

zhuzhurk approved these changes Jan 11, 2021

View reviewed changes

[FLINK-20850][runtime] Apply renamings

51e4355

Minor renamings are applied that were brought up during the review. Additionally, missing JavaDoc was added.

rmetzger reviewed Jan 13, 2021

View reviewed changes

rmetzger closed this in ac968b8 Jan 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-20850][runtime] Removing usage of CoLocationConstraints #14584

[FLINK-20850][runtime] Removing usage of CoLocationConstraints #14584

XComp commented Jan 8, 2021

flinkbot commented Jan 8, 2021 •

edited

flinkbot commented Jan 8, 2021 •

edited

XComp commented Jan 11, 2021

zhuzhurk commented Jan 11, 2021

XComp commented Jan 11, 2021 •

edited

zhuzhurk left a comment •

edited

rmetzger left a comment

rmetzger Jan 13, 2021

XComp Jan 13, 2021

rmetzger Jan 13, 2021

rmetzger Jan 13, 2021

XComp Jan 13, 2021

rmetzger commented Jan 13, 2021

[FLINK-20850][runtime] Removing usage of CoLocationConstraints #14584

[FLINK-20850][runtime] Removing usage of CoLocationConstraints #14584

Conversation

XComp commented Jan 8, 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

flinkbot commented Jan 8, 2021 • edited

Automated Checks

Review Progress

flinkbot commented Jan 8, 2021 • edited

CI report:

XComp commented Jan 11, 2021

zhuzhurk commented Jan 11, 2021

XComp commented Jan 11, 2021 • edited

zhuzhurk left a comment • edited

Choose a reason for hiding this comment

rmetzger left a comment

Choose a reason for hiding this comment

rmetzger Jan 13, 2021

Choose a reason for hiding this comment

XComp Jan 13, 2021

Choose a reason for hiding this comment

rmetzger Jan 13, 2021

Choose a reason for hiding this comment

rmetzger Jan 13, 2021

Choose a reason for hiding this comment

XComp Jan 13, 2021

Choose a reason for hiding this comment

rmetzger commented Jan 13, 2021

flinkbot commented Jan 8, 2021 •

edited

flinkbot commented Jan 8, 2021 •

edited

XComp commented Jan 11, 2021 •

edited

zhuzhurk left a comment •

edited