You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "X-czh (via GitHub)" <gi...@apache.org> on 2023/09/20 16:44:37 UTC
[GitHub] [flink] X-czh opened a new pull request, #23447: [FLINK-31650][metrics][rest] Cleanup terminal subtask attempt metrics to avoid confusion caused by outdated metrics
X-czh opened a new pull request, #23447:
URL: https://github.com/apache/flink/pull/23447
<!--
*Thank you very much for contributing to Apache Flink - we are happy that you want to help us improve Flink. To help the community review your contribution in the best possible way, please go through the checklist below, which will get the contribution into a shape in which it can be best reviewed.*
*Please understand that we do not do this to make contributions to Flink a hassle. In order to uphold a high standard of quality for code contributions, while at the same time managing a large number of contributions, we need contributors to prepare the contributions well, and give reviewers enough contextual information for the review. Please also understand that contributions that do not follow this guide will take longer to review and thus typically be picked up with lower priority by the community.*
## Contribution Checklist
- Make sure that the pull request corresponds to a [JIRA issue](https://issues.apache.org/jira/projects/FLINK/issues). Exceptions are made for typos in JavaDoc or documentation files, which need no JIRA issue.
- Name the pull request in the form "[FLINK-XXXX] [component] Title of the pull request", where *FLINK-XXXX* should be replaced by the actual issue number. Skip *component* if you are unsure about which is the best component.
Typo fixes that have no associated JIRA issue should be named following this pattern: `[hotfix] [docs] Fix typo in event time introduction` or `[hotfix] [javadocs] Expand JavaDoc for PuncuatedWatermarkGenerator`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Make sure that the change passes the automated tests, i.e., `mvn clean verify` passes. You can set up Azure Pipelines CI to do that following [this guide](https://cwiki.apache.org/confluence/display/FLINK/Azure+Pipelines#AzurePipelines-Tutorial:SettingupAzurePipelinesforaforkoftheFlinkrepository).
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message (including the JIRA id)
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
## What is the purpose of the change
This pull request cleanups terminal subtask attempt metrics to avoid confusion caused by outdated metrics. For example, a FINISHED task may have its last updated 100 % busy time metrics retained in the metric store and shown on th UI, which is obviously unreasonable.
## Brief change log
Removes terminal subtask attempt from the current execution attempts in `JobDetails` so that its metrics will be removed on update.
## Verifying this change
- Added UT to test that task metrics are removed when the task is removed from current execution attempts map (this cantake place when the all subtasks reached terminal state)
- Tested with a real job with a bounded source and verified on UI that the metrics were unavailable after the bounded source finished:
## 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, 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "X-czh (via GitHub)" <gi...@apache.org>.
X-czh commented on code in PR #23447:
URL: https://github.com/apache/flink/pull/23447#discussion_r1430072448
##########
flink-runtime/src/main/java/org/apache/flink/runtime/messages/webmonitor/JobDetails.java:
##########
@@ -355,16 +356,21 @@ public String toString() {
/**
* The CurrentAttempts holds the attempt number of the current representative execution attempt,
- * and the attempt numbers of all the running attempts.
+ * the attempt numbers of all the running attempts, and whether the current execution has
+ * reached terminal state.
*/
public static final class CurrentAttempts implements Serializable {
private final int representativeAttempt;
private final Set<Integer> currentAttempts;
- public CurrentAttempts(int representativeAttempt, Set<Integer> currentAttempts) {
+ private final boolean terminal;
Review Comment:
Updated, thx~
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java:
##########
@@ -57,6 +60,17 @@
public class MetricStore {
private static final Logger LOG = LoggerFactory.getLogger(MetricStore.class);
+ /**
+ * The set holds the names of the transient metrics which are no longer useful after a subtask
+ * reaches terminal state and shall be removed to avoid misleading users.
+ */
Review Comment:
Added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "wanglijie95 (via GitHub)" <gi...@apache.org>.
wanglijie95 commented on PR #23447:
URL: https://github.com/apache/flink/pull/23447#issuecomment-1869194011
Thanks @X-czh , and thanks for the review of @JunRuiLee. I will close this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] JunRuiLee commented on pull request #23447: [FLINK-31650][metrics][rest] Cleanup terminal subtask attempt metrics to avoid confusion caused by outdated metrics
Posted by "JunRuiLee (via GitHub)" <gi...@apache.org>.
JunRuiLee commented on PR #23447:
URL: https://github.com/apache/flink/pull/23447#issuecomment-1731028477
@X-czh
Hi Zhanghao, thanks for your effort.
After a quick review of your PR, if I understand correctly, your changes will remove all metrics from the operators in the terminal state.
I think this change is not entirely reliable because some non-transient metrics like numRecordsIn and numRecordsOut should not be removed just although the task state becomes finished. I would prefer to remove only those metrics that may cause confused when the tasks in the terminal state, and the decision on which metrics are unnecessary for terminal state tasks should be determined through community discussion.
However, to keep it simple for this PR, I could also accept the removal of only the backpressure-related metrics mentioned in this issue.
Both approaches are acceptable to me. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "X-czh (via GitHub)" <gi...@apache.org>.
X-czh commented on code in PR #23447:
URL: https://github.com/apache/flink/pull/23447#discussion_r1429559942
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java:
##########
@@ -126,9 +140,14 @@ public synchronized void updateCurrentExecutionAttempts(Collection<JobDetails> j
taskMetricStore.getSubtaskMetricStore(
subtaskIndex))
.ifPresent(
- subtaskMetricStore ->
- subtaskMetricStore.retainAttempts(
- attempts.getCurrentAttempts()));
+ subtaskMetricStore -> {
+ subtaskMetricStore.retainAttempts(
Review Comment:
Good catch! But I'm wondering if this is a good approach to do so here. It complicates the code and makes maintenance more difficult. Since the duplication was introduced to overcome the issue that WebInterface task metric queries currently do not account for subtasks, how about leaving it unremoved here for now and create a new JIRA on updating the WebInterface to account for subtasks? I'd be willing to help with that as well. cc @JunRuiLee @wanglijie95
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "wanglijie95 (via GitHub)" <gi...@apache.org>.
wanglijie95 closed pull request #23447: [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state
URL: https://github.com/apache/flink/pull/23447
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "X-czh (via GitHub)" <gi...@apache.org>.
X-czh commented on PR #23447:
URL: https://github.com/apache/flink/pull/23447#issuecomment-1868963641
OK, I'll prepare it tonight
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "JunRuiLee (via GitHub)" <gi...@apache.org>.
JunRuiLee commented on code in PR #23447:
URL: https://github.com/apache/flink/pull/23447#discussion_r1429512642
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java:
##########
@@ -126,9 +140,14 @@ public synchronized void updateCurrentExecutionAttempts(Collection<JobDetails> j
taskMetricStore.getSubtaskMetricStore(
subtaskIndex))
.ifPresent(
- subtaskMetricStore ->
- subtaskMetricStore.retainAttempts(
- attempts.getCurrentAttempts()));
+ subtaskMetricStore -> {
+ subtaskMetricStore.retainAttempts(
Review Comment:
Since the metrics for subtasks also exist in the taskMetricsStore in the form of taskInfo.subtaskIndex + "." + name, it is also necessary to clean up the transient metrics stored in the taskMetricsStore. Otherwise, it could lead to inconsistent behaviors that may confuse users, as depicted in the screenshot below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "wanglijie95 (via GitHub)" <gi...@apache.org>.
wanglijie95 commented on PR #23447:
URL: https://github.com/apache/flink/pull/23447#issuecomment-1868951155
Hi @X-czh, kindly remind again, we need a PR for release-1.17 ~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "X-czh (via GitHub)" <gi...@apache.org>.
X-czh commented on PR #23447:
URL: https://github.com/apache/flink/pull/23447#issuecomment-1859486000
@wanglijie95 Kindly remind~ Could you help take a look when you have time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "X-czh (via GitHub)" <gi...@apache.org>.
X-czh commented on PR #23447:
URL: https://github.com/apache/flink/pull/23447#issuecomment-1862095289
@wanglijie95 Squashed and rebased, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "X-czh (via GitHub)" <gi...@apache.org>.
X-czh commented on PR #23447:
URL: https://github.com/apache/flink/pull/23447#issuecomment-1864445063
Sure, no problem
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] X-czh commented on pull request #23447: [FLINK-31650][metrics][rest] Cleanup terminal subtask attempt metrics to avoid confusion caused by outdated metrics
Posted by "X-czh (via GitHub)" <gi...@apache.org>.
X-czh commented on PR #23447:
URL: https://github.com/apache/flink/pull/23447#issuecomment-1731133109
I previously thought it fine to completely remove metrics of terminal vertices, as the core I/O metrics displayed on the UI of terminal vertices are actually accessed via the metrics stored in `AccessExecution` and would be affected. But after some offline discussion with my colleagues, I realized some users may rely on it. So +1 for your suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "wanglijie95 (via GitHub)" <gi...@apache.org>.
wanglijie95 commented on PR #23447:
URL: https://github.com/apache/flink/pull/23447#issuecomment-1862700155
@X-czh Could you prepare a backport PR for branch release-1.17 ? It has conflicts when I cherry-pick the changes here to release-1.17
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "JunRuiLee (via GitHub)" <gi...@apache.org>.
JunRuiLee commented on code in PR #23447:
URL: https://github.com/apache/flink/pull/23447#discussion_r1430850407
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java:
##########
@@ -570,6 +578,19 @@ void retainSubtasks(Set<Integer> activeSubtasks) {
subtasks.keySet().retainAll(activeSubtasks);
}
+ void removeTransientMetrics(int subtaskIndex) {
+ if (subtasks.containsKey(subtaskIndex)) {
+ // Remove in both places as task metrics are duplicated in task metric store and
+ // subtask metric store for metrics query on WebInterface.
+ metrics.keySet()
Review Comment:
I prefer not specifically mentioning 'query on WebInterface' since the metric store also exposes metrics to users through the REST API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Cleanup terminal subtask attempt metrics to avoid confusion caused by outdated metrics [flink]
Posted by "X-czh (via GitHub)" <gi...@apache.org>.
X-czh commented on PR #23447:
URL: https://github.com/apache/flink/pull/23447#issuecomment-1826376453
Hi @JunRuiLee. It's been a while, I've updated the PR to remove the transient metrics (idle/busy/backpressured time) for terminal subtasks only. Could you help take a review when you are free? Many thanks in advance~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "X-czh (via GitHub)" <gi...@apache.org>.
X-czh commented on code in PR #23447:
URL: https://github.com/apache/flink/pull/23447#discussion_r1429759033
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java:
##########
@@ -126,9 +140,14 @@ public synchronized void updateCurrentExecutionAttempts(Collection<JobDetails> j
taskMetricStore.getSubtaskMetricStore(
subtaskIndex))
.ifPresent(
- subtaskMetricStore ->
- subtaskMetricStore.retainAttempts(
- attempts.getCurrentAttempts()));
+ subtaskMetricStore -> {
+ subtaskMetricStore.retainAttempts(
Review Comment:
Hi @JunRuiLee, thanks for the clarification. I get your point and my concern is actual about maintainability of the code in the future. When there're both insertion and deletion operations, the duplication of task-level metrics actually makes it more difficult to maintain consistency (as is the case here), so I think it would be better to optimize the duplication issue here in the future.
I'll clean up simultaneously for both cases first here, and create a new issue for the optimization later. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "JunRuiLee (via GitHub)" <gi...@apache.org>.
JunRuiLee commented on code in PR #23447:
URL: https://github.com/apache/flink/pull/23447#discussion_r1429785675
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java:
##########
@@ -126,9 +140,14 @@ public synchronized void updateCurrentExecutionAttempts(Collection<JobDetails> j
taskMetricStore.getSubtaskMetricStore(
subtaskIndex))
.ifPresent(
- subtaskMetricStore ->
- subtaskMetricStore.retainAttempts(
- attempts.getCurrentAttempts()));
+ subtaskMetricStore -> {
+ subtaskMetricStore.retainAttempts(
Review Comment:
Clean up simultaneously for both cases first here sounds good to me, Please feel free and go ahead~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #23447: [FLINK-31650][metrics][rest] Cleanup terminal subtask attempt metrics to avoid confusion caused by outdated metrics
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #23447:
URL: https://github.com/apache/flink/pull/23447#issuecomment-1728104779
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a3d22bcb204a26716c51f71ef66c0cf58e5fff92",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "a3d22bcb204a26716c51f71ef66c0cf58e5fff92",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a3d22bcb204a26716c51f71ef66c0cf58e5fff92 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] X-czh commented on pull request #23447: [FLINK-31650][metrics][rest] Cleanup terminal subtask attempt metrics to avoid confusion caused by outdated metrics
Posted by "X-czh (via GitHub)" <gi...@apache.org>.
X-czh commented on PR #23447:
URL: https://github.com/apache/flink/pull/23447#issuecomment-1731130211
Hi @JunRuiLee, thanks for reviewing. +1 for the need to preserve some non-transient metrics like numRecordsIn and numRecordsOut after vertices reaching terminal state. I'll redesign to remove only the backpressure-related metrics mentioned in this issue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "X-czh (via GitHub)" <gi...@apache.org>.
X-czh commented on code in PR #23447:
URL: https://github.com/apache/flink/pull/23447#discussion_r1430859019
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java:
##########
@@ -570,6 +578,19 @@ void retainSubtasks(Set<Integer> activeSubtasks) {
subtasks.keySet().retainAll(activeSubtasks);
}
+ void removeTransientMetrics(int subtaskIndex) {
+ if (subtasks.containsKey(subtaskIndex)) {
+ // Remove in both places as task metrics are duplicated in task metric store and
+ // subtask metric store for metrics query on WebInterface.
+ metrics.keySet()
Review Comment:
Addressed, thx
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "wanglijie95 (via GitHub)" <gi...@apache.org>.
wanglijie95 commented on code in PR #23447:
URL: https://github.com/apache/flink/pull/23447#discussion_r1429465647
##########
flink-runtime/src/main/java/org/apache/flink/runtime/messages/webmonitor/JobDetails.java:
##########
@@ -355,16 +356,21 @@ public String toString() {
/**
* The CurrentAttempts holds the attempt number of the current representative execution attempt,
- * and the attempt numbers of all the running attempts.
+ * the attempt numbers of all the running attempts, and whether the current execution has
+ * reached terminal state.
*/
public static final class CurrentAttempts implements Serializable {
private final int representativeAttempt;
private final Set<Integer> currentAttempts;
- public CurrentAttempts(int representativeAttempt, Set<Integer> currentAttempts) {
+ private final boolean terminal;
Review Comment:
Maybe `terminal` -> `reachTerminalState` or `isTerminalState`
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java:
##########
@@ -57,6 +60,17 @@
public class MetricStore {
private static final Logger LOG = LoggerFactory.getLogger(MetricStore.class);
+ /**
+ * The set holds the names of the transient metrics which are no longer useful after a subtask
+ * reaches terminal state and shall be removed to avoid misleading users.
+ */
Review Comment:
Maybe add one more line: "Note that there may be other transient metrics, we currently only support cleaning these three"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "JunRuiLee (via GitHub)" <gi...@apache.org>.
JunRuiLee commented on code in PR #23447:
URL: https://github.com/apache/flink/pull/23447#discussion_r1429717030
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java:
##########
@@ -126,9 +140,14 @@ public synchronized void updateCurrentExecutionAttempts(Collection<JobDetails> j
taskMetricStore.getSubtaskMetricStore(
subtaskIndex))
.ifPresent(
- subtaskMetricStore ->
- subtaskMetricStore.retainAttempts(
- attempts.getCurrentAttempts()));
+ subtaskMetricStore -> {
+ subtaskMetricStore.retainAttempts(
Review Comment:
Thanks for clarification @X-czh .
I'm not quite certain I understand your concern.
In my opinion, this issue is unrelated to the web interface and more related to the inconsistency in the MetricStore because the WebUI is also get data from MetricStore. Specifically, the metrics in the subtaskMetricsStore are being removed, while the metrics in the taskMetricsStore are not synchronously removed, which could be confusing for users.
Based on your changes, you can perform the following test:
For a jobVertex that has already finished, you can use the JobVertexMetricsHandler to retrieve subtask metrics like below:
http://localhost:8081/jobs//vertices//metrics?get=0.backPressuredTimeMsPerSecond,0.busyTimeMsPerSecond
Then, compare the results with the SubtaskMetricsHandler:
http://localhost:8081/jobs//vertices//subtasks/0/metrics?get=backPressuredTimeMsPerSecond,busyTimeMsPerSecond
The results from these two endpoints are different. In my local test, the results are as shown in the attached image. I prefer that cleaning up should be done simultaneously for both, WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-31650][metrics][rest] Remove transient metrics for subtasks in terminal state [flink]
Posted by "X-czh (via GitHub)" <gi...@apache.org>.
X-czh commented on code in PR #23447:
URL: https://github.com/apache/flink/pull/23447#discussion_r1430072844
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java:
##########
@@ -126,9 +140,14 @@ public synchronized void updateCurrentExecutionAttempts(Collection<JobDetails> j
taskMetricStore.getSubtaskMetricStore(
subtaskIndex))
.ifPresent(
- subtaskMetricStore ->
- subtaskMetricStore.retainAttempts(
- attempts.getCurrentAttempts()));
+ subtaskMetricStore -> {
+ subtaskMetricStore.retainAttempts(
Review Comment:
Updated, PTAL when you are free, thx~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org