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/09 18:00:19 UTC
[GitHub] [ozone] JyotinderSingh opened a new pull request, #3395: HDDS-6720. Pass OMMetadataManager to Request Validators in Upgrade Framework.
JyotinderSingh opened a new pull request, #3395:
URL: https://github.com/apache/ozone/pull/3395
## What changes were proposed in this pull request?
This Jira adds `OMMetadataManager` to the argument list for the `RequestValidators` in the upgrade framework - some of the requests/responses in case of the Bucket Layout feature cannot be rejected based on just the `OMRequest`/`OMResonse` object. We need to look inside the OM DB state to check what the bucket layout of the associated bucket would be. To enable this we need to pass the MetadataManager instance into the request validator methods.
Please see [HDDS-6681](https://issues.apache.org/jira/browse/HDDS-6681) for more details.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6720
## How was this patch tested?
Modified current tests.
--
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 #3395: HDDS-6720. Add a function to get bucket layout info in Request Validators
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on code in PR #3395:
URL: https://github.com/apache/ozone/pull/3395#discussion_r870045995
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/ValidationContext.java:
##########
@@ -34,19 +39,36 @@ public interface ValidationContext {
*/
LayoutVersionManager versionManager();
+ /**
+ * Gets the {@link BucketLayout} of the given bucket. In case of a link bucket
+ * the method returns the layout of the source bucket.
+ *
+ * @return {@link BucketLayout} of the given bucket.
+ */
+ BucketLayout getBucketLayout(String volumeName, String bucketName)
+ throws IOException;
+
/**
* Creates a context object based on the given parameters.
*
* @param versionManager the {@link LayoutVersionManager} of the service
* @return the {@link ValidationContext} specified by the parameters.
*/
- static ValidationContext of(LayoutVersionManager versionManager) {
+ static ValidationContext of(LayoutVersionManager versionManager,
+ OMMetadataManager omMetadataManager) {
return new ValidationContext() {
@Override
public LayoutVersionManager versionManager() {
return versionManager;
}
+
+ @Override
+ public BucketLayout getBucketLayout(String volumeName, String bucketName)
+ throws IOException {
+ return OzoneManagerUtils.getBucketLayout(omMetadataManager, volumeName,
+ bucketName);
Review Comment:
Thanks for the suggestion @rakeshadr, I have added this unit test to 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
[GitHub] [ozone] JyotinderSingh merged pull request #3395: HDDS-6720. Add a function to get bucket layout info in Request Validators
Posted by GitBox <gi...@apache.org>.
JyotinderSingh merged PR #3395:
URL: https://github.com/apache/ozone/pull/3395
--
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 #3395: HDDS-6720. Add a function to get bucket layout info in Request Validators
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on PR #3395:
URL: https://github.com/apache/ozone/pull/3395#issuecomment-1124648595
Thank you @errose28 @adoroszlai @rakeshadr for the reviews, I will merge the PR shortly.
--
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 #3395: HDDS-6720. Pass OMMetadataManager to Request Validators in Upgrade Framework.
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3395:
URL: https://github.com/apache/ozone/pull/3395#issuecomment-1121522398
Yes, ValidationContext is a sort of "parameter object" for the validators to get the info they need, so the info to get bucket layout should be added to that object. However, I do not think we should expose the whole metadata manager here, as it could easily be misused for anything other than a simple read request. I think it may be better to have the ValidationContext be constructed with a MetadataManager, but instead of exposing it as a getter, only expose a method like `ValidationContext#getBucketLayout(bucket)`, which uses the metadata manager internally.
--
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 #3395: HDDS-6720. Pass OMMetadataManager to Request Validators in Upgrade Framework.
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3395:
URL: https://github.com/apache/ozone/pull/3395#issuecomment-1121560396
We discussed this offline, but an important difference between bucket layout validation and layout feature validation is that once a layout feature is finalized, it can never be un-finalized. This means if we let a request through the validator based on a layout feature, we know that information is still valid in validateAndUpdateCache. This is not necessarily the case for bucket layouts. After the validator checks the bucket by its name, a bucket can be deleted and recreated with the same name and a different layout before the request is passed on to validateAndUpdateCache. There are a few ways we can handle this, some of which we discussed previously:
1. Recheck the same validator condition in validateAndUpdateCache of every relevant request.
- This could become error prone if new validators are added, as it requires an identical update in two places for correctness.
2. Check that the bucket ID we checked against in the validator matches the same bucket ID we get in validateAndUpdateCache.
- Since bucket layout is currently immutable for a given bucket, this should give us correct results.
- One way to do this could be to add a field to the `OmBucketInfo` for "validated bucket ID", and then in some part of the serialized Ratis write path add a check that "validated bucket ID" matches the actual bucket ID if it is present.
- For new requests, they would only need to modify code in the validator.
- This general approach could help with the non-FSO/FSO request splitting problem as well, where we must check the bucket layout outside the serialized path to determine whether to send an FSO or non-FSO request, but must validate that when the request is applied that is still the bucket's correct layout.
3. Refactor the validators to allow them to be triggered in a serialized part of the Ratis apply transaction path.
- This might require more work. We have numerous improvements to the validators in mind already, so maybe this can be added as a todo if it is not feasible right now.
- This will not help the non-FSO/FSO request splitting problem mentioned above.
2 seems like the best option to me, but others please add your thoughts as well.
--
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 #3395: HDDS-6720. Pass OMMetadataManager to Request Validators in Upgrade Framework.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on PR #3395:
URL: https://github.com/apache/ozone/pull/3395#issuecomment-1121454396
@adoroszlai thank you for pointing this out, that makes sense. I wasn't completely aware of `ValidationContext` object's purpose.
I will update 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
[GitHub] [ozone] JyotinderSingh commented on pull request #3395: HDDS-6720. Pass OMMetadataManager to Request Validators in Upgrade Framework.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on PR #3395:
URL: https://github.com/apache/ozone/pull/3395#issuecomment-1122270358
Thank you everyone for the reviews and suggestions, I have incorporated the feedback into the patch. As @rakeshadr suggested, we can take up the in-flight request validation as a separate task under HDDS-6682.
--
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 #3395: HDDS-6720. Pass OMMetadataManager to Request Validators in Upgrade Framework.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3395:
URL: https://github.com/apache/ozone/pull/3395#issuecomment-1121456423
> I wasn't completely aware of `ValidationContext` object's purpose. I will update the patch.
@JyotinderSingh you may want to wait until confirmation from someone else. As I mentioned, this is only my intuition here.
--
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 #3395: HDDS-6720. Pass OMMetadataManager to Request Validators in Upgrade Framework.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on PR #3395:
URL: https://github.com/apache/ozone/pull/3395#issuecomment-1121543743
> Yes, ValidationContext is a sort of "parameter object" for the validators to get the info they need, so the info to get bucket layout should be added to that object. However, I do not think we should expose the whole metadata manager here, as it could easily be misused for anything other than a simple read request. I think it may be better to have the ValidationContext be constructed with a MetadataManager, but instead of exposing it as a getter, only expose a method like `ValidationContext#getBucketLayout(bucket)`, which uses the metadata manager internally.
Thank you for the explanation and the suggestion @errose28. I have now made the change to instead only expose the `getBucketLayout` method that leverages the `OMMetadataManager` instance passed during the instantiation of the `RequestValidations` object. This should successfully prevent any misuse of the metadata manager.
--
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 #3395: HDDS-6720. Pass OMMetadataManager to Request Validators in Upgrade Framework.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on PR #3395:
URL: https://github.com/apache/ozone/pull/3395#issuecomment-1121944673
> *a bucket can be deleted and recreated with the same name and a different layout before the request is passed on to validateAndUpdateCache
@errose28 Agreed with you. +1 for the 2nd option.
After the request validators, there is `OMClientRequest#preExecute` and `OMClientRequest#validateAndUpdateCache` steps, it should have checks `validatedBucketID` with the `OMState - OmBucketInfo.objectID` and throws `BucketIDMismatchException`. There is a separate sub-task HDDS-6682 to discuss & implement this inflight request handling. IMHO, we can move this discussion to that sub-task and move the current PR to completion.
cc: @JyotinderSingh Please make a note of 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] rakeshadr commented on a diff in pull request #3395: HDDS-6720. Add a function to get bucket layout info in Request Validators
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3395:
URL: https://github.com/apache/ozone/pull/3395#discussion_r869904286
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/ValidationContext.java:
##########
@@ -34,19 +39,36 @@ public interface ValidationContext {
*/
LayoutVersionManager versionManager();
+ /**
+ * Gets the {@link BucketLayout} of the given bucket. In case of a link bucket
+ * the method returns the layout of the source bucket.
+ *
+ * @return {@link BucketLayout} of the given bucket.
+ */
+ BucketLayout getBucketLayout(String volumeName, String bucketName)
+ throws IOException;
+
/**
* Creates a context object based on the given parameters.
*
* @param versionManager the {@link LayoutVersionManager} of the service
* @return the {@link ValidationContext} specified by the parameters.
*/
- static ValidationContext of(LayoutVersionManager versionManager) {
+ static ValidationContext of(LayoutVersionManager versionManager,
+ OMMetadataManager omMetadataManager) {
return new ValidationContext() {
@Override
public LayoutVersionManager versionManager() {
return versionManager;
}
+
+ @Override
+ public BucketLayout getBucketLayout(String volumeName, String bucketName)
+ throws IOException {
+ return OzoneManagerUtils.getBucketLayout(omMetadataManager, volumeName,
+ bucketName);
Review Comment:
@JyotinderSingh Please add a unit test case to cover the getBucketLayout function in TestRequestValidations.java.
```
@Test
public void testValidationContextGetBucketLayout()
throws Exception {
ValidationContext ctx = of(anUnfinalizedVersionManager(), metadataManager);
String volName = "vol-1";
String buckName = "buck-1";
String buckKey = volName + OzoneConsts.OZONE_URI_DELIMITER + buckName;
when(metadataManager.getBucketKey(volName, buckName)).thenReturn(buckKey);
Table<String, OmBucketInfo> buckTable = mock(Table.class);
when(metadataManager.getBucketTable()).thenReturn(buckTable);
OmBucketInfo buckInfo = mock(OmBucketInfo.class);
when(buckTable.get(buckKey)).thenReturn(buckInfo);
when(buckInfo.isLink()).thenReturn(false);
when(buckInfo.getBucketLayout())
.thenReturn(BucketLayout.FILE_SYSTEM_OPTIMIZED);
BucketLayout buckLayout = ctx.getBucketLayout("vol-1", "buck-1");
Assert.assertTrue(buckLayout.isFileSystemOptimized());
}
````
--
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