phet commented on a change in pull request #3454:
URL: https://github.com/apache/gobblin/pull/3454#discussion_r791097576
##########
File path:
gobblin-metrics-libs/gobblin-metrics-base/src/test/java/org/apache/gobblin/metrics/metric/filter/MetricNameRegexFilterTest.java
##########
@@ -42,5 +42,11 @@ public void matchesTest() {
Assert.assertTrue(metricNameRegexFilter2.matches("test1",
mock(Metric.class)));
Assert.assertFalse(metricNameRegexFilter2.matches("test2",
mock(Metric.class)));
Assert.assertFalse(metricNameRegexFilter2.matches("test3",
mock(Metric.class)));
+ MetricNameRegexFilter metricNameRegexFilter3 = new
MetricNameRegexFilter("GobblinService\\..*\\.RunningStatus");
Review comment:
I don't want to go for overkill, yet I'd hope to validate the connection
between this regex hard-coded here and what's constructed in the impl:
```
ServiceMetricNames.GOBBLIN_SERVICE_PREFIX + "\\..*\\." +
ServiceMetricNames.RUNNING_STATUS
```
maybe add a static method there to constructs the regex and then have the
test call it to set `metricNameRegexFilter3`?
##########
File path:
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -399,6 +400,7 @@ public synchronized void setActive(boolean active) {
} else { //Mark the DagManager inactive.
log.info("Inactivating the DagManager. Shutting down all DagManager
threads");
this.scheduledExecutorPool.shutdown();
+ RootMetricContext.get().removeMatching(new
MetricNameRegexFilter(ServiceMetricNames.GOBBLIN_SERVICE_PREFIX + "\\..*\\." +
ServiceMetricNames.RUNNING_STATUS));
Review comment:
let's add a comment here about background info (since a bit complex how
several pieces conspire to create the issue)... something like:
the DMThread's metrics mappings follow the lifecycle of the DMThread itself
and so are lost by DM deactivation-reactivation. the `RootMetricContext` OTOH
is a (persistent) singleton. to avoid XYZ exception by the RMC preventing
(re-)add of a metric already known, remove all metrics that a new DMThread
thread would attempt to add (in `DagManagerThread::initialize`) whenever
running post-re-enablement
--
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]
