From 82874e7895d6706704e8a128fb4f7728cb8dd36e Mon Sep 17 00:00:00 2001 From: Inigo Goiri Date: Thu, 21 Jun 2018 18:25:30 -0700 Subject: [PATCH] YARN-8412. Move ResourceRequest.clone logic everywhere into a proper API. Contributed by Botong Huang. --- .../yarn/api/records/ResourceRequest.java | 20 +++++++++++++++++++ .../yarn/client/api/impl/AMRMClientImpl.java | 9 +-------- .../api/impl/TestAMRMClientOnRMRestart.java | 6 +----- .../hadoop/yarn/server/AMRMClientRelayer.java | 8 +------- .../LocalityMulticastAMRMProxyPolicy.java | 10 +--------- .../server/scheduler/ResourceRequestSet.java | 14 ++----------- .../yarn/server/utils/BuilderUtils.java | 12 ----------- .../LocalitySchedulingPlacementSet.java | 10 ++-------- .../server/resourcemanager/Application.java | 10 ++++------ .../resourcemanager/TestAppManager.java | 19 +++--------------- 10 files changed, 35 insertions(+), 83 deletions(-) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/ResourceRequest.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/ResourceRequest.java index e9be6c3c14b..94eda7c4826 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/ResourceRequest.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/ResourceRequest.java @@ -101,6 +101,26 @@ public abstract class ResourceRequest implements Comparable { .executionTypeRequest(executionTypeRequest).build(); } + /** + * Clone a ResourceRequest object (shallow copy). Please keep it loaded with + * all (new) fields + * + * @param rr the object to copy from + * @return the copied object + */ + @Public + @Evolving + public static ResourceRequest clone(ResourceRequest rr) { + // Please keep it loaded with all (new) fields + return ResourceRequest.newBuilder().priority(rr.getPriority()) + .resourceName(rr.getResourceName()).capability(rr.getCapability()) + .numContainers(rr.getNumContainers()) + .relaxLocality(rr.getRelaxLocality()) + .nodeLabelExpression(rr.getNodeLabelExpression()) + .executionTypeRequest(rr.getExecutionTypeRequest()) + .allocationRequestId(rr.getAllocationRequestId()).build(); + } + @Public @Unstable public static ResourceRequestBuilder newBuilder() { diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/main/java/org/apache/hadoop/yarn/client/api/impl/AMRMClientImpl.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/main/java/org/apache/hadoop/yarn/client/api/impl/AMRMClientImpl.java index caeaa7df21e..e4561db42c5 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/main/java/org/apache/hadoop/yarn/client/api/impl/AMRMClientImpl.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/main/java/org/apache/hadoop/yarn/client/api/impl/AMRMClientImpl.java @@ -417,14 +417,7 @@ public class AMRMClientImpl extends AMRMClient { for(ResourceRequest r : ask) { // create a copy of ResourceRequest as we might change it while the // RPC layer is using it to send info across - ResourceRequest rr = ResourceRequest.newBuilder() - .priority(r.getPriority()).resourceName(r.getResourceName()) - .capability(r.getCapability()).numContainers(r.getNumContainers()) - .relaxLocality(r.getRelaxLocality()) - .nodeLabelExpression(r.getNodeLabelExpression()) - .executionTypeRequest(r.getExecutionTypeRequest()) - .allocationRequestId(r.getAllocationRequestId()).build(); - askList.add(rr); + askList.add(ResourceRequest.clone(r)); } return askList; } diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestAMRMClientOnRMRestart.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestAMRMClientOnRMRestart.java index 337d7d4af70..4add9500388 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestAMRMClientOnRMRestart.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestAMRMClientOnRMRestart.java @@ -568,11 +568,7 @@ public class TestAMRMClientOnRMRestart { ContainerUpdates updateRequests) { List askCopy = new ArrayList(); for (ResourceRequest req : ask) { - ResourceRequest reqCopy = - ResourceRequest.newInstance(req.getPriority(), - req.getResourceName(), req.getCapability(), - req.getNumContainers(), req.getRelaxLocality()); - askCopy.add(reqCopy); + askCopy.add(ResourceRequest.clone(req)); } lastAsk = ask; lastRelease = release; diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/AMRMClientRelayer.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/AMRMClientRelayer.java index 9aa6bcd28ce..2e7c184b9fa 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/AMRMClientRelayer.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/AMRMClientRelayer.java @@ -206,13 +206,7 @@ public class AMRMClientRelayer extends AbstractService for (ResourceRequest r : ask) { // create a copy of ResourceRequest as we might change it while the // RPC layer is using it to send info across - askList.add(ResourceRequest.newBuilder().priority(r.getPriority()) - .resourceName(r.getResourceName()).capability(r.getCapability()) - .numContainers(r.getNumContainers()) - .relaxLocality(r.getRelaxLocality()) - .nodeLabelExpression(r.getNodeLabelExpression()) - .executionTypeRequest(r.getExecutionTypeRequest()) - .allocationRequestId(r.getAllocationRequestId()).build()); + askList.add(ResourceRequest.clone(r)); } allocateRequest = AllocateRequest.newBuilder() diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/policies/amrmproxy/LocalityMulticastAMRMProxyPolicy.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/policies/amrmproxy/LocalityMulticastAMRMProxyPolicy.java index 06c9eded2ac..e318e3a6d93 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/policies/amrmproxy/LocalityMulticastAMRMProxyPolicy.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/policies/amrmproxy/LocalityMulticastAMRMProxyPolicy.java @@ -361,15 +361,7 @@ public class LocalityMulticastAMRMProxyPolicy extends AbstractAMRMProxyPolicy { for (SubClusterId targetId : targetSCs) { // if the calculated request is non-empty add it to the answer if (containerNums.get(i) > 0) { - ResourceRequest out = - ResourceRequest.newInstance(originalResourceRequest.getPriority(), - originalResourceRequest.getResourceName(), - originalResourceRequest.getCapability(), - originalResourceRequest.getNumContainers(), - originalResourceRequest.getRelaxLocality(), - originalResourceRequest.getNodeLabelExpression(), - originalResourceRequest.getExecutionTypeRequest()); - out.setAllocationRequestId(allocationId); + ResourceRequest out = ResourceRequest.clone(originalResourceRequest); out.setNumContainers(containerNums.get(i)); if (ResourceRequest.isAnyLocation(out.getResourceName())) { allocationBookkeeper.addAnyRR(targetId, out); diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/scheduler/ResourceRequestSet.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/scheduler/ResourceRequestSet.java index b1e6b6e211a..cf24bbf361f 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/scheduler/ResourceRequestSet.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/scheduler/ResourceRequestSet.java @@ -165,7 +165,7 @@ public class ResourceRequestSet { // the same numContainers value Map newAsks = new HashMap<>(); for (ResourceRequest rr : this.asks.values()) { - ResourceRequest clone = cloneResourceRequest(rr); + ResourceRequest clone = ResourceRequest.clone(rr); clone.setNumContainers(newValue); newAsks.put(clone.getResourceName(), clone); } @@ -176,22 +176,12 @@ public class ResourceRequestSet { throw new YarnException( "No ANY RR found in requestSet with numContainers=" + oldValue); } - ResourceRequest clone = cloneResourceRequest(rr); + ResourceRequest clone = ResourceRequest.clone(rr); clone.setNumContainers(newValue); this.asks.put(ResourceRequest.ANY, clone); } } - private ResourceRequest cloneResourceRequest(ResourceRequest rr) { - return ResourceRequest.newBuilder().priority(rr.getPriority()) - .resourceName(rr.getResourceName()).capability(rr.getCapability()) - .numContainers(rr.getNumContainers()) - .relaxLocality(rr.getRelaxLocality()) - .nodeLabelExpression(rr.getNodeLabelExpression()) - .executionTypeRequest(rr.getExecutionTypeRequest()) - .allocationRequestId(rr.getAllocationRequestId()).build(); - } - @Override public String toString() { StringBuilder builder = new StringBuilder(); diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/utils/BuilderUtils.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/utils/BuilderUtils.java index e7f47af2647..cd146a17a63 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/utils/BuilderUtils.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/utils/BuilderUtils.java @@ -371,18 +371,6 @@ public class BuilderUtils { return request; } - public static ResourceRequest newResourceRequest(ResourceRequest r) { - ResourceRequest request = recordFactory - .newRecordInstance(ResourceRequest.class); - request.setPriority(r.getPriority()); - request.setResourceName(r.getResourceName()); - request.setCapability(r.getCapability()); - request.setNumContainers(r.getNumContainers()); - request.setNodeLabelExpression(r.getNodeLabelExpression()); - request.setExecutionTypeRequest(r.getExecutionTypeRequest()); - return request; - } - public static ApplicationReport newApplicationReport( ApplicationId applicationId, ApplicationAttemptId applicationAttemptId, String user, String queue, String name, String host, int rpcPort, diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/placement/LocalitySchedulingPlacementSet.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/placement/LocalitySchedulingPlacementSet.java index 6cc8cc72031..5c3fd43d99c 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/placement/LocalitySchedulingPlacementSet.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/placement/LocalitySchedulingPlacementSet.java @@ -223,14 +223,8 @@ public class LocalitySchedulingPlacementSet } public ResourceRequest cloneResourceRequest(ResourceRequest request) { - ResourceRequest newRequest = ResourceRequest.newBuilder() - .priority(request.getPriority()) - .allocationRequestId(request.getAllocationRequestId()) - .resourceName(request.getResourceName()) - .capability(request.getCapability()) - .numContainers(1) - .relaxLocality(request.getRelaxLocality()) - .nodeLabelExpression(request.getNodeLabelExpression()).build(); + ResourceRequest newRequest = ResourceRequest.clone(request); + newRequest.setNumContainers(1); return newRequest; } diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/Application.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/Application.java index da1bc2e21a7..b962f19f85c 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/Application.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/Application.java @@ -305,9 +305,8 @@ public class Application { // Note this down for next interaction with ResourceManager ask.remove(request); - ask.add( - org.apache.hadoop.yarn.server.utils.BuilderUtils.newResourceRequest( - request)); // clone to ensure the RM doesn't manipulate the same obj + // clone to ensure the RM doesn't manipulate the same obj + ask.add(ResourceRequest.clone(request)); if (LOG.isDebugEnabled()) { LOG.debug("addResourceRequest: applicationId=" + applicationId.getId() @@ -463,9 +462,8 @@ public class Application { // Note this for next interaction with ResourceManager ask.remove(request); - ask.add( - org.apache.hadoop.yarn.server.utils.BuilderUtils.newResourceRequest( - request)); // clone to ensure the RM doesn't manipulate the same obj + // clone to ensure the RM doesn't manipulate the same obj + ask.add(ResourceRequest.clone(request)); if(LOG.isDebugEnabled()) { LOG.debug("updateResourceRequest:" + " application=" + applicationId diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestAppManager.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestAppManager.java index 8a5c7300b7e..adf6bbe7cbc 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestAppManager.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestAppManager.java @@ -563,7 +563,8 @@ public class TestAppManager{ ResourceRequest req = ResourceRequest.newInstance(Priority.newInstance(0), ResourceRequest.ANY, Resources.createResource(1025), 1, true); - asContext.setAMContainerResourceRequest(cloneResourceRequest(req)); + req.setNodeLabelExpression(RMNodeLabelsManager.NO_LABEL); + asContext.setAMContainerResourceRequest(ResourceRequest.clone(req)); // getAMContainerResourceRequests uses a singleton list of // getAMContainerResourceRequest Assert.assertEquals(req, asContext.getAMContainerResourceRequest()); @@ -920,25 +921,11 @@ public class TestAppManager{ YarnConfiguration.DEFAULT_RM_SCHEDULER_MINIMUM_ALLOCATION_MB); } - private static ResourceRequest cloneResourceRequest(ResourceRequest req) { - return ResourceRequest.newInstance( - Priority.newInstance(req.getPriority().getPriority()), - new String(req.getResourceName()), - Resource.newInstance(req.getCapability().getMemorySize(), - req.getCapability().getVirtualCores()), - req.getNumContainers(), - req.getRelaxLocality(), - req.getNodeLabelExpression() != null - ? new String(req.getNodeLabelExpression()) : null, - ExecutionTypeRequest.newInstance( - req.getExecutionTypeRequest().getExecutionType())); - } - private static List cloneResourceRequests( List reqs) { List cloneReqs = new ArrayList<>(); for (ResourceRequest req : reqs) { - cloneReqs.add(cloneResourceRequest(req)); + cloneReqs.add(ResourceRequest.clone(req)); } return cloneReqs; }