You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/04/02 01:41:14 UTC
[GitHub] [ozone] errose28 commented on a change in pull request #3262: HDDS-5909. EC: Onboard EC into upgrade framework
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org