zentol commented on a change in pull request #17387:
URL: https://github.com/apache/flink/pull/17387#discussion_r745473559
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskExecutor.java
##########
@@ -682,7 +682,9 @@ private void stopTaskExecutorServices() throws Exception {
try {
changelogStorage =
changelogStoragesManager.stateChangelogStorageForJob(
- jobId,
taskManagerConfiguration.getConfiguration());
+ jobId,
+ taskManagerConfiguration.getConfiguration(),
+
taskManagerMetricGroup.getJobMetricsGroup(jobId));
Review comment:
On the one hand it is about consistency; we only use getters for things
like the IOMetricGroups, because there we can guarantee that they actually
exists because they are created in the constructor of the containing group.
Here, we rely on some other call above that is _technically_ not quite
related to the changelog storage metrics to implicitly create the TMJMG. To
someone who doesn't know how `addTaskForJob` works (aka, that it even creates 2
groups), this isn't intuitive.
Doing 2 steps at once was fine previously because no one needed the TMJMG
anyway, but this has changed now. It should thus be split.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
