phet commented on code in PR #3733:
URL: https://github.com/apache/gobblin/pull/3733#discussion_r1286518474
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/FlowTriggerHandler.java:
##########
@@ -104,6 +104,8 @@ public FlowTriggerHandler(Config config,
Optional<MultiActiveLeaseArbiter> lease
public void handleTriggerEvent(Properties jobProps, DagActionStore.DagAction
flowAction, long eventTimeMillis)
throws IOException {
if (multiActiveLeaseArbiter.isPresent()) {
+ log.info("Multi-active scheduler about to handle trigger event: [{},
triggerEventTimestamp: {}]", flowAction,
+ eventTimeMillis);
Review Comment:
I'm all for whatever logging is needed to understand and debug. just
wondering here: couldn't essentially similar logging instead live within
`tryAcquireLease`, which is basically what this message announces is about to
be invoked?
e.g. are there too many code paths leading to `tryAcquireLease`, yet no good
way to pinpoint that it was specifically this one?
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/Orchestrator.java:
##########
@@ -244,6 +244,7 @@ public void orchestrate(Spec spec, Properties jobProps,
long triggerTimestampMil
return;
}
Map<String, String> flowMetadata =
TimingEventUtils.getFlowMetadata((FlowSpec) spec);
+ FlowCompilationValidationHelper.addFlowExecutionIdIfAbsent(flowMetadata,
jobExecutionPlanDagOptional.get());
Review Comment:
seems an important property to add.
on that note, do we have any existing `Orchestrator.orchestrate` test case
we might be able to augment with an assert that this is in fact set? (e.g.
using test inputs that would fail that assert w/o the above code change.)
--
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]
