[GitHub] [gobblin] sv2000 commented on a change in pull request #3345: [GOBBLIN-1500]Support gobblin on yarn to be able to run on clusters with robin enabled (WIP)

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

sv2000 commented on a change in pull request #3345:
URL: https://github.com/apache/gobblin/pull/3345#discussion_r681079489



##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnConfigurationKeys.java
##########
@@ -37,6 +37,8 @@
   public static final int DEFAULT_RELEASED_CONTAINERS_CACHE_EXPIRY_SECS = 300;
   public static final String APP_VIEW_ACL = GOBBLIN_YARN_PREFIX + "appViewAcl";
   public static final String DEFAULT_APP_VIEW_ACL = "*";
+  public static final String YARN_RESOURCEMANAGER_ADDRESS= "yarn.resourcemanager.address";
+  public static final String POTENTIAL_YARN_RESOURCEMANAGER_ADDRESSES= "potential.yarn.resourcemanager.addresses";

Review comment:
       OTHER_YARN_RESOURCE_MANAGER_ADDRESSES instead of POTENTIAL_

##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -574,19 +592,21 @@ private String sanitizeApplicationId(String applicationId) {
 
   @VisibleForTesting
   Optional<ApplicationId> getReconnectableApplicationId() throws YarnException, IOException {
-    List<ApplicationReport> applicationReports =
-        this.yarnClient.getApplications(APPLICATION_TYPES, RECONNECTABLE_APPLICATION_STATES);
-    if (applicationReports == null || applicationReports.isEmpty()) {
-      return Optional.absent();
-    }
+    for (YarnClient yarnClient: potentialYarnClients.values()) {
+      List<ApplicationReport> applicationReports = yarnClient.getApplications(APPLICATION_TYPES, RECONNECTABLE_APPLICATION_STATES);
+      if (applicationReports == null || applicationReports.isEmpty()) {
+        continue;
+      }
 
-    // Try to find an application with a matching application name
-    for (ApplicationReport applicationReport : applicationReports) {
-      if (this.applicationName.equals(applicationReport.getName())) {
-        String applicationId = sanitizeApplicationId(applicationReport.getApplicationId().toString());
-        LOGGER.info("Found reconnectable application with application ID: " + applicationId);
-        LOGGER.info("Application Tracking URL: " + applicationReport.getTrackingUrl());
-        return Optional.of(applicationReport.getApplicationId());
+      // Try to find an application with a matching application name
+      for (ApplicationReport applicationReport : applicationReports) {

Review comment:
       What happens if applicationReports is null? We won't enter this loop. Shouldn't we return Optional.absent() for that case?

##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java
##########
@@ -835,14 +855,22 @@ private void setupSecurityTokens(ContainerLaunchContext containerLaunchContext)
 
     TokenUtils.getAllFSTokens(new Configuration(), credentials, renewerName,
         Optional.absent(), ConfigUtils.getStringList(this.config, TokenUtils.OTHER_NAMENODES));
-
+    // Only pass token here and no secrets. (since there is no simple way to remove single token/ get secrets)
+    // For RM token, only pass the RM token for the current RM, or the RM will fail to update the token
+    Credentials finalCredentials = new Credentials();
+    for ( Token<? extends TokenIdentifier> token: credentials.getAllTokens()) {

Review comment:
       Nit: Remove unnecessary whitespaces after "for" and "(".




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