phet commented on code in PR #3800:
URL: https://github.com/apache/gobblin/pull/3800#discussion_r1361208310
##########
gobblin-metrics-libs/gobblin-metrics/src/main/java/org/apache/gobblin/metrics/ServiceMetricNames.java:
##########
@@ -43,6 +43,12 @@ public class ServiceMetricNames {
public static final String FLOW_TRIGGER_HANDLER_JOB_DOES_NOT_EXIST_COUNT =
GOBBLIN_SERVICE_PREFIX + "." + FLOW_TRIGGER_HANDLER_PREFIX +
".jobDoesNotExistInScheduler";
public static final String FLOW_TRIGGER_HANDLER_FAILED_TO_SET_REMINDER_COUNT
= GOBBLIN_SERVICE_PREFIX + "." + FLOW_TRIGGER_HANDLER_PREFIX +
".failedToSetReminderCount";
+ // DagManager Related Metrics
+ public static final String DAG_MANAGER_HANDLING_PREFIX =
GOBBLIN_SERVICE_PREFIX + ".dagManagerHandling";
Review Comment:
overall, this is minor... not wanting to press too hard here.
just observing that `DagManagerHandling`, not being an actual class name,
seems a synonym for `DagMgrWorking`, `DagMgrInAction`, `DagMgrDoingStuff`,
which is to say it's vague. even more, DM "doing stuff" is implied simply by
the prefix `DagMgr`.
so if there is something you want to differentiate here, that's fine... but
do make sure what that is is clearly stated, so a future maintainer knows
whether the metric they wish to add belongs under `DagMgr` or your new prefix.
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/Orchestrator.java:
##########
@@ -347,9 +352,13 @@ public void submitFlowToDagManager(FlowSpec flowSpec,
Dag<JobExecutionPlan> jobE
//Send the dag to the DagManager.
this.dagManager.get().addDag(jobExecutionPlanDag, true, true);
} catch (Exception ex) {
+ String failureMessage = "Failed to add Job Execution Plan due to: " +
ex.getMessage();
+ _log.warn("Orchestrator call - " + failureMessage);
Review Comment:
since we don't expect this very frequently, we may prefer to print the stack
trace rather than merely the exception msg
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -620,6 +627,8 @@ public void run() {
}
//Initialize dag.
initialize(dag);
+ } else {
+ log.warn("Null dag; ignoring the dag");
Review Comment:
key point is "null dag, despite non-empty queue"
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -509,14 +511,19 @@ public void
handleLaunchFlowEvent(DagActionStore.DagAction launchAction) {
// Upon handling the action, delete it so on leadership change this is
not duplicated
this.dagActionStore.get().deleteDagAction(launchAction);
} catch (URISyntaxException e) {
- log.warn("Could not create URI object for flowId {} due to exception
{}", flowId, e.getMessage());
+ log.warn(String.format("Could not create URI object for flowId %s due to
exception", flowId), e.fillInStackTrace());
Review Comment:
don't we want the true stack trace of the exception? I would just pass this
as `e` (no method invocation)
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -306,6 +306,8 @@ protected void startUp() {
* Note this should only be called from the {@link Orchestrator} or {@link
org.apache.gobblin.service.monitoring.DagActionStoreChangeMonitor}
*/
public synchronized void addDag(Dag<JobExecutionPlan> dag, boolean persist,
boolean setStatus) throws IOException {
+ // TODO: Additional log added here to track missing dag issue, remove
later as needed
+ log.info("Add dag called for dag: {} to be persisted: {} and status set:
{}", dag, persist, setStatus);
Review Comment:
nit:
```
log.info("addDag(persist: {}; setStatus: {}): {}", persist, setStatus, dag)
```
(to give the short and easily parseable log msgs I most prefer working with)
--
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]
