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/05/13 08:50:33 UTC
[GitHub] [ozone] JyotinderSingh opened a new pull request, #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
JyotinderSingh opened a new pull request, #3411:
URL: https://github.com/apache/ozone/pull/3411
## What changes were proposed in this pull request?
[Work In Progress]
This Jira implements the post-finalization behavior of the Bucket Layout feature.
Post Finalization behavior for this feature from a backward compatibility perspective is defined as follows:
Older clients cannot read any data from an FSO/OBS bucket.
Older clients cannot create keys/directories in FSO/OBS buckets.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6681
## How was this patch tested?
Will be adding acceptance tests, this is currently a WIP.
--
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
[GitHub] [ozone] adoroszlai commented on pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3411:
URL: https://github.com/apache/ozone/pull/3411#issuecomment-1125815525
Thanks @JyotinderSingh for working on this. Opening PR as draft lets Github skip CI on WIP code to reduce resource usage. If you still would like CI feedback for your work, please enable the [`build-branch` workflow in your fork](https://github.com/JyotinderSingh/ozone/actions/workflows/post-commit.yml).
--
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
[GitHub] [ozone] errose28 commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r880749486
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashOzoneFileSystem.java:
##########
@@ -577,6 +580,7 @@ boolean processKeyPath(List<String> keyPathList) {
try {
omRequest = OzoneManagerProtocolProtos.OMRequest.newBuilder()
.setClientId(CLIENT_ID.toString())
+ .setVersion(ClientVersion.CURRENT_VERSION)
Review Comment:
Got it, thanks for the explanation.
--
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
[GitHub] [ozone] rakeshadr commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r882395193
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/RequestValidations.java:
##########
@@ -97,7 +97,7 @@ public OMResponse validateResponse(OMRequest request, OMResponse response)
m.getName(), m.getDeclaringClass().getPackage().getName(),
m.getDeclaringClass().getSimpleName());
validatedResponse =
- (OMResponse) m.invoke(null, request, response, context);
+ (OMResponse) m.invoke(null, request, validatedResponse, context);
Review Comment:
Awesome!
--
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
[GitHub] [ozone] errose28 commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r875125881
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMOpenKeysDeleteRequest.java:
##########
@@ -188,4 +197,46 @@ private void updateOpenKeyTableCache(OzoneManager ozoneManager,
}
}
+ /**
+ * Validates open keys delete requests.
+ * We do not want to allow older clients to delete open keys in buckets which
+ * use non LEGACY layouts.
+ *
+ * @param req - the request to validate
+ * @param ctx - the validation context
+ * @return the validated request
+ * @throws OMException if the request is invalid
+ */
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.DeleteOpenKeys
+ )
+ public static OMRequest blockDeleteOpenKeysWithBucketLayoutFromOldClient(
Review Comment:
This one is also not submitted by the client, so we probably don't need to block it.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyPurgeRequest.java:
##########
@@ -72,4 +83,47 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
return omClientResponse;
}
+ /**
+ * Validates key purge requests.
+ * We do not want to allow older clients to purge keys in buckets which use
+ * non LEGACY layouts.
+ *
+ * @param req - the request to validate
+ * @param ctx - the validation context
+ * @return the validated request
+ * @throws OMException if the request is invalid
+ */
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.PurgeKeys
+ )
+ public static OMRequest blockPurgeKeysWithBucketLayoutFromOldClient(
Review Comment:
This is an internal request submitted directly to Ratis by the OM. I don't think we need to block this.
--
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
[GitHub] [ozone] avijayanhwx commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r875026118
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java:
##########
@@ -418,4 +418,37 @@ public static OMRequest disallowCreateBucketWithBucketLayoutDuringPreFinalize(
}
return req;
}
+
+ /**
+ * Validates bucket create requests.
+ * Handles the cases where an older client attempts to create a bucket
+ * (without passing any bucket layout) and the cluster's protobuf definition
+ * defaults the missing value to a non LEGACY layout.
+ * We do not want to allow this to happen, since this would cause the client
+ * to be able to create buckets it cannot understand.
+ *
+ * @param req - the request to validate
+ * @param ctx - the validation context
+ * @return the validated request
+ * @throws OMException if the request is invalid
+ */
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.CreateBucket
+ )
+ public static OMRequest blockCreateBucketWithBucketLayoutFromOldClient(
+ OMRequest req, ValidationContext ctx) throws OMException {
+ if (req.getCreateBucketRequest().getBucketInfo().hasBucketLayout()
+ &&
+ !BucketLayout.fromProto(req.getCreateBucketRequest().getBucketInfo()
Review Comment:
How can an older client request a bucket of FSO type? Won't that be absent in the client library?
--
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
[GitHub] [ozone] JyotinderSingh commented on pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on PR #3411:
URL: https://github.com/apache/ozone/pull/3411#issuecomment-1126578676
> Thanks for working on this @JyotinderSingh. I think there are more requests we need to cover to fully block older clients from new bucket layouts. Things like get bucket, read/write/delete for files, directories, and keys, list and rename operations, etc.
Yes @errose28 I had prepared an initial patch to showcase the approach I would be following while implementing the post finalization behaviour.
I had tried to cover a few different kinds of requests at first so it would be easier to review.
--
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
[GitHub] [ozone] JyotinderSingh merged pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh merged PR #3411:
URL: https://github.com/apache/ozone/pull/3411
--
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
[GitHub] [ozone] sadanand48 commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r880607063
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashOzoneFileSystem.java:
##########
@@ -577,6 +580,7 @@ boolean processKeyPath(List<String> keyPathList) {
try {
omRequest = OzoneManagerProtocolProtos.OMRequest.newBuilder()
.setClientId(CLIENT_ID.toString())
+ .setVersion(ClientVersion.CURRENT_VERSION)
Review Comment:
> Can we refactor this class to use a common request builder method like the client side uses in OzoneManagerProtocolClientSideTranslatorPB#createOMRequest? This way we only need to specify common fields like client ID, user info, and client version in one place.
Thanks @errose28 for the suggestion. Will open a separate jira for this.
> Also while looking at this code, I do not think TrashOzoneFileSystem#submitRequest should be manually invoking preExecute. The call should go to OzoneManagerProtocolServerSideTranslatorPB#processRequest which will invoke preExecute, so it looks like the current implementation is calling preExecute twice.
It is not calling preExecute twice in the current implementation . It will call `OzoneManagerProtocolServerSideTranslatorPB#submitRequest` if `ozoneManager.isRatisEnabled()` is false. If true ,it will call preExecute and call `OzoneManagerRatisServer#submitRequest`. We did it this way to avoid creating an RPC since the filesystem is in the OM server itself. I recall there was a [test failure](https://github.com/apache/ozone/pull/1818) when we tried to call `OzoneManagerRatisServer#submitRequest`.
--
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
[GitHub] [ozone] JyotinderSingh commented on pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on PR #3411:
URL: https://github.com/apache/ozone/pull/3411#issuecomment-1125816540
> Thanks @JyotinderSingh for working on this. Opening PR as draft lets Github skip CI on WIP code to reduce resource usage. If you still would like CI feedback for your work, please enable the [`build-branch` workflow in your fork](https://github.com/JyotinderSingh/ozone/actions/workflows/post-commit.yml).
Thank you for the suggestion @adoroszlai! I will enable the workflow in my branch.
--
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
[GitHub] [ozone] rakeshadr commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r878096086
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerRequestHandler.java:
##########
@@ -796,6 +984,52 @@ public static OMResponse disallowListStatusResponseWithECReplicationConfig(
return resp;
}
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.POST_PROCESS,
+ requestType = Type.ListStatus
+ )
+ public static OMResponse disallowListStatusResponseWithBucketLayout(
+ OMRequest req, OMResponse resp, ValidationContext ctx)
+ throws ServiceException, IOException {
+ if (!resp.hasListStatusResponse()) {
+ return resp;
+ }
+
+ // Add the volume and bucket pairs to a set to avoid duplicate entries.
+ List<OzoneFileStatusProto> statuses =
+ resp.getListStatusResponse().getStatusesList();
+ HashSet<Pair<String, String>> volumeBucketSet = new HashSet<>();
+
+ for (OzoneFileStatusProto status : statuses) {
+ KeyInfo keyInfo = status.getKeyInfo();
+ if (keyInfo.hasVolumeName() && keyInfo.hasBucketName()) {
+ volumeBucketSet.add(
+ new ImmutablePair<>(keyInfo.getVolumeName(),
+ keyInfo.getBucketName()));
+ }
+ }
+
+ // If any of the keys are present in a bucket with a non LEGACY bucket
+ // layout, then the client needs to be upgraded before proceeding.
+ for (Pair<String, String> volumeBucket : volumeBucketSet) {
+ if (!ctx.getBucketLayout(volumeBucket.getLeft(),
+ volumeBucket.getRight()).isLegacy()) {
+ resp = resp.toBuilder()
+ .setStatus(Status.NOT_SUPPORTED_OPERATION)
+ .setMessage("The list of keys contains keys with Erasure Coded"
Review Comment:
Typo: It says EC. Please modify the message to reflect the bucket layout feature.
--
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
[GitHub] [ozone] JyotinderSingh commented on pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on PR #3411:
URL: https://github.com/apache/ozone/pull/3411#issuecomment-1138262153
Thank you @rakeshadr @errose28 @avijayanhwx @sadanand48 @adoroszlai for the helpful discussion and reviews.
Merging this PR.
--
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
[GitHub] [ozone] JyotinderSingh commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r873652418
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java:
##########
@@ -192,4 +199,42 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ /**
+ * Validates bucket delete requests.
+ * Handles the cases where an older client attempts to delete a bucket
+ * a new bucket layout.
+ * We do not want to allow this to happen, since this would cause the client
+ * to be able to delete buckets it cannot understand.
+ *
+ * @param req - the request to validate
+ * @param ctx - the validation context
+ * @return the validated request
+ * @throws OMException if the request is invalid
+ */
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.DeleteBucket
+ )
+ public static OMRequest blockBucketDeleteWithBucketLayoutFromOldClient(
+ OMRequest req, ValidationContext ctx) throws IOException {
+ if (!ctx.versionManager()
+ .isAllowed(OMLayoutFeature.BUCKET_LAYOUT_SUPPORT)) {
+
+ DeleteBucketRequest request = req.getDeleteBucketRequest();
+
+ if (request.hasBucketName() && request.hasVolumeName() &&
+ !ctx.getBucketLayout(
+ request.getVolumeName(), request.getBucketName())
+ .equals(BucketLayout.LEGACY)) {
+
+ throw new OMException("Cluster does not have the Bucket Layout"
Review Comment:
Done
--
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
[GitHub] [ozone] errose28 commented on pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3411:
URL: https://github.com/apache/ozone/pull/3411#issuecomment-1126550908
Are we planning on introducing the `SERVER_DEFAULT` layout and its handling for newer clients in this PR or a different one?
--
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
[GitHub] [ozone] JyotinderSingh commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r878461108
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/BucketLayout.java:
##########
@@ -88,4 +89,20 @@ public static BucketLayout fromString(String value) {
// Added safer `isBlank` check for unit test cases.
return StringUtils.isBlank(value) ? LEGACY : BucketLayout.valueOf(value);
}
+
+ /**
+ * Helper method for upgrade scenarios. Throws an exception if a bucket layout
+ * is not supported on an older client.
+ *
+ * @throws OMException if bucket layout is not supported on older clients.
+ */
+ public void validateSupportedOperation() throws OMException {
+ // Older clients do not support any bucket layout other than LEGACY.
+ if (isLegacy()) {
Review Comment:
Ah, good catch. Fixing this.
--
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
[GitHub] [ozone] sadanand48 commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r878460257
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/BucketLayout.java:
##########
@@ -88,4 +89,20 @@ public static BucketLayout fromString(String value) {
// Added safer `isBlank` check for unit test cases.
return StringUtils.isBlank(value) ? LEGACY : BucketLayout.valueOf(value);
}
+
+ /**
+ * Helper method for upgrade scenarios. Throws an exception if a bucket layout
+ * is not supported on an older client.
+ *
+ * @throws OMException if bucket layout is not supported on older clients.
+ */
+ public void validateSupportedOperation() throws OMException {
+ // Older clients do not support any bucket layout other than LEGACY.
+ if (isLegacy()) {
Review Comment:
Shouldn’t this be `!isLegacy()` ? We can also print the bucket layout in the error message
--
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
[GitHub] [ozone] rakeshadr commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r876705930
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java:
##########
@@ -192,4 +198,38 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ /**
+ * Validates bucket delete requests.
+ * Handles the cases where an older client attempts to delete a bucket
+ * a new bucket layout.
+ * We do not want to allow this to happen, since this would cause the client
+ * to be able to delete buckets it cannot understand.
+ *
+ * @param req - the request to validate
+ * @param ctx - the validation context
+ * @return the validated request
+ * @throws OMException if the request is invalid
+ */
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.DeleteBucket
+ )
+ public static OMRequest blockBucketDeleteWithBucketLayoutFromOldClient(
+ OMRequest req, ValidationContext ctx) throws IOException {
+ DeleteBucketRequest request = req.getDeleteBucketRequest();
+
+ if (request.hasBucketName() && request.hasVolumeName() &&
+ !ctx.getBucketLayout(
Review Comment:
@JyotinderSingh Its duplicated everywhere. Please move this to BucketLayout and do something like below:
```
BucketLayout.java
/**
* Add javadoc....
*
* @throws OMException
*/
void validateSupportedOperation() throws OMException {
if (!isLegacy()) {
throw new OMException("Client is attempting to delete a bucket which" +
" uses non-LEGACY bucket layout features. Please upgrade the" +
" client to a compatible version before trying to delete" +
" the bucket.",
OMException.ResultCodes.NOT_SUPPORTED_OPERATION);
}
}
```
```
BucketLayout buckLayout = ctx.getBucketLayout(
keyArgs.getVolumeName(), keyArgs.getBucketName());
buckLayout.validateSupportedOperation();
```
--
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
[GitHub] [ozone] sadanand48 commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r878460257
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/BucketLayout.java:
##########
@@ -88,4 +89,20 @@ public static BucketLayout fromString(String value) {
// Added safer `isBlank` check for unit test cases.
return StringUtils.isBlank(value) ? LEGACY : BucketLayout.valueOf(value);
}
+
+ /**
+ * Helper method for upgrade scenarios. Throws an exception if a bucket layout
+ * is not supported on an older client.
+ *
+ * @throws OMException if bucket layout is not supported on older clients.
+ */
+ public void validateSupportedOperation() throws OMException {
+ // Older clients do not support any bucket layout other than LEGACY.
+ if (isLegacy()) {
Review Comment:
Shouldn’t this be `!isLegacy()` ?
--
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
[GitHub] [ozone] sadanand48 commented on pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on PR #3411:
URL: https://github.com/apache/ozone/pull/3411#issuecomment-1133216717
@JyotinderSingh Thanks for working on this . The Problem with testTrash is because TrashOzoneFilesystem.java uses an old client version in its client requests. Please set the version to ClientVersion.CURRENT in the requests. Attaching patch [here](https://issues.apache.org/jira/secure/attachment/13043980/testTrash.patch) for a quick apply.
--
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
[GitHub] [ozone] rakeshadr commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r875654517
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMOpenKeysDeleteRequest.java:
##########
@@ -188,4 +197,46 @@ private void updateOpenKeyTableCache(OzoneManager ozoneManager,
}
}
+ /**
+ * Validates open keys delete requests.
+ * We do not want to allow older clients to delete open keys in buckets which
+ * use non LEGACY layouts.
+ *
+ * @param req - the request to validate
+ * @param ctx - the validation context
+ * @return the validated request
+ * @throws OMException if the request is invalid
+ */
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.DeleteOpenKeys
+ )
+ public static OMRequest blockDeleteOpenKeysWithBucketLayoutFromOldClient(
Review Comment:
Yes, exactly, this is an internal call and not required to block.
cc: @JyotinderSingh
--
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
[GitHub] [ozone] rakeshadr commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r875654291
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyPurgeRequest.java:
##########
@@ -72,4 +83,47 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
return omClientResponse;
}
+ /**
+ * Validates key purge requests.
+ * We do not want to allow older clients to purge keys in buckets which use
+ * non LEGACY layouts.
+ *
+ * @param req - the request to validate
+ * @param ctx - the validation context
+ * @return the validated request
+ * @throws OMException if the request is invalid
+ */
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.PurgeKeys
+ )
+ public static OMRequest blockPurgeKeysWithBucketLayoutFromOldClient(
Review Comment:
Yes, exactly, this is an internal call and not required to block.
cc: @JyotinderSingh
--
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
[GitHub] [ozone] rakeshadr commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r875675511
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java:
##########
@@ -418,4 +418,37 @@ public static OMRequest disallowCreateBucketWithBucketLayoutDuringPreFinalize(
}
return req;
}
+
+ /**
+ * Validates bucket create requests.
+ * Handles the cases where an older client attempts to create a bucket
+ * (without passing any bucket layout) and the cluster's protobuf definition
+ * defaults the missing value to a non LEGACY layout.
+ * We do not want to allow this to happen, since this would cause the client
+ * to be able to create buckets it cannot understand.
+ *
+ * @param req - the request to validate
+ * @param ctx - the validation context
+ * @return the validated request
+ * @throws OMException if the request is invalid
+ */
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.CreateBucket
+ )
+ public static OMRequest blockCreateBucketWithBucketLayoutFromOldClient(
+ OMRequest req, ValidationContext ctx) throws OMException {
+ if (req.getCreateBucketRequest().getBucketInfo().hasBucketLayout()
+ &&
+ !BucketLayout.fromProto(req.getCreateBucketRequest().getBucketInfo()
Review Comment:
Yes, older clients won't have the option to set FSO or OBS type explicitly. I too agree to exclude the OLDER_CLIENT_REQUESTS validate condition here.
@JyotinderSingh
--
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
[GitHub] [ozone] errose28 commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r879713369
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashOzoneFileSystem.java:
##########
@@ -577,6 +580,7 @@ boolean processKeyPath(List<String> keyPathList) {
try {
omRequest = OzoneManagerProtocolProtos.OMRequest.newBuilder()
.setClientId(CLIENT_ID.toString())
+ .setVersion(ClientVersion.CURRENT_VERSION)
Review Comment:
Also while looking at this code, I do not think `TrashOzoneFileSystem#submitRequest` should be manually invoking `preExecute`. The call should go to `OzoneManagerProtocolServerSideTranslatorPB#processRequest` which will invoke `preExecute`, so it looks like the current implementation is calling `preExecute` twice.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashOzoneFileSystem.java:
##########
@@ -577,6 +580,7 @@ boolean processKeyPath(List<String> keyPathList) {
try {
omRequest = OzoneManagerProtocolProtos.OMRequest.newBuilder()
.setClientId(CLIENT_ID.toString())
+ .setVersion(ClientVersion.CURRENT_VERSION)
Review Comment:
Can we refactor this class to use a common request builder method like the client side uses in `OzoneManagerProtocolClientSideTranslatorPB#createOMRequest`? This way we only need to specify common fields like client ID, user info, and client version in one place.
--
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
[GitHub] [ozone] errose28 commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r872808179
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java:
##########
@@ -192,4 +199,42 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ /**
+ * Validates bucket delete requests.
+ * Handles the cases where an older client attempts to delete a bucket
+ * a new bucket layout.
+ * We do not want to allow this to happen, since this would cause the client
+ * to be able to delete buckets it cannot understand.
+ *
+ * @param req - the request to validate
+ * @param ctx - the validation context
+ * @return the validated request
+ * @throws OMException if the request is invalid
+ */
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.DeleteBucket
+ )
+ public static OMRequest blockBucketDeleteWithBucketLayoutFromOldClient(
+ OMRequest req, ValidationContext ctx) throws IOException {
+ if (!ctx.versionManager()
+ .isAllowed(OMLayoutFeature.BUCKET_LAYOUT_SUPPORT)) {
+
+ DeleteBucketRequest request = req.getDeleteBucketRequest();
+
+ if (request.hasBucketName() && request.hasVolumeName() &&
+ !ctx.getBucketLayout(
+ request.getVolumeName(), request.getBucketName())
+ .equals(BucketLayout.LEGACY)) {
+
+ throw new OMException("Cluster does not have the Bucket Layout"
Review Comment:
This message needs to be updated to pertain to old clients.
--
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
[GitHub] [ozone] rakeshadr commented on pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on PR #3411:
URL: https://github.com/apache/ozone/pull/3411#issuecomment-1127230285
> Are we planning on introducing the `SERVER_DEFAULT` layout and its handling for newer clients in this PR or a different one?
Thanks @errose28 for pointing out this. @JyotinderSingh has created a separate sub-task [HDDS-6683](https://issues.apache.org/jira/browse/HDDS-6683) to discuss and implement this behavior. Thanks @JyotinderSingh for splitting the task and IMHO that will help for more concrete discussions.
--
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
[GitHub] [ozone] JyotinderSingh commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r881580983
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/RequestValidations.java:
##########
@@ -97,7 +97,7 @@ public OMResponse validateResponse(OMRequest request, OMResponse response)
m.getName(), m.getDeclaringClass().getPackage().getName(),
m.getDeclaringClass().getSimpleName());
validatedResponse =
- (OMResponse) m.invoke(null, request, response, context);
+ (OMResponse) m.invoke(null, request, validatedResponse, context);
Review Comment:
We want to use the `validatedResponse` object across all the RequestValidators - otherwise, the validated responses from these validators would all end up being ignored - and only the validated response from whatever the last validator that was run would be used.
--
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
[GitHub] [ozone] JyotinderSingh commented on a diff in pull request #3411: HDDS-6681. Post-Finalize behaviour for Bucket Layout feature.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on code in PR #3411:
URL: https://github.com/apache/ozone/pull/3411#discussion_r877740729
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java:
##########
@@ -192,4 +198,38 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ /**
+ * Validates bucket delete requests.
+ * Handles the cases where an older client attempts to delete a bucket
+ * a new bucket layout.
+ * We do not want to allow this to happen, since this would cause the client
+ * to be able to delete buckets it cannot understand.
+ *
+ * @param req - the request to validate
+ * @param ctx - the validation context
+ * @return the validated request
+ * @throws OMException if the request is invalid
+ */
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.DeleteBucket
+ )
+ public static OMRequest blockBucketDeleteWithBucketLayoutFromOldClient(
+ OMRequest req, ValidationContext ctx) throws IOException {
+ DeleteBucketRequest request = req.getDeleteBucketRequest();
+
+ if (request.hasBucketName() && request.hasVolumeName() &&
+ !ctx.getBucketLayout(
Review Comment:
Thank you for the suggestion, I have update the patch.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java:
##########
@@ -192,4 +198,38 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ /**
+ * Validates bucket delete requests.
+ * Handles the cases where an older client attempts to delete a bucket
+ * a new bucket layout.
+ * We do not want to allow this to happen, since this would cause the client
+ * to be able to delete buckets it cannot understand.
+ *
+ * @param req - the request to validate
+ * @param ctx - the validation context
+ * @return the validated request
+ * @throws OMException if the request is invalid
+ */
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.OLDER_CLIENT_REQUESTS,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.DeleteBucket
+ )
+ public static OMRequest blockBucketDeleteWithBucketLayoutFromOldClient(
+ OMRequest req, ValidationContext ctx) throws IOException {
+ DeleteBucketRequest request = req.getDeleteBucketRequest();
+
+ if (request.hasBucketName() && request.hasVolumeName() &&
+ !ctx.getBucketLayout(
Review Comment:
Thank you for the suggestion, I have updated the patch.
--
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