[FLINK-24864][metrics] Release TaskManagerJobMetricGroup with the last slot rather than task #17387

rkhachatryan · 2021-09-29T13:42:13Z

What is the purpose of the change

Release TaskManagerJobMetricGroup on last job slot release to avoid issues when using it with StateChangelogStorage, which is released on last slot release (please see ticket description for more details).

Verifying this change

added TaskExecutorTest.testReleasingJobResources
added TaskManagerMetricGroupTest

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? no

flinkbot · 2021-09-29T13:46:18Z

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 3825d66 (Wed Sep 29 13:46:18 UTC 2021)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!
This pull request references an unassigned Jira ticket. According to the code contribution guide, tickets need to be assigned before starting with the implementation work.

_{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-09-29T14:43:53Z

CI report:

1a458e1 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

rkhachatryan · 2021-11-06T09:59:28Z

Build failure unrelated (FLINK-24806).

zentol

I'd say the change to the TMJMG lifecycle exceeds the scope of a hotfix by quite a bit.

flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/TaskManagerMetricGroup.java

...runtime/src/main/java/org/apache/flink/runtime/metrics/groups/TaskManagerJobMetricGroup.java

zentol · 2021-11-07T22:42:21Z

flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/TaskManagerMetricGroup.java

-    public void removeJobMetricsGroup(JobID jobId, TaskManagerJobMetricGroup group) {
-        if (jobId == null || group == null || !group.isClosed()) {
-            return;
+        if (jobGroup == null) {


why are you no longer checking whether the jobGroup is closed?

The check is already done in close() method itself (and it cannot be removed from there because of concurrency).
And IIUC, this method removeJobMetricsGroup can not be called after close because they are called from
TaskExecutor.releaseJobResources and stopTaskExecutorServices respectively.

flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/TaskManagerMetricGroup.java

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

flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/TaskManagerMetricGroup.java

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

rkhachatryan · 2021-11-08T10:27:01Z

Thanks for the feedback @zentol !

I'd say the change to the TMJMG lifecycle exceeds the scope of a hotfix by quite a bit.

Would you prefer this change to be in a separate ticket and PR?

.../flink-dstl-dfs/src/main/java/org/apache/flink/changelog/fs/ChangelogStorageMetricGroup.java

zentol · 2021-11-09T10:29:44Z

Would you prefer this change to be in a separate ticket and PR?

A separate ticket for sure; whether we need a separate PR is up to you (as you'll need to manage the discussions/commits, so pick whatever works best for you).

rkhachatryan · 2021-11-10T15:42:11Z

I've created a separate ticket (FLINK-24864) and PR (#17757) for changelog-related changes.
This PR and ticket now only contains TMJMG lifecycle changes.

I've also rebased it onto latest master and addressed your feedback, PTAL.

flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/TaskManagerMetricGroup.java

...untime/src/test/java/org/apache/flink/runtime/metrics/groups/TaskManagerMetricGroupTest.java

...runtime/src/main/java/org/apache/flink/runtime/metrics/groups/TaskManagerJobMetricGroup.java

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

zentol

+1

…t slot rather than task Motivation: TaskManagerJobMetricGroup might be used by components shared across tasks of a job (e.g. ChangelogStorage). The lifecycle of those is bound to slots rather than tasks (for various reasons including performance). Releasing them differently causes those metrics to be not reported. Besides that, this simplifies release code in TaskManagerJobMetricGroup and makes its lifecycle more consistent (creation and deletion in the same place).

rkhachatryan · 2021-11-16T08:22:13Z

Thanks @zentol for reviewing.
Merged as f849ef3 into master.

rmetzger added component=Runtime/StateBackends component=Runtime/Metrics labels Sep 29, 2021

rkhachatryan changed the title ~~[WIP][FLINK-23486][state] Changelog Backend metrics~~ [WIP][FLINK-23486][state] ChangelogUploader metrics Nov 2, 2021

rkhachatryan force-pushed the f23486 branch 3 times, most recently from 1736c54 to d8834de Compare November 5, 2021 22:25

rkhachatryan changed the title ~~[WIP][FLINK-23486][state] ChangelogUploader metrics~~ [FLINK-23486][state] ChangelogUploader metrics Nov 5, 2021

rkhachatryan marked this pull request as ready for review November 5, 2021 22:29

rkhachatryan requested review from zentol and pnowojski November 5, 2021 22:30

rkhachatryan force-pushed the f23486 branch 2 times, most recently from fc8a945 to 2db1ab7 Compare November 5, 2021 23:12

zentol requested changes Nov 7, 2021

View reviewed changes

zentol reviewed Nov 9, 2021

View reviewed changes

.../flink-dstl-dfs/src/main/java/org/apache/flink/changelog/fs/ChangelogStorageMetricGroup.java Outdated Show resolved Hide resolved

rkhachatryan changed the title ~~[FLINK-23486][state] ChangelogUploader metrics~~ [FLINK-24864][metrics] Release TaskManagerJobMetricGroup with the last slot rather than task Nov 10, 2021

rkhachatryan mentioned this pull request Nov 10, 2021

[FLINK-23486][state][metrics] Add metrics for the Changelog uploader #17757

Merged

rkhachatryan force-pushed the f23486 branch from 2db1ab7 to 5188cc8 Compare November 10, 2021 15:39

zentol requested changes Nov 11, 2021

View reviewed changes

zentol self-assigned this Nov 11, 2021

zentol approved these changes Nov 15, 2021

View reviewed changes

rkhachatryan force-pushed the f23486 branch from 5687d34 to 3f327ff Compare November 15, 2021 16:50

rkhachatryan force-pushed the f23486 branch from 3f327ff to 1a458e1 Compare November 15, 2021 21:43

rkhachatryan merged commit f849ef3 into apache:master Nov 16, 2021

rkhachatryan deleted the f23486 branch November 16, 2021 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-24864][metrics] Release TaskManagerJobMetricGroup with the last slot rather than task #17387

[FLINK-24864][metrics] Release TaskManagerJobMetricGroup with the last slot rather than task #17387

rkhachatryan commented Sep 29, 2021 •

edited

flinkbot commented Sep 29, 2021

flinkbot commented Sep 29, 2021 •

edited

rkhachatryan commented Nov 6, 2021

zentol left a comment

zentol Nov 7, 2021

rkhachatryan Nov 8, 2021

rkhachatryan commented Nov 8, 2021

zentol commented Nov 9, 2021

rkhachatryan commented Nov 10, 2021

zentol left a comment

rkhachatryan commented Nov 16, 2021

[FLINK-24864][metrics] Release TaskManagerJobMetricGroup with the last slot rather than task #17387

[FLINK-24864][metrics] Release TaskManagerJobMetricGroup with the last slot rather than task #17387

Conversation

rkhachatryan commented Sep 29, 2021 • edited

What is the purpose of the change

Verifying this change

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

Documentation

flinkbot commented Sep 29, 2021

Automated Checks

Review Progress

flinkbot commented Sep 29, 2021 • edited

CI report:

rkhachatryan commented Nov 6, 2021

zentol left a comment

Choose a reason for hiding this comment

zentol Nov 7, 2021

Choose a reason for hiding this comment

rkhachatryan Nov 8, 2021

Choose a reason for hiding this comment

rkhachatryan commented Nov 8, 2021

zentol commented Nov 9, 2021

rkhachatryan commented Nov 10, 2021

zentol left a comment

Choose a reason for hiding this comment

rkhachatryan commented Nov 16, 2021

rkhachatryan commented Sep 29, 2021 •

edited

flinkbot commented Sep 29, 2021 •

edited