Upload patch.

Fixed tests accordingly.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12691069/YARN-3024.01.patch
against trunk revision ae91b13.

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager:

org.apache.hadoop.yarn.server.nodemanager.containermanager.localizer.TestResourceLocalizationService

Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6290//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6290//console

This message is automatically generated.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12691073/YARN-3024.02.patch
against trunk revision ae91b13.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager.

Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6291//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6291//console

This message is automatically generated.

Updated patch.

I refactored LocalizerRunner#update(), separating the following two phase:

• first read and process resource statuses from the localizer through heartbeat
• then find the next resource to be localized and send it through response

Also, in the original code base, there is a small problem about the response action. Now if one of the following conditions is met, the response action will be DIE:

• Got at least one FETCH_FAILURE
• findNextResource() returns null, and pending is empty

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12691878/YARN-3024.03.patch
against trunk revision c4cba61.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager:

org.apache.hadoop.yarn.server.nodemanager.containermanager.logaggregation.TestLogAggregationService

Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6318//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6318//console

This message is automatically generated.

Chengbing Liu Thanks for working on this ticket. I am starting to look at the patch. Overall looks good, but

• on the latest patch, looks like you change the logic for

            case FETCH_PENDING:
              break;
  

Originally, we will directly return the response with LocalizerAction.LIVE
But now we have to do:

      LocalResource next = findNextResource();
      if (next != null) {
        try {
          ResourceLocalizationSpec resource =
              NodeManagerBuilderUtils.newResourceLocalizationSpec(next,
                getPathForLocalization(next));
          rsrcs.add(resource);
        } catch (IOException e) {
          LOG.error("local path for PRIVATE localization could not be " +
            "found. Disks might have failed.", e);
        } catch (URISyntaxException e) {
            //TODO fail? Already translated several times...
        }
      } else if (pending.isEmpty()) {
        // TODO: Synchronization
        action = LocalizerAction.DIE;
      }

      response.setLocalizerAction(action);
      response.setResourceSpecs(rsrcs);
      return response;

• Could you fix this format

  +      if (action == LocalizerAction.DIE) {
  +	response.setLocalizerAction(action);
  +	return response;
  +      }
  

Xuan Gong Thanks for reviewing.

on the latest patch, looks like you change the logic for

The logic of giving resources to be localized is actually changed.

Previously, LocalizedRunner does not give the next resource to ContainerLocalizer until the previous has been downloaded.

In this patch, LocalizedRunner will not wait for the previous resource to be downloaded. ContainerLocalizer can handle that by submitting the download task to its CompletionService, which is able to queue those tasks, before executing them. The download thread pool of the CompletionService remains a single thread executor.

Therefore, it is possible that ContainerLocalizer sends multiple LocalResourceStatus to LocalizerRunner through heartbeat. In this case, I think we should try to find the next resources to be localized even when getting FETCH_PENDING.

I have tested it on a real cluster. I specified a large archive which should take a long time to be localized. The result shows they were getting localized serially, and one heartbeat contained multiple statuses of small files (thus reducing the number of heartbeat).

Could you fix this format

My bad, I will fix this.

Xuan Gong Could you please review the changed logic described in my last comment? This patch will save at least one second for each localization process.

Yes, I will take a look shortly.

Chengbing Liu

Could you please review the changed logic described in my last comment?

The changed logic looks fine for me. Even if we submit multiple requests before the previous request is finished, those requests will be queued to process.

So, can we add the test cases to verify this changed behavior ?

Others are looks good to me.

Modified the test to verify the changed logic.

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12693845/YARN-3024.04.patch
against trunk revision 786dbdf.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager.

Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6388//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6388//console

This message is automatically generated.

+1 LGTM. Will commit it

Committed to trunk/branch-2. Thanks, Chengbing !

FAILURE: Integrated in Hadoop-trunk-Commit #6928 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6928/)
YARN-3024. LocalizerRunner should give DIE action when all resources are (xgong: rev 0d6bd62102f94c55d59f7a0a86a684e99d746127)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceLocalizationService.java
• hadoop-yarn-project/CHANGES.txt

Thanks Xuan Gong for the review!

FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #85 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/85/)
YARN-3024. LocalizerRunner should give DIE action when all resources are (xgong: rev 0d6bd62102f94c55d59f7a0a86a684e99d746127)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java
• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceLocalizationService.java

FAILURE: Integrated in Hadoop-Yarn-trunk #819 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/819/)
YARN-3024. LocalizerRunner should give DIE action when all resources are (xgong: rev 0d6bd62102f94c55d59f7a0a86a684e99d746127)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceLocalizationService.java
• hadoop-yarn-project/CHANGES.txt

FAILURE: Integrated in Hadoop-Hdfs-trunk #2017 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2017/)
YARN-3024. LocalizerRunner should give DIE action when all resources are (xgong: rev 0d6bd62102f94c55d59f7a0a86a684e99d746127)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java
• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceLocalizationService.java

SUCCESS: Integrated in Hadoop-Hdfs-trunk-Java8 #82 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/82/)
YARN-3024. LocalizerRunner should give DIE action when all resources are (xgong: rev 0d6bd62102f94c55d59f7a0a86a684e99d746127)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java
• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceLocalizationService.java

FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #86 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/86/)
YARN-3024. LocalizerRunner should give DIE action when all resources are (xgong: rev 0d6bd62102f94c55d59f7a0a86a684e99d746127)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceLocalizationService.java
• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java

FAILURE: Integrated in Hadoop-Mapreduce-trunk #2036 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2036/)
YARN-3024. LocalizerRunner should give DIE action when all resources are (xgong: rev 0d6bd62102f94c55d59f7a0a86a684e99d746127)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceLocalizationService.java
• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java

Chengbing Liu, Xuan Gong - is there a particular reason for changing .equals() calls to ==? Also, the patch seems to add some TODOs. Were there any follow-up JIRAs filed?

Karthik Kambatla, I think we should use == for enum members, for it is both null safe and it saves a function call.
The TODOs were there before this patch. Previously there were 5 TODOs. I did some refactoring to remove the duplicated code, and now there are 3. Would you like me to create JIRAs to follow the issue?

Chengbing Liu - thanks for the clarifications. Makes sense.

For the TODOs, it would be nice to have follow-up JIRAs. If it is not too much trouble, can you create them so interested contributors could follow up?

Karthik Kambatla, I created YARN-3396 to track the URISyntaxException issue.
For multiple downloads per ContainerLocalizer, I found YARN-665 already created.
As for the other TODO, i.e. synchronization, I don't see any need for this. I think we can safely remove this one.

Added this to 2.6.1 as a dependency for YARN-3464.

Patch applied cleanly for the most part, except for one non-trivial merge conflict.

Pulled this into 2.6.1. Ran compilation and TestResourceLocalizationService before the push.