You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2022/01/22 00:14:22 UTC

[GitHub] [gobblin] ZihanLi58 opened a new pull request #3454: [GOBBLIN-1598]Fix metrics already exist issue in dag manager

ZihanLi58 opened a new pull request #3454:
URL: https://github.com/apache/gobblin/pull/3454


   Dear Gobblin maintainers,
   
   Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
   
   ### JIRA
   - [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
       - https://issues.apache.org/jira/browse/GOBBLIN-1598
   
   
   ### Description
   - [ ] Here are some details about my PR, including screenshots (if applicable):
   2021/06/12 07:11:32.067 ERROR [DagManager] [pool-19-thread-3] [gobblin-service-war] [] Exception encountered in org.apache.gobblin.service.modules.orchestration.DagManager$DagManagerThread
   java.lang.IllegalArgumentException: A metric named GobblinService.testFlow.RunningStatus already exists
    at org.apache.gobblin.metrics.InnerMetricContext.register(InnerMetricContext.java:266) 
   
   
   Likely root cause:
   
   If we activate/deactivate DagManager several times, DagManagerThreads alongside with their flowGauges maps will be recreated, but RootMetricContext will still stay the same. So the newly created threads will incorrectly think that the flow was not registered previously, and try to register it again.
   
   ### Tests
   - [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
   Unit tests
   
   ### Commits
   - [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
       1. Subject is separated from body by a blank line
       2. Subject is limited to 50 characters
       3. Subject does not end with a period
       4. Subject uses the imperative mood ("add", not "adding")
       5. Body wraps at 72 characters
       6. Body explains "what" and "why", not "how"
   
   


-- 
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] codecov-commenter commented on pull request #3454: [GOBBLIN-1598]Fix metrics already exist issue in dag manager

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #3454:
URL: https://github.com/apache/gobblin/pull/3454#issuecomment-1020670585


   # [Codecov](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#3454](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (60cc565) into [master](https://codecov.io/gh/apache/gobblin/commit/0213b1e89822f5d53f27a3d963f917e0d871886f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0213b1e) will **decrease** coverage by `0.00%`.
   > The diff coverage is `50.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/gobblin/pull/3454/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3454      +/-   ##
   ============================================
   - Coverage     46.63%   46.62%   -0.01%     
   - Complexity    10342    10346       +4     
   ============================================
     Files          2074     2074              
     Lines         80962    80964       +2     
     Branches       9031     9031              
   ============================================
   - Hits          37759    37753       -6     
   - Misses        39728    39736       +8     
     Partials       3475     3475              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...blin/service/modules/orchestration/DagManager.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXIuamF2YQ==) | `71.29% <50.00%> (-0.07%)` | :arrow_down: |
   | [.../modules/scheduler/GobblinServiceJobScheduler.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zY2hlZHVsZXIvR29iYmxpblNlcnZpY2VKb2JTY2hlZHVsZXIuamF2YQ==) | `63.68% <0.00%> (-5.48%)` | :arrow_down: |
   | [...in/java/org/apache/gobblin/cluster/HelixUtils.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhVdGlscy5qYXZh) | `30.43% <0.00%> (-2.90%)` | :arrow_down: |
   | [.../org/apache/gobblin/cluster/GobblinTaskRunner.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpblRhc2tSdW5uZXIuamF2YQ==) | `63.46% <0.00%> (-0.33%)` | :arrow_down: |
   | [...anagement/copy/replication/ConfigBasedDataset.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvcmVwbGljYXRpb24vQ29uZmlnQmFzZWREYXRhc2V0LmphdmE=) | `68.87% <0.00%> (ø)` | |
   | [...main/java/org/apache/gobblin/yarn/YarnService.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vWWFyblNlcnZpY2UuamF2YQ==) | `16.23% <0.00%> (+0.77%)` | :arrow_up: |
   | [...ata/management/copy/hive/HiveCopyEntityHelper.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvaGl2ZS9IaXZlQ29weUVudGl0eUhlbHBlci5qYXZh) | `61.12% <0.00%> (+0.84%)` | :arrow_up: |
   | [...gobblin/service/modules/core/GitConfigMonitor.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9jb3JlL0dpdENvbmZpZ01vbml0b3IuamF2YQ==) | `83.05% <0.00%> (+1.69%)` | :arrow_up: |
   | [.../java/org/apache/gobblin/cluster/SleepingTask.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2xlZXBpbmdUYXNrLmphdmE=) | `45.45% <0.00%> (+6.06%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [0213b1e...60cc565](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] phet commented on a change in pull request #3454: [GOBBLIN-1598]Fix metrics already exist issue in dag manager

Posted by GitBox <gi...@apache.org>.
phet commented on a change in pull request #3454:
URL: https://github.com/apache/gobblin/pull/3454#discussion_r791313196



##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -411,6 +416,11 @@ public synchronized void setActive(boolean active) {
     }
   }
 
+  @VisibleForTesting
+  protected static MetricNameRegexFilter getMetricsFilterForDagManager() {

Review comment:
       minor: since part of the `DagManager` class seems redundant to add suffix, `ForDagManager`.
   
   (e.g. look how it appears when called in the test)




-- 
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] phet commented on a change in pull request #3454: [GOBBLIN-1598]Fix metrics already exist issue in dag manager

Posted by GitBox <gi...@apache.org>.
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

##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -411,6 +416,11 @@ public synchronized void setActive(boolean active) {
     }
   }
 
+  @VisibleForTesting
+  protected static MetricNameRegexFilter getMetricsFilterForDagManager() {

Review comment:
       minor: since part of the `DagManager` class seems redundant to add suffix, `ForDagManager`.
   
   (e.g. look how it appears when called in the test)




-- 
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] arjun4084346 commented on pull request #3454: [GOBBLIN-1598]Fix metrics already exist issue in dag manager

Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on pull request #3454:
URL: https://github.com/apache/gobblin/pull/3454#issuecomment-1019427191


   LGTM if you are trying to fix this one type of metric (RunningStatus). Any plans to make it scalable to all metrics in future?


-- 
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] ZihanLi58 merged pull request #3454: [GOBBLIN-1598]Fix metrics already exist issue in dag manager

Posted by GitBox <gi...@apache.org>.
ZihanLi58 merged pull request #3454:
URL: https://github.com/apache/gobblin/pull/3454


   


-- 
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] ZihanLi58 commented on pull request #3454: [GOBBLIN-1598]Fix metrics already exist issue in dag manager

Posted by GitBox <gi...@apache.org>.
ZihanLi58 commented on pull request #3454:
URL: https://github.com/apache/gobblin/pull/3454#issuecomment-1018985988


   @phet @arjun4084346 can you help review it 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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] codecov-commenter commented on pull request #3454: [GOBBLIN-1598]Fix metrics already exist issue in dag manager

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #3454:
URL: https://github.com/apache/gobblin/pull/3454#issuecomment-1020670585


   # [Codecov](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#3454](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (60cc565) into [master](https://codecov.io/gh/apache/gobblin/commit/0213b1e89822f5d53f27a3d963f917e0d871886f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0213b1e) will **decrease** coverage by `0.00%`.
   > The diff coverage is `50.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/gobblin/pull/3454/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3454      +/-   ##
   ============================================
   - Coverage     46.63%   46.62%   -0.01%     
   - Complexity    10342    10346       +4     
   ============================================
     Files          2074     2074              
     Lines         80962    80964       +2     
     Branches       9031     9031              
   ============================================
   - Hits          37759    37753       -6     
   - Misses        39728    39736       +8     
     Partials       3475     3475              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...blin/service/modules/orchestration/DagManager.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXIuamF2YQ==) | `71.29% <50.00%> (-0.07%)` | :arrow_down: |
   | [.../modules/scheduler/GobblinServiceJobScheduler.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zY2hlZHVsZXIvR29iYmxpblNlcnZpY2VKb2JTY2hlZHVsZXIuamF2YQ==) | `63.68% <0.00%> (-5.48%)` | :arrow_down: |
   | [...in/java/org/apache/gobblin/cluster/HelixUtils.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhVdGlscy5qYXZh) | `30.43% <0.00%> (-2.90%)` | :arrow_down: |
   | [.../org/apache/gobblin/cluster/GobblinTaskRunner.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpblRhc2tSdW5uZXIuamF2YQ==) | `63.46% <0.00%> (-0.33%)` | :arrow_down: |
   | [...anagement/copy/replication/ConfigBasedDataset.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvcmVwbGljYXRpb24vQ29uZmlnQmFzZWREYXRhc2V0LmphdmE=) | `68.87% <0.00%> (ø)` | |
   | [...main/java/org/apache/gobblin/yarn/YarnService.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vWWFyblNlcnZpY2UuamF2YQ==) | `16.23% <0.00%> (+0.77%)` | :arrow_up: |
   | [...ata/management/copy/hive/HiveCopyEntityHelper.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvaGl2ZS9IaXZlQ29weUVudGl0eUhlbHBlci5qYXZh) | `61.12% <0.00%> (+0.84%)` | :arrow_up: |
   | [...gobblin/service/modules/core/GitConfigMonitor.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9jb3JlL0dpdENvbmZpZ01vbml0b3IuamF2YQ==) | `83.05% <0.00%> (+1.69%)` | :arrow_up: |
   | [.../java/org/apache/gobblin/cluster/SleepingTask.java](https://codecov.io/gh/apache/gobblin/pull/3454/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2xlZXBpbmdUYXNrLmphdmE=) | `45.45% <0.00%> (+6.06%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [0213b1e...60cc565](https://codecov.io/gh/apache/gobblin/pull/3454?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] ZihanLi58 merged pull request #3454: [GOBBLIN-1598]Fix metrics already exist issue in dag manager

Posted by GitBox <gi...@apache.org>.
ZihanLi58 merged pull request #3454:
URL: https://github.com/apache/gobblin/pull/3454


   


-- 
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] phet commented on a change in pull request #3454: [GOBBLIN-1598]Fix metrics already exist issue in dag manager

Posted by GitBox <gi...@apache.org>.
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org