zentol commented on a change in pull request #18749:
URL: https://github.com/apache/flink/pull/18749#discussion_r805829724
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/dispatcher/cleanup/DispatcherResourceCleanerFactoryTest.java
##########
@@ -192,7 +192,6 @@ private void assertLocalCleanupNotTriggered() {
assertThat(jobManagerRunnerRegistryLocalCleanupFuture).isNotDone();
assertThat(jobGraphWriterLocalCleanupFuture).isNotDone();
assertThat(blobServer.getLocalCleanupFuture()).isNotDone();
-
assertThat(jobManagerMetricGroup.numRegisteredJobMetricGroups()).isEqualTo(1);
Review comment:
is it not triggered because the global cleanup runs first?
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/JobManagerMetricGroup.java
##########
@@ -38,7 +39,7 @@
* tasks any more
*/
public class JobManagerMetricGroup extends
ComponentMetricGroup<JobManagerMetricGroup>
- implements LocallyCleanableResource {
+ implements LocallyCleanableResource, GloballyCleanableResource {
Review comment:
Please expand on why this is a global resource. Is it because it the
contained metrics _may_ be persisted to an external system?
I'm confused since GloballyCleanableResource says that global resources must
survive a failover, but that's generally not the case for the JMJMG.
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/JobManagerMetricGroup.java
##########
@@ -38,7 +39,7 @@
* tasks any more
*/
public class JobManagerMetricGroup extends
ComponentMetricGroup<JobManagerMetricGroup>
- implements LocallyCleanableResource {
+ implements LocallyCleanableResource, GloballyCleanableResource {
Review comment:
To add to that, what are the actual semantics when implementing both the
local & global variants?
Local says they are cleaned up on a locally-terminated state, global says
_only_ after a globally-terminal state.
Now which is it?
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/dispatcher/cleanup/DispatcherResourceCleanerFactory.java
##########
@@ -108,6 +108,7 @@ public ResourceCleaner createGlobalResourceCleaner(
.withRegularCleanup(jobGraphWriter)
.withRegularCleanup(blobServer)
.withRegularCleanup(highAvailabilityServices)
+ .withRegularCleanup(jobManagerMetricGroup)
Review comment:
Off-topic: Why are there no javadocs explaining the difference between
regular and prioritized cleanups?
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/JobManagerMetricGroup.java
##########
@@ -38,7 +39,7 @@
* tasks any more
*/
public class JobManagerMetricGroup extends
ComponentMetricGroup<JobManagerMetricGroup>
- implements LocallyCleanableResource {
+ implements LocallyCleanableResource, GloballyCleanableResource {
Review comment:
Off-topic: The GloballyCleanableResource javadocs seem incorrect as they
should link to LocallyCleanableResource in Line 3.
--
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]
