errose28 commented on code in PR #3262:
URL: https://github.com/apache/ozone/pull/3262#discussion_r850913125
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/RequestValidations.java:
##########
@@ -59,20 +60,26 @@ public synchronized RequestValidations load() {
return this;
}
- public OMRequest validateRequest(OMRequest request) throws ServiceException {
+ public OMRequest validateRequest(OMRequest request)
+ throws Exception {
List<Method> validations = registry.validationsFor(
conditions(request), request.getCmdType(), PRE_PROCESS);
OMRequest validatedRequest = request.toBuilder().build();
try {
for (Method m : validations) {
- validatedRequest =
- (OMRequest) m.invoke(null, validatedRequest, context);
- LOG.debug("Running the {} request pre-process validation from {}.{}",
+ LOG.info("Running the {} request pre-process validation from {}.{}",
Review Comment:
Maybe debug would be better for this message? Seems like it could flood the
logs if there are lots of requests while the cluster is pre-finalized. I see
the post process log message below is debug.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -131,7 +132,17 @@ public OzoneManagerProtocolServerSideTranslatorPB(
@Override
public OMResponse submitRequest(RpcController controller,
OMRequest request) throws ServiceException {
- OMRequest validatedRequest = requestValidations.validateRequest(request);
+ OMRequest validatedRequest;
+ try {
+ validatedRequest = requestValidations.validateRequest(request);
+ } catch (Exception e) {
+ LOG.info("Request failed", e);
+ if (e instanceof OMException) {
+ LOG.info("Request fail... creating error response for ", e);
Review Comment:
The two log messages above look like leftovers from debugging.
##########
hadoop-ozone/dist/src/main/compose/upgrade/upgrades/non-rolling-upgrade/1.2.1-1.3.0/callback.sh:
##########
@@ -0,0 +1,84 @@
+#!/usr/bin/env bash
Review Comment:
I will reduce the code duplication between callbacks as part of HDDS-6016
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java:
##########
@@ -172,9 +191,185 @@ public ScmContainerLocationResponse
submitRequest(RpcController controller,
scm.getScmHAManager().getRatisServer().triggerNotLeaderException(),
scm.getClientRpcPort(), scm.getScmId());
}
- return dispatcher
+ // After the request interceptor (now validator) framework is extended to
+ // this server interface, this should be removed and solved via new
+ // annotated interceptors.
+ boolean checkResponseForECRepConfig = false;
+ if (request.getVersion() <
+ ClientVersion.ERASURE_CODING_SUPPORT.toProtoValue()) {
+ if (request.getCmdType() == GetContainer
+ || request.getCmdType() == ListContainer
+ || request.getCmdType() == GetContainerWithPipeline
+ || request.getCmdType() == GetContainerWithPipelineBatch
+ || request.getCmdType() == GetExistContainerWithPipelinesInBatch
+ || request.getCmdType() == ListPipelines
+ || request.getCmdType() == GetPipeline) {
+
+ checkResponseForECRepConfig = true;
+ }
+ }
+ ScmContainerLocationResponse response = dispatcher
.processRequest(request, this::processRequest, request.getCmdType(),
request.getTraceID());
+ if (checkResponseForECRepConfig) {
+ switch (response.getCmdType()) {
+ case GetContainer:
+ disallowECReplicationConfigInGetContainerResponse(response);
+ break;
+ case ListContainer:
+ disallowECReplicationConfigInListContainerResponse(response);
+ break;
+ case GetContainerWithPipeline:
+
disallowECReplicationConfigInGetContainerWithPipelineResponse(response);
+ break;
+ case GetContainerWithPipelineBatch:
+ disallowECReplicationConfigInGetContainerWithPipelineBatchResponse(
+ response);
+ break;
+ case GetExistContainerWithPipelinesInBatch:
+
disallowECReplicationConfigInGetExistContainerWithPipelineBatchResponse(
+ response);
+ break;
+ case ListPipelines:
+ disallowECReplicationConfigInListPipelinesResponse(response);
+ break;
+ case GetPipeline:
+ disallowECReplicationConfigInGetPipelineResponse(response);
+ break;
+ default:
+ }
+ }
+ return response;
+ }
+
+ private void disallowECReplicationConfigInListContainerResponse(
+ ScmContainerLocationResponse response) throws ServiceException {
+ if (!response.hasScmListContainerResponse()) {
+ return;
+ }
+ for (HddsProtos.ContainerInfoProto containerInfo :
+ response.getScmListContainerResponse().getContainersList()) {
+ if (containerInfo.hasEcReplicationConfig()) {
+ throw new ServiceException(ERROR_LIST_CONTAINS_EC_REPLICATION_CONFIG);
+ }
+ }
+ }
+
+ private void disallowECReplicationConfigInGetContainerResponse(
+ ScmContainerLocationResponse response) throws ServiceException {
+ if (!response.hasGetContainerResponse()) {
+ return;
+ }
+ if (!response.getGetContainerResponse().hasContainerInfo()) {
+ return;
+ }
+ if (response.getGetContainerResponse().getContainerInfo()
+ .hasEcReplicationConfig()) {
+ throw new
ServiceException(ERROR_RESPONSE_CONTAINS_EC_REPLICATION_CONFIG);
+ }
+ }
+
+ private void disallowECReplicationConfigInGetContainerWithPipelineResponse(
+ ScmContainerLocationResponse response) throws ServiceException {
+ if (!response.hasGetContainerWithPipelineResponse()) {
+ return;
+ }
+ if (!response.getGetContainerWithPipelineResponse()
+ .hasContainerWithPipeline()) {
+ return;
+ }
+ if (response.getGetContainerWithPipelineResponse()
+ .getContainerWithPipeline().hasContainerInfo()) {
+ HddsProtos.ContainerInfoProto containerInfo =
+ response.getGetContainerWithPipelineResponse()
+ .getContainerWithPipeline().getContainerInfo();
+ if (containerInfo.hasEcReplicationConfig()) {
+ throw new ServiceException(
Review Comment:
Discussed offline, but we should check if the admin CLI will do retries for
this (which we don't want) because the type is ServiceException.
##########
hadoop-ozone/dist/src/main/compose/upgrade/upgrades/non-rolling-upgrade/1.1.0-1.2.0/callback.sh:
##########
@@ -54,6 +54,8 @@ with_new_version_pre_finalized() {
generate new1
validate new1
+
+ check_ec_is_disabled
Review Comment:
I think we can remove these since these versions don't have EC.
##########
hadoop-ozone/dist/src/main/compose/xcompat/test.sh:
##########
@@ -73,6 +73,45 @@ test_cross_compatibility() {
KEEP_RUNNING=false stop_docker_env
}
+test_ec_cross_compatibility() {
+ echo "Running Erasure Coded storage backward compatibility tests."
+ local cluster_versions_with_ec="1.3.0"
+ local non_ec_client_versions="1.0.0 1.1.0 1.2.1"
+
+ for cluster_version in ${cluster_versions_with_ec}; do
+ export COMPOSE_FILE=new-cluster.yaml:clients.yaml
cluster_version=${cluster_version}
Review Comment:
new-cluster.yaml is running the current code in the ozone-runner image
regardless of the value of this `cluster_version` variable. If we want to try
all cluster versions that support EC, we would need one line before the loop to
test against the current version, then iterate the past versions in this loop.
See lines 118-124 farther down in this file for an example.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
