[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use

[GitBox]

Will-Lo commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1025865028


##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,24 @@ void connectHelixManagerWithRetry() {
   private void addInstanceTags() {
     HelixManager receiverManager = getReceiverManager();
     if (receiverManager.isConnected()) {
-      // The helix instance associated with this container should be 
consistent on helix tag
-      List<String> existedTags = receiverManager.getClusterManagmentTool()
-          .getInstanceConfig(this.clusterName, 
this.helixInstanceName).getTags();
-      Set<String> desiredTags = new HashSet<>(
-          ConfigUtils.getStringList(this.clusterConfig, 
GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
-      if (!desiredTags.isEmpty()) {
-        // Remove tag assignments for the current Helix instance from a 
previous run
-        for (String tag : existedTags) {
-          if (!desiredTags.contains(tag))
-            
receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, 
this.helixInstanceName, tag);
-          logger.info("Removed unrelated helix tag {} for instance {}", tag, 
this.helixInstanceName);
+      try {
+        Set<String> desiredTags = new 
HashSet<>(ConfigUtils.getStringList(this.clusterConfig, 
GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+        if (!desiredTags.isEmpty()) {
+          // The helix instance associated with this container should be 
consistent on helix tag
+          List<String> existedTags =
+              
receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, 
this.helixInstanceName).getTags();
+          // Remove tag assignments for the current Helix instance from a 
previous run
+          for (String tag : existedTags) {
+            if (!desiredTags.contains(tag))
+              
receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, 
this.helixInstanceName, tag);
+            logger.info("Removed unrelated helix tag {} for instance {}", tag, 
this.helixInstanceName);
+          }
+          desiredTags.forEach(desiredTag -> 
receiverManager.getClusterManagmentTool().addInstanceTag(this.clusterName, 
this.helixInstanceName, desiredTag));
+          logger.info("Actual tags binding " + 
receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, 
this.helixInstanceName).getTags());
         }
-        desiredTags.forEach(desiredTag -> 
receiverManager.getClusterManagmentTool()
-            .addInstanceTag(this.clusterName, this.helixInstanceName, 
desiredTag));
+      } catch (HelixException e) {
+        logger.warn("some error with helix get instance config that we're 
ignoring and moving on: {}", e);

Review Comment:
   Can we reword this error to provide more context on when it's safe and 
unsafe? i.e.
   ```
   "Error with Helix getting instance config tags used in YARN cluster 
configuration. Will ignore and attempt to move on"
   ```



-- 
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-unsubscr...@gobblin.apache.org

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