[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #3027: [GOBBLIN-1187] cancel flow execution when dag manager is disabled

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

autumnust commented on a change in pull request #3027:
URL: https://github.com/apache/incubator-gobblin/pull/3027#discussion_r439150290



##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/job_monitor/AvroJobSpecKafkaJobMonitor.java
##########
@@ -108,10 +102,10 @@ protected void createMetrics() {
   /**
    * Creates a {@link JobSpec} or {@link URI} from the {@link AvroJobSpec} record.
    * @param record the record as an {@link AvroJobSpec}
-   * @return a {@link JobSpec} or {@link URI} wrapped in a {@link Collection} of {@link Either}
+   * @return a {@link JobSpec}
    */
   @Override
-  public Collection<Either<JobSpec, URI>> parseJobSpec(AvroJobSpec record) {
+  public Collection<JobSpec> parseJobSpec(AvroJobSpec record) {

Review comment:
       A failure in maintaining backward compatibility ?

##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/job_catalog/NonObservingFSJobCatalog.java
##########
@@ -121,5 +125,11 @@ public synchronized void remove(URI jobURI) {
       LOGGER.warn("No file with URI:" + jobURI + " is found. Deletion failed.");
     }
   }
+
+  @Override
+  public synchronized void cancel(JobSpec jobSpec) {
+    Preconditions.checkState(state() == State.RUNNING, String.format("%s is not running.", this.getClass().getName()));

Review comment:
       I am confused by this: Check the state equals to running but log "xxx is not running" ? 




----------------------------------------------------------------
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.

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