phet commented on code in PR #3840:
URL: https://github.com/apache/gobblin/pull/3840#discussion_r1412677033
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManagerMetrics.java:
##########
@@ -76,8 +76,9 @@ public class DagManagerMetrics {
private final Map<String, ContextAwareMeter> executorSlaExceededMeters =
Maps.newConcurrentMap();
private final Map<String, ContextAwareMeter> executorJobSentMeters =
Maps.newConcurrentMap();
- // Metrics for unexpected flow handling failures
+ // Metric for unexpected flow handling failures
private ContextAwareCounter failedLaunchEventsOnActivationCount;
+ private ContextAwareCounter successfulLaunchEventsOnActivationCount;
Review Comment:
now these are for flow handling outcomes (although they're both only for
startup, so not in the general sense)
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GobblinServiceGuiceModule.java:
##########
@@ -200,6 +200,12 @@ public void configure(Binder binder) {
ServiceConfigKeys.TOPOLOGYSPEC_FACTORY_KEY,
ServiceConfigKeys.DEFAULT_TOPOLOGY_SPEC_FACTORY));
}
+ // Initialize flow catalog before DagManager is enabled so templates are
loaded when initializing the DagManager
Review Comment:
I'm not too familiar w/ Guice... but are you sure the call to `binder.bind`
is guaranteed to synchronously carryout "initializing the flow catalog" (so
that finishes before the later call to `binder.bind(DagManager.class)`)?
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManagerMetrics.java:
##########
@@ -212,6 +216,13 @@ public void incrementFailedLaunchCount() {
}
}
+ // Increment the count for num of successful launches attempted during
leader activation
+ public void incrementSuccessfulLaunchAttemptCount() {
Review Comment:
I see the comment above (should be javadoc), but the method name invites
confusion, since it sounds like this is for counting successes, irrespective of
whether or not during startup
(BTW, "system startup" is slightly more accurate for our multi-leader arch
than "leader activation" )
--
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]
