[
https://issues.apache.org/jira/browse/YARN-11236?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17576068#comment-17576068
]
ASF GitHub Bot commented on YARN-11236:
---------------------------------------
goiri commented on code in PR #4711:
URL: https://github.com/apache/hadoop/pull/4711#discussion_r939396343
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/MemoryFederationStateStore.java:
##########
@@ -312,4 +324,41 @@ public Version loadVersion() {
return null;
}
+ @Override
+ public AddReservationHomeSubClusterResponse addReservationHomeSubCluster(
+ AddReservationHomeSubClusterRequest request) throws YarnException {
+ FederationReservationHomeSubClusterStoreInputValidator
+ .validateAddReservationHomeSubClusterRequest(request);
Review Comment:
These names are pretty verose.
If we are in FederationReservationHomeSubClusterStoreInputValidator isn't
enough to say validateRequest? or validate as the type is in the arg?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/utils/FederationReservationHomeSubClusterStoreInputValidator.java:
##########
@@ -0,0 +1,134 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.yarn.server.federation.store.utils;
+
+import org.apache.hadoop.yarn.api.records.ReservationId;
+import
org.apache.hadoop.yarn.server.federation.store.exception.FederationStateStoreInvalidInputException;
+import
org.apache.hadoop.yarn.server.federation.store.records.AddReservationHomeSubClusterRequest;
+import
org.apache.hadoop.yarn.server.federation.store.records.ReservationHomeSubCluster;
+import
org.apache.hadoop.yarn.server.federation.store.records.GetReservationHomeSubClusterRequest;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Utility class to validate the inputs to
+ * {@code FederationReservationHomeSubClusterStore}, allows a fail fast
+ * mechanism for invalid user inputs.
+ *
+ */
+public final class FederationReservationHomeSubClusterStoreInputValidator {
+
+ private static final Logger LOG = LoggerFactory
+ .getLogger(FederationReservationHomeSubClusterStoreInputValidator.class);
+
+ private FederationReservationHomeSubClusterStoreInputValidator() {
+ }
+
+ /**
+ * Quick validation on the input to check some obvious fail conditions (fail
+ * fast). Check if the provided {@link AddReservationHomeSubClusterRequest}
+ * for adding a new reservation is valid or not.
+ *
+ * @param request the {@link AddReservationHomeSubClusterRequest} to validate
+ * against
+ * @throws FederationStateStoreInvalidInputException if the request is
invalid
+ */
+ public static void validateAddReservationHomeSubClusterRequest(
+ AddReservationHomeSubClusterRequest request)
+ throws FederationStateStoreInvalidInputException {
+ if (request == null) {
+ String message = "Missing AddReservationHomeSubCluster Request."
+ + " Please try again by specifying"
+ + " an AddReservationHomeSubCluster information.";
+ LOG.warn(message);
+ throw new FederationStateStoreInvalidInputException(message);
+ }
+
+ // validate ReservationHomeSubCluster info
+ checkReservationHomeSubCluster(request.getReservationHomeSubCluster());
+ }
+
+ /**
+ * Quick validation on the input to check some obvious fail conditions (fail
+ * fast). Check if the provided {@link GetReservationHomeSubClusterRequest}
+ * for querying reservation's information is valid or not.
+ *
+ * @param request the {@link GetReservationHomeSubClusterRequest} to validate
+ * against
+ * @throws FederationStateStoreInvalidInputException if the request is
invalid
+ */
+ public static void validateGetReservationHomeSubClusterRequest(
Review Comment:
validate() should be enough as the type is there.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/federation/FederationStateStoreService.java:
##########
@@ -115,10 +121,11 @@ protected void serviceInit(Configuration conf) throws
Exception {
heartbeatInterval = conf.getLong(
YarnConfiguration.FEDERATION_STATESTORE_HEARTBEAT_INTERVAL_SECS,
-
YarnConfiguration.DEFAULT_FEDERATION_STATESTORE_HEARTBEAT_INTERVAL_SECS);
+ YarnConfiguration
+ .DEFAULT_FEDERATION_STATESTORE_HEARTBEAT_INTERVAL_SECS);
if (heartbeatInterval <= 0) {
- heartbeatInterval =
-
YarnConfiguration.DEFAULT_FEDERATION_STATESTORE_HEARTBEAT_INTERVAL_SECS;
Review Comment:
Can we leave the these changes as they were?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/utils/FederationReservationHomeSubClusterStoreInputValidator.java:
##########
@@ -0,0 +1,134 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.yarn.server.federation.store.utils;
+
+import org.apache.hadoop.yarn.api.records.ReservationId;
+import
org.apache.hadoop.yarn.server.federation.store.exception.FederationStateStoreInvalidInputException;
+import
org.apache.hadoop.yarn.server.federation.store.records.AddReservationHomeSubClusterRequest;
+import
org.apache.hadoop.yarn.server.federation.store.records.ReservationHomeSubCluster;
+import
org.apache.hadoop.yarn.server.federation.store.records.GetReservationHomeSubClusterRequest;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Utility class to validate the inputs to
+ * {@code FederationReservationHomeSubClusterStore}, allows a fail fast
+ * mechanism for invalid user inputs.
+ *
+ */
+public final class FederationReservationHomeSubClusterStoreInputValidator {
+
+ private static final Logger LOG = LoggerFactory
+ .getLogger(FederationReservationHomeSubClusterStoreInputValidator.class);
+
+ private FederationReservationHomeSubClusterStoreInputValidator() {
+ }
+
+ /**
+ * Quick validation on the input to check some obvious fail conditions (fail
+ * fast). Check if the provided {@link AddReservationHomeSubClusterRequest}
+ * for adding a new reservation is valid or not.
+ *
+ * @param request the {@link AddReservationHomeSubClusterRequest} to validate
+ * against
+ * @throws FederationStateStoreInvalidInputException if the request is
invalid
+ */
+ public static void validateAddReservationHomeSubClusterRequest(
+ AddReservationHomeSubClusterRequest request)
+ throws FederationStateStoreInvalidInputException {
+ if (request == null) {
+ String message = "Missing AddReservationHomeSubCluster Request."
+ + " Please try again by specifying"
+ + " an AddReservationHomeSubCluster information.";
+ LOG.warn(message);
+ throw new FederationStateStoreInvalidInputException(message);
+ }
+
+ // validate ReservationHomeSubCluster info
+ checkReservationHomeSubCluster(request.getReservationHomeSubCluster());
+ }
+
+ /**
+ * Quick validation on the input to check some obvious fail conditions (fail
+ * fast). Check if the provided {@link GetReservationHomeSubClusterRequest}
+ * for querying reservation's information is valid or not.
+ *
+ * @param request the {@link GetReservationHomeSubClusterRequest} to validate
+ * against
+ * @throws FederationStateStoreInvalidInputException if the request is
invalid
+ */
+ public static void validateGetReservationHomeSubClusterRequest(
+ GetReservationHomeSubClusterRequest request)
+ throws FederationStateStoreInvalidInputException {
+ if (request == null) {
+ String message = "Missing GetReservationHomeSubCluster Request."
+ + " Please try again by specifying an Reservation Id information.";
+ LOG.warn(message);
+ throw new FederationStateStoreInvalidInputException(message);
+ }
+
+ // validate Reservation Id
+ checkReservationId(request.getReservationId());
+ }
+
+ /**
+ * Validate if the ReservationHomeSubCluster info are present or not.
+ *
+ * @param reservationHomeSubCluster the information of the Reservation to be
+ * verified
+ * @throws FederationStateStoreInvalidInputException if the SubCluster Info
+ * are invalid
+ */
+ private static void checkReservationHomeSubCluster(
+ ReservationHomeSubCluster reservationHomeSubCluster)
+
+ throws FederationStateStoreInvalidInputException {
+ if (reservationHomeSubCluster == null) {
+ String message = "Missing ReservationHomeSubCluster Info."
+ + " Please try again by specifying"
+ + " an ReservationHomeSubCluster information.";
+ LOG.warn(message);
+ throw new FederationStateStoreInvalidInputException(message);
+ }
+ // validate Reservation Id
+ checkReservationId(reservationHomeSubCluster.getReservationId());
+
+ // validate subcluster Id
+ FederationMembershipStateStoreInputValidator
+ .checkSubClusterId(reservationHomeSubCluster.getHomeSubCluster());
+
+ }
+
+ /**
+ * Validate if the Reservation id is present or not.
+ *
+ * @param reservationId the id of the Reservation to be verified
+ * @throws FederationStateStoreInvalidInputException if the Reservation Id is
+ * invalid
+ */
+ private static void checkReservationId(ReservationId reservationId)
+ throws FederationStateStoreInvalidInputException {
+ if (reservationId == null) {
+ String message = "Missing ReservationId."
Review Comment:
Single line?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/policies/router/AbstractRouterPolicy.java:
##########
@@ -98,7 +99,16 @@ protected Map<SubClusterId, SubClusterInfo>
prefilterSubClusters(
// if a reservation exists limit scope to the sub-cluster this
// reservation is mapped to
- // TODO: Implemented in YARN-11236
+ if (reservationId != null) {
+ // note this might throw YarnException if the reservation is
+ // unknown. This is to be expected, and should be handled by
+ // policy invoker.
+ SubClusterId resSubCluster =
getPolicyContext().getFederationStateStoreFacade().
+ getReservationHomeSubCluster(reservationId);
+
+ return Collections.singletonMap(resSubCluster,
activeSubClusters.get(resSubCluster));
Review Comment:
Extract the cluster
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/policies/router/AbstractRouterPolicy.java:
##########
@@ -98,7 +99,16 @@ protected Map<SubClusterId, SubClusterInfo>
prefilterSubClusters(
// if a reservation exists limit scope to the sub-cluster this
// reservation is mapped to
- // TODO: Implemented in YARN-11236
+ if (reservationId != null) {
+ // note this might throw YarnException if the reservation is
+ // unknown. This is to be expected, and should be handled by
+ // policy invoker.
+ SubClusterId resSubCluster =
getPolicyContext().getFederationStateStoreFacade().
Review Comment:
Extract the facade.
> Implement FederationReservationHomeSubClusterStore With MemoryStore
> -------------------------------------------------------------------
>
> Key: YARN-11236
> URL: https://issues.apache.org/jira/browse/YARN-11236
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: federation
> Affects Versions: 3.4.0
> Reporter: fanshilun
> Assignee: fanshilun
> Priority: Major
> Labels: pull-request-available
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
