[GitHub] [gobblin] phet commented on a change in pull request #3478: [GOBBLIN-1621] Make HelixRetriggeringJobCallable emit job skip event when job is dropped due to previous job is running

[GitBox <gi...@apache.org>]

phet commented on a change in pull request #3478:
URL: https://github.com/apache/gobblin/pull/3478#discussion_r826622825



##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/HelixRetriggeringJobCallable.java
##########
@@ -266,6 +277,10 @@ private void runJobExecutionLauncher() throws JobException {
 
       try {
         if (planningJobIdFromStore.isPresent() && !canRun(planningJobIdFromStore.get(), planningJobHelixManager)) {
+          TimingEvent timer = new TimingEvent(eventSubmitter, TimingEvent.JOB_SKIP_TIME);
+          HashMap<String, String> metadata = new HashMap<>(Tag.toMap(Tag.tagValuesToString(
+              Lists.newArrayList(HelixUtils.addAdditionalMetadataTags(jobProps, Lists.newArrayList())))));

Review comment:
       sorry if I'm being dense, but why can't you just use the (new) `List` returned, rather than needing to create one more copy?

##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/HelixUtils.java
##########
@@ -146,6 +153,59 @@ static void waitJobInitialization(
     log.info("Work flow {} initialized", workFlowName);
   }
 
+  /**
+   * Inject in some additional properties
+   * @param jobProps job properties
+   * @param inputTags list of metadata tags
+   * @return
+   */
+  public static List<? extends Tag<?>> addAdditionalMetadataTags(Properties jobProps,

Review comment:
       nit: seems possible to name more precisely - maybe `initializeCoreMetadataTags` or `initBaseEventTags`?

##########
File path: gobblin-metrics-libs/gobblin-metrics-base/src/main/java/org/apache/gobblin/metrics/event/TimingEvent.java
##########
@@ -101,6 +101,7 @@
   public static final String METADATA_MESSAGE = "message";
   public static final String JOB_ORCHESTRATED_TIME = "jobOrchestratedTime";
   public static final String JOB_START_TIME = "jobStartTime";
+  public static final String JOB_SKIP_TIME = "jobSkipTime";

Review comment:
       minor, but `JOB_SKIPPED_TIME` would align with `JOB_ORCHESTRATED_TIME`

##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/Task.java
##########
@@ -1048,6 +1048,7 @@ boolean hasTaskFuture() {
   public synchronized boolean cancel() {
     LOG.info("Calling task cancel with interrupt flag: {}", this.shouldInterruptTaskOnCancel);
     if (this.taskFuture != null && this.taskFuture.cancel(this.shouldInterruptTaskOnCancel)) {
+      this.taskStateTracker.onTaskCommitCompletion(this);

Review comment:
       I'm not quite following why you add this now--please explain.  at this point, unclear whether a comment would help or whether it ought to be obvious (yet I'm not graping)

##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/KafkaAvroJobStatusMonitor.java
##########
@@ -160,6 +160,7 @@ public GobblinTrackingEvent deserializeEvent(DecodeableKafkaRecord<byte[],byte[]
         break;
       case TimingEvent.FlowTimings.FLOW_CANCELLED:
       case TimingEvent.LauncherTimings.JOB_CANCEL:
+      case TimingEvent.JOB_SKIP_TIME:
         properties.put(JobStatusRetriever.EVENT_NAME_FIELD, ExecutionStatus.CANCELLED.name());

Review comment:
       I definitely agree with a distinct status... and also see the complication, so can understand the consideration to follow up.  to get more specific, how long do we expect between this change and that new status?  do you propse an immediate follow-on or something we might not prioritize for a month or more?




-- 
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: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org