errose28 commented on a change in pull request #3262:
URL: https://github.com/apache/ozone/pull/3262#discussion_r840977428
##########
File path:
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/ClientVersion.java
##########
@@ -35,6 +35,9 @@
VERSION_HANDLES_UNKNOWN_DN_PORTS(1,
"Client version that handles the REPLICATION port in DatanodeDetails."),
+ ERASURE_CODING_SUPPORT(2, "This client version has support for Erasure"
+ + "Coding."),
Review comment:
nit. String concat is missing a space.
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java
##########
@@ -172,9 +192,184 @@ 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.getCmdType() == GetContainer
+ || request.getCmdType() == ListContainer
+ || request.getCmdType() == GetContainerWithPipeline
+ || request.getCmdType() == GetContainerWithPipelineBatch
+ || request.getCmdType() == GetExistContainerWithPipelinesInBatch
+ || request.getCmdType() == ListPipelines
+ || request.getCmdType() == GetPipeline) {
+ if (request.getVersion() <
+ ClientVersion.ERASURE_CODING_SUPPORT.toProtoValue()) {
Review comment:
Since this inner case will usually be false, it might be slightly better
to check it first.
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/ScmBlockLocationProtocolServerSideTranslatorPB.java
##########
@@ -127,6 +128,16 @@ private SCMBlockLocationResponse processMessage(
try {
switch (request.getCmdType()) {
case AllocateScmBlock:
+ if (scm.getLayoutVersionManager().needsFinalization() &&
+ scm.getLayoutVersionManager().getMetadataLayoutVersion() <
+ HDDSLayoutFeature.ERASURE_CODED_STORAGE_SUPPORT.layoutVersion()
+ ) {
+ if (request.getAllocateScmBlockRequest().hasEcReplicationConfig()) {
+ throw new ServiceException("Cluster is not finalized yet, it is"
+ + " not enabled to create blocks with Erasure Coded"
+ + " replication type.");
Review comment:
I know we don't have them yet, but is the plan to eventually handle this
with request hooks when we get those ported to the SCM as well?
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerRequestHandler.java
##########
@@ -581,6 +711,30 @@ private GetFileStatusResponse getOzoneFileStatus(
return rb.build();
}
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.POST_PROCESS,
+ requestType = Type.GetFileStatus
+ )
+ public static OMResponse disallowGetFileStatusWithECReplicationConfig(
+ OMRequest req, OMResponse resp, ValidationContext ctx)
+ throws ServiceException {
+ if (!resp.hasGetFileStatusResponse()) {
+ return resp;
+ }
+ if (resp.getGetFileStatusResponse().getStatus().getKeyInfo()
+ .hasEcReplicationConfig()) {
+ throw new ServiceException("Key is a key with"
+ + " Erasure Coded replication, which the client can not understand."
+ + " Please upgrade the client before trying to read the key info"
+ + " for" + req.getGetFileStatusRequest().getKeyArgs().getVolumeName()
Review comment:
nit. Missing space in concatenation.
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/ScmBlockLocationProtocolServerSideTranslatorPB.java
##########
@@ -127,6 +128,16 @@ private SCMBlockLocationResponse processMessage(
try {
switch (request.getCmdType()) {
case AllocateScmBlock:
+ if (scm.getLayoutVersionManager().needsFinalization() &&
+ scm.getLayoutVersionManager().getMetadataLayoutVersion() <
+ HDDSLayoutFeature.ERASURE_CODED_STORAGE_SUPPORT.layoutVersion()
Review comment:
Could this be simplified to
`scm.getLayoutVersionManager().isAllowed(HDDSLayoutFeature.ERASURE_CODED_STORAGE_SUPPORT)`?
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerRequestHandler.java
##########
@@ -597,6 +751,28 @@ private LookupFileResponse lookupFile(LookupFileRequest
request,
.build();
}
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.POST_PROCESS,
+ requestType = Type.LookupFile
+ )
+ public static OMResponse disallowLookupFileWithECReplicationConfig(
+ OMRequest req, OMResponse resp, ValidationContext ctx)
+ throws ServiceException {
+ if (!resp.hasLookupFileResponse()) {
+ return resp;
+ }
+ if (resp.getLookupFileResponse().getKeyInfo().hasEcReplicationConfig()) {
+ throw new ServiceException("Key is a key with"
+ + " Erasure Coded replication, which the client can not understand."
+ + " Please upgrade the client before trying to read the key info"
+ + " for" + req.getLookupFileRequest().getKeyArgs().getVolumeName()
Review comment:
nit. Missing space in concatenation.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java
##########
@@ -407,4 +414,24 @@ public static OMDirectoryCreateRequest getInstance(
}
return new OMDirectoryCreateRequest(omRequest, bucketLayout);
}
+
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.CLUSTER_NEEDS_FINALIZATION,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.CreateDirectory
+ )
+ public static OMRequest disallowCreateDirectoryWithECReplicationConfig(
+ OMRequest req, ValidationContext ctx) throws ServiceException {
+ if (ctx.versionManager().getMetadataLayoutVersion()
+ < OMLayoutFeature.ERASURE_CODED_STORAGE_SUPPORT.layoutVersion()) {
+ if (req.getCreateDirectoryRequest().getKeyArgs()
+ .hasEcReplicationConfig()) {
Review comment:
Not directly related to this patch, and I am missing some background on
the EC feature here, but what does it mean to create an erasure coded directory?
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerRequestHandler.java
##########
@@ -378,6 +387,30 @@ private InfoBucketResponse infoBucket(InfoBucketRequest
request)
return resp.build();
}
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
Review comment:
This will block the request for all old clients, but a few versions in
the future, clients can be old but still have EC support.
I think we could get the client version to this method by having
`RequestValidations#validateResponse` (and `RequestValidations#validateRequest`
as well) take the context as a parameter which they can forward to the
annotated methods, instead of all the annotated methods receiving the same
pre-constructed context.
`OzoneManagerProtocolServerSideTranslatorPB#submitRequest` should have enough
information to construct this context when invoking these methods.
Let me know what you think of this approach, or if you have a better idea.
--
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]
