[GitHub] [gobblin] ZihanLi58 commented on a diff in pull request #3603: [GOBBLIN-1744] Improve handling of null value edge cases when querying Helix

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

ZihanLi58 commented on code in PR #3603:
URL: https://github.com/apache/gobblin/pull/3603#discussion_r1034120156


##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/HelixUtils.java:
##########
@@ -395,11 +392,22 @@ private static void deleteStoppedHelixJob(HelixManager helixManager, String work
    * @return a map from jobNames to their Helix Workflow Ids.
    */
   public static Map<String, String> getWorkflowIdsFromJobNames(HelixManager helixManager, Collection<String> jobNames) {
-    Map<String, String> jobNameToWorkflowId = new HashMap<>();
     TaskDriver taskDriver = new TaskDriver(helixManager);
+    return getWorkflowIdsFromJobNames(taskDriver, jobNames);
+  }
+
+  public static Map<String, String> getWorkflowIdsFromJobNames(TaskDriver taskDriver, Collection<String> jobNames) {
+    Map<String, String> jobNameToWorkflowId = new HashMap<>();
     Map<String, WorkflowConfig> workflowConfigMap = taskDriver.getWorkflows();
-    for (String workflow : workflowConfigMap.keySet()) {
-      WorkflowConfig workflowConfig = taskDriver.getWorkflowConfig(workflow);
+    for (Map.Entry<String, WorkflowConfig> entry : workflowConfigMap.entrySet()) {
+      String workflow = entry.getKey();
+      WorkflowConfig workflowConfig = entry.getValue();
+      if (workflowConfig == null) {

Review Comment:
   Same here, if this is not expected, throw exception instead of just log it out



##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/HelixAssignedParticipantCheck.java:
##########
@@ -139,16 +139,24 @@ public void execute() throws CommitStepException {
 
       if (jobContext != null) {
         String participant = jobContext.getAssignedParticipant(partitionNum);
-        if (participant != null) {
-          boolean isAssignedParticipant = participant.equalsIgnoreCase(helixInstanceName);
-          if (!isAssignedParticipant) {
-            log.info("The current helix instance is not the assigned participant. helixInstanceName={}, assignedParticipant={}",
-                helixInstanceName, participant);
-          }
-
-          return isAssignedParticipant;
+        if (participant == null) {
+          log.error("The current assigned participant is null. This implies that \n"

Review Comment:
   If this is not expected, we should throw exception



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