[
https://issues.apache.org/jira/browse/GOBBLIN-1930?focusedWorklogId=885460&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-885460
]
ASF GitHub Bot logged work on GOBBLIN-1930:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 16/Oct/23 20:28
Start Date: 16/Oct/23 20:28
Worklog Time Spent: 10m
Work Description: 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)
Issue Time Tracking
-------------------
Worklog Id: (was: 885460)
Time Spent: 1h 20m (was: 1h 10m)
> Improve Logs & Metrics around Multi-active Launch Handling
> ----------------------------------------------------------
>
> Key: GOBBLIN-1930
> URL: https://issues.apache.org/jira/browse/GOBBLIN-1930
> Project: Apache Gobblin
> Issue Type: Improvement
> Components: gobblin-service
> Reporter: Urmi Mustafi
> Assignee: Abhishek Tiwari
> Priority: Major
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> Improve logging and metrics around multi-active launch flow event handling to
> identify any missing events between the {{MysqlMultiActiveLeaseArbiter}}
> committing the launch event to the {{dagActionStore}} and the
> {{DagActionMonitor}} receiving events for processing. We want to be able to
> distinguish between the following cases ofÂ
> * events that are never received by the {{DagActionMonitor}}
> * events incorrectly filtered out by the {{DagActionMonitor}}
> * any failed submissions of dags to the {{DagManager}} either upon leader
> change or from the {{DagActionChangeMonitor}}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
