-
Notifications
You must be signed in to change notification settings - Fork 749
[GOBBLIN-2227] Make dag action and spec store monitor initialization config driven #4141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GOBBLIN-2227] Make dag action and spec store monitor initialization config driven #4141
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4141 +/- ##
============================================
- Coverage 42.81% 38.75% -4.07%
+ Complexity 2480 1598 -882
============================================
Files 513 388 -125
Lines 21744 16018 -5726
Branches 2478 1589 -889
============================================
- Hits 9309 6207 -3102
+ Misses 11481 9313 -2168
+ Partials 954 498 -456 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/DagActionChangeMonitor.java
Show resolved
Hide resolved
...service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java
Show resolved
Hide resolved
...a/org/apache/gobblin/service/monitoring/DagManagementDagActionStoreChangeMonitorFactory.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * A marker interface for dag action change monitors to generalize initialization in {@link org.apache.gobblin.service.modules.core.GobblinServiceManager#dagManagementDagActionStoreChangeMonitor} | ||
| */ | ||
| public interface DagActionChangeMonitor extends Service { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given we are extending Service class, we can explore startAsync / awaitRunning method from that. setActive might not be required then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is extending Service only due to its initialization https://github.com/apache/gobblin/blob/master/gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GobblinServiceManager.java#L276 startAsync / awaitRunning is implemented by AbstractIdleService which is further extended by HighLevelConsumer, we can look to refactor in better way these later
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/SpecChangeMonitor.java
Show resolved
Hide resolved
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/SpecStoreChangeMonitor.java
Outdated
Show resolved
Hide resolved
...rvice/src/main/java/org/apache/gobblin/service/monitoring/SpecStoreChangeMonitorFactory.java
Outdated
Show resolved
Hide resolved
thisisArjit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
aga9900
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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
Description
Made the DagActionChangeMonitor, SpecStoreChangeMonitor config driven initialization
Also changed DagActionChangeMonitor, SpecStoreChangeMonitor and JobStatusMonitor to bind using interface in place of Abstract class
Tests
NA
Commits