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/09/13 06:56:09 UTC
[GitHub] [ozone] xBis7 opened a new pull request, #3750: Hdds 7217
xBis7 opened a new pull request, #3750:
URL: https://github.com/apache/ozone/pull/3750
## What changes were proposed in this pull request?
While creating a bucket without specifying the bucket layout, OM logs display only the ozone default bucket layout which is currently LEGACY. `bucket info` displays the correct bucket layout. The logs ignore the layout if it’s specified in the ozone configuration. If we specify a bucket layout while issuing the `bucket create` command or don’t declare a layout in the configuration then the correct layout is logged.
This issue was first noticed while creating an encrypted bucket but it appears even with a non encrypted one.
You can reproduce this issue in a docker based cluster like so
<br/>
while in `compose/ozone` edit `docker-config` and add
```
OZONE-SITE.XML_ozone.default.bucket.layout=FILE_SYSTEM_OPTIMIZED
```
```
$ docker-compose up –scale datanode=3 -d
$ docker exec -it ozone_om_1 bash
bash-4.2$ ozone sh volume create /vol
bash-4.2$ ozone sh bucket create /vol/buck
bash-4.2$ ozone sh bucket info /vol/buck
{
"metadata" : { },
"volumeName" : "vol",
"name" : "buck",
"storageType" : "DISK",
"versioning" : false,
"usedBytes" : 0,
"usedNamespace" : 0,
"creationTime" : "2022-09-12T17:11:15.175Z",
"modificationTime" : "2022-09-12T17:11:15.175Z",
"quotaInBytes" : -1,
"quotaInNamespace" : -1,
"bucketLayout" : "FILE_SYSTEM_OPTIMIZED",
"owner" : "hadoop",
"link" : false
}
bash-4.2$ exit
$ docker logs ozone_om_1
[OM StateMachine ApplyTransaction Thread - 0] INFO bucket.OMBucketCreateRequest: created bucket: buck of layout LEGACY in volume: vol
```
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7217
## How was this patch tested?
This patch was tested manually in a docker based cluster. Furthermore, added a new unit test in `TestOMBucketCreateRequestWithFSO`. The existing unit tests were initialized with a client version 0 and were using a layout that was either the system default or defined in the command request . The new one checks the case where we are using the latest client version and we create a bucket request while getting the bucket layout from the configuration file. We are comparing the request layout to the configuration layout and to the cache layout.
--
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] neils-dev commented on a diff in pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3750:
URL: https://github.com/apache/ozone/pull/3750#discussion_r981451596
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/bucket/TestBucketRequest.java:
##########
@@ -69,6 +69,9 @@ public void setup() throws Exception {
OzoneConfiguration ozoneConfiguration = new OzoneConfiguration();
ozoneConfiguration.set(OMConfigKeys.OZONE_OM_DB_DIRS,
folder.newFolder().getAbsolutePath());
+ ozoneConfiguration.set(OMConfigKeys.OZONE_DEFAULT_BUCKET_LAYOUT,
+ OMConfigKeys.OZONE_BUCKET_LAYOUT_FILE_SYSTEM_OPTIMIZED);
Review Comment:
Thanks @xBis7 for the PR updates and @sadanand48 for your review and comments. Re-running workflow jobs for a green build prior to closing.
--
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] kerneltime commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1263856198
It might make sense to create a separate PR for the testing code changes here. Let's keep this PR to what the jira is fixing.
--
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] kerneltime commented on a diff in pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3750:
URL: https://github.com/apache/ozone/pull/3750#discussion_r984824854
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java:
##########
@@ -258,7 +258,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
// return response.
if (exception == null) {
LOG.info("created bucket: {} of layout {} in volume: {}", bucketName,
- bucketInfo.getBucketLayout(), volumeName);
+ omBucketInfo.getBucketLayout(), volumeName);
Review Comment:
Thanks @sadanand48. This looks much cleaner.
--
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] neils-dev commented on a diff in pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3750:
URL: https://github.com/apache/ozone/pull/3750#discussion_r980045920
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/bucket/TestBucketRequest.java:
##########
@@ -69,6 +69,9 @@ public void setup() throws Exception {
OzoneConfiguration ozoneConfiguration = new OzoneConfiguration();
ozoneConfiguration.set(OMConfigKeys.OZONE_OM_DB_DIRS,
folder.newFolder().getAbsolutePath());
+ ozoneConfiguration.set(OMConfigKeys.OZONE_DEFAULT_BUCKET_LAYOUT,
+ OMConfigKeys.OZONE_BUCKET_LAYOUT_FILE_SYSTEM_OPTIMIZED);
Review Comment:
Thanks @xBis7 for the patch and for the changes. LGTM.
--
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] neils-dev commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
neils-dev commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1264021580
@errose28, a new Jira will be filed for the updated tests that were included in this PR.
> Thanks @errose28 for reviewing this PR and for your comments. Your comments of the tests for the client version
--
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] xBis7 commented on a diff in pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3750:
URL: https://github.com/apache/ozone/pull/3750#discussion_r971350019
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/ratis/TestOzoneManagerDoubleBufferWithOMResponse.java:
##########
@@ -469,7 +469,7 @@ private OMBucketCreateResponse createBucket(String volumeName,
OzoneManagerProtocolProtos.OMRequest omRequest =
OMRequestTestUtils.createBucketRequest(bucketName, volumeName, false,
- OzoneManagerProtocolProtos.StorageTypeProto.DISK);
+ OzoneManagerProtocolProtos.StorageTypeProto.DISK, 0);
Review Comment:
@neils-dev `ClientVersion` has to be part of the proto OmRequest builder. We can't change it later. If we don't set it, it initializes to default which is 0. All these tests were written for client version 0. I think it's best to not change the client version for the existing tests. Most of these tests are counting on getting a LEGACY bucket layout due to the client version(`OMBucketCreateRequest.getDefaultBucketLayout()`). If we change that, the tests will still get LEGACY layout because it's currently the default for ozone. If at some point the default layout is not LEGACY then these tests will end up broken. Instead of hardcoding 0, we can replace it with `ClientVersion.DEFAULT_VERSION.toProtoValue()`
--
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] neils-dev commented on a diff in pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3750:
URL: https://github.com/apache/ozone/pull/3750#discussion_r979335205
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/bucket/TestBucketRequest.java:
##########
@@ -69,6 +69,9 @@ public void setup() throws Exception {
OzoneConfiguration ozoneConfiguration = new OzoneConfiguration();
ozoneConfiguration.set(OMConfigKeys.OZONE_OM_DB_DIRS,
folder.newFolder().getAbsolutePath());
+ ozoneConfiguration.set(OMConfigKeys.OZONE_DEFAULT_BUCKET_LAYOUT,
+ OMConfigKeys.OZONE_BUCKET_LAYOUT_FILE_SYSTEM_OPTIMIZED);
Review Comment:
It may be useful to declare the `ozoneConfiguration` variable at the class level as a protected variable. Then allow the separate tests that extend the class to change the default layout for the specific tests. In this case for the `TestBucketRequest` the layout can be set to `OZONE_DEFAULT_BUCKET_LAYOUT_DEFAULT `for consistency with the default `OMConfigKeys`. Then in the tests it can be set when needed. For FSO specific tests (`TestOMBucketCreateRequestWithFSO.java`), we can set the protected OzoneConfiguration to FSO layout like:
`ozoneConfiguration.set(OMConfigKeys.OZONE_BUCKET_LAYOUT_FILE_SYSTEM_OPTIMIZED)`.
--
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] neils-dev commented on a diff in pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3750:
URL: https://github.com/apache/ozone/pull/3750#discussion_r971210510
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/OMRequestTestUtils.java:
##########
@@ -531,7 +531,8 @@ public static void addBucketToDB(String volumeName, String bucketName,
public static OzoneManagerProtocolProtos.OMRequest createBucketRequest(
String bucketName, String volumeName, boolean isVersionEnabled,
- OzoneManagerProtocolProtos.StorageTypeProto storageTypeProto) {
+ OzoneManagerProtocolProtos.StorageTypeProto storageTypeProto,
+ int clientVersion) {
Review Comment:
Is the `clientVersion` necessary for the tests, or can we just use the static from `ClientVersion`?
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/bucket/TestOMBucketCreateRequest.java:
##########
@@ -76,7 +76,7 @@ public void testValidateAndUpdateCacheWithNoVolume() throws Exception {
String bucketName = UUID.randomUUID().toString();
OMRequest originalRequest = OMRequestTestUtils.createBucketRequest(
- bucketName, volumeName, false, StorageTypeProto.SSD);
+ bucketName, volumeName, false, StorageTypeProto.SSD, 0);
Review Comment:
Same comment as previous on clientversion: 0 replaced with` ClientVersion.CURRENT_VERSION`.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/ratis/TestOzoneManagerDoubleBufferWithOMResponse.java:
##########
@@ -469,7 +469,7 @@ private OMBucketCreateResponse createBucket(String volumeName,
OzoneManagerProtocolProtos.OMRequest omRequest =
OMRequestTestUtils.createBucketRequest(bucketName, volumeName, false,
- OzoneManagerProtocolProtos.StorageTypeProto.DISK);
+ OzoneManagerProtocolProtos.StorageTypeProto.DISK, 0);
Review Comment:
Is it necessary to change the TestUtils `createBucketRequest `interface to include the client version? Or can we just set it within the tests when needed with the static variable `ClientVersion.CURRENT_VERSION`?
If we leave the interface changed to include the clientVersion (line 535), change the call in line 472 from
`OzoneManagerProtocolProtos.StorageTypeProto.DISK, 0`) to
`OzoneManagerProtocolProtos.StorageTypeProto.DISK, ClientVersion.CURRENT_VERSION`)
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/TestOMClientRequestWithUserInfo.java:
##########
@@ -95,7 +95,7 @@ public void testUserInfo() throws Exception {
String volumeName = UUID.randomUUID().toString();
OzoneManagerProtocolProtos.OMRequest omRequest =
OMRequestTestUtils.createBucketRequest(bucketName, volumeName, true,
- OzoneManagerProtocolProtos.StorageTypeProto.DISK);
+ OzoneManagerProtocolProtos.StorageTypeProto.DISK, 0);
Review Comment:
Change the constant 0 to use the `ClientVersion.CURRENT_VERSION` if possible.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/bucket/TestOMBucketCreateRequest.java:
##########
@@ -145,7 +145,7 @@ private OMBucketCreateRequest doPreExecute(String volumeName,
addCreateVolumeToTable(volumeName, omMetadataManager);
OMRequest originalRequest =
OMRequestTestUtils.createBucketRequest(bucketName, volumeName, false,
- StorageTypeProto.SSD);
+ StorageTypeProto.SSD, 0);
Review Comment:
clientVersion set from 0 to `ClientVersion.CURRENT_VERSION`
--
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] neils-dev commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
neils-dev commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1260101679
Thanks @kerneltime for your comment. I was about to close it. On setting the default layout in the `rpcClient.` I'm not sure there is a problem with this fix. If the client specifies the layout in the `createbucket `request then there is no issue. If the client does not specify the layout, then in the `rpcClient `that is common to all clients, the default layout from the `OmConfigKeys` is used. The `rpcClient` is common for all clients and if the client does not have a specific config for the layout the default for the OM is used. This seems to match up for what the user may expect the client behavior to be. If there is _**no specific**_ config for the client to set the default layout from, then the user expects the config _ozone.default.bucket.layout_ (which is from the `OmConfigKeys`). What do you think?
--
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] xBis7 commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
xBis7 commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1263890375
@kerneltime okay, I will take the test changes out of this PR and create a new one for them
--
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 merged pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
sadanand48 merged PR #3750:
URL: https://github.com/apache/ozone/pull/3750
--
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 #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1261301601
I looked into the code once again for this and I think if it's just the logging we're trying to fix here , we are just printing the wrong bucketInfo object , and we could fix this easily just by printing the right object.
```java
if (!bucketInfo.hasBucketLayout()) {
BucketLayout defaultBucketLayout =
getDefaultBucketLayout(ozoneManager, volumeName, bucketName);
omBucketInfo =
OmBucketInfo.getFromProtobuf(bucketInfo, defaultBucketLayout);
} else {
omBucketInfo = OmBucketInfo.getFromProtobuf(bucketInfo);
}
```
The `bucketLayout` is an optional Enum field in `BucketInfo` proto and if unset by the client, the default value is the first defined entry in the Enum. In this case , the client will not set anything and therefore `bucketInfo.bucketLayout = LEGACY` (as this is the first entry), Since client has not explicitly set this field , `hasBucketLayout `will return false and it will pick the bucket type as defined in the OM default config (inside if condition) and assign it to new object `omBucketInfo`. Now omBucketInfo will have the bucketType as set by the OM default i.e in this case FSO and it goes ahead and creates FSO bucket but the problem is that we're logging based on the initial bucketInfo object which has the LEGACY bucket type .
``` java
LOG.info("created bucket: {} of layout {} in volume: {}", bucketName,
bucketInfo.getBucketLayout(), volumeName);
```
The right fix here should be
``` java
LOG.info("created bucket: {} of layout {} in volume: {}", bucketName,
omBucketInfo.getBucketLayout(), volumeName);
```
--
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] xBis7 commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
xBis7 commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1260604342
Right now, the client only reads the bucket layout from the create command. If the layout parameter is null then OM default layout will be used(currently LEGACY). @kerneltime Let me repeat what you are saying to make sure I understand it correctly. The admin has set a custom default layout in the configuration file, let's say FSO. At some point the admin decides to change it to OBS but the client will still consider that the default layout is FSO because there is no way to update the configuration on the client side.
--
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] xBis7 commented on a diff in pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3750:
URL: https://github.com/apache/ozone/pull/3750#discussion_r971335688
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -632,6 +632,10 @@ public void createBucket(
StorageType storageType = bucketArgs.getStorageType() == null ?
StorageType.DEFAULT : bucketArgs.getStorageType();
BucketLayout bucketLayout = bucketArgs.getBucketLayout();
+ if (bucketLayout == null) {
+ bucketLayout = BucketLayout
+ .fromString(conf.get(OMConfigKeys.OZONE_DEFAULT_BUCKET_LAYOUT));
Review Comment:
@sadanand48 Thanks for reviewing this PR. It seems safe to me. The logs are reading the bucket layout from the client and the client in return reads it from the command args. I don't see another approach to 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] neils-dev commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
neils-dev commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1264017244
Thanks @xBis7 for the updated PR for the log fix. Please post in the PR the jira filed for tests removed from 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] sadanand48 commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1271180328
Thanks @xBis7 for the contribution and @neils-dev, @kerneltime, @errose28 for the reviews and discussions. Merged this to master.
--
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] kerneltime commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1259852528
The default OM bucket type is an OM enforced config. To fix the client log, this change seems incorrect, it makes sense to read the bucket info post create and log it or extend the create bucket response to include the bucket type. Reading the OM config on the client side does not seem like the right approach as there is no mechanism to check if the client config is up to date with the OM config. This might even break the expected behavior when OM has a config update and client does not specify the bucket type.
--
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] neils-dev commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
neils-dev commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1260195318
Thanks @errose28 for reviewing this PR and for your comments. Your comments of the tests for the client version
> If a version needs to be specified, the latest version should be used, not the earliest
was brought up. There are unit tests that work with the older version of the client and expect the layout from that older version "LEGACY" in order to work. So to not disturb those tests, the default client version is used.
I made a comment on perhaps filing a Jira is there is a concern of unit tests bound to the older default version of the client and layouts to investigate and resolve the issue.
see https://github.com/apache/ozone/pull/3750#discussion_r971350019
--
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] kerneltime commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1261905709
> Right now, the client only reads the bucket layout from the create command. If the layout parameter is null then OM default layout will be used(currently LEGACY). @kerneltime Let me repeat what you are saying to make sure I understand it correctly. The admin has set a custom default layout in the configuration file, let's say FSO. At some point the admin decides to change the layout from the configuration to OBS but the client will still consider that the default layout is FSO because there is no way to update the configuration on the client side.
Yes. OM config should be read only by OM. The admin might choose to not update the client config as the config is marked as an OM config.
--
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] xBis7 commented on a diff in pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3750:
URL: https://github.com/apache/ozone/pull/3750#discussion_r982131232
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/TestOMClientRequestWithUserInfo.java:
##########
@@ -95,7 +96,8 @@ public void testUserInfo() throws Exception {
String volumeName = UUID.randomUUID().toString();
OzoneManagerProtocolProtos.OMRequest omRequest =
OMRequestTestUtils.createBucketRequest(bucketName, volumeName, true,
- OzoneManagerProtocolProtos.StorageTypeProto.DISK);
+ OzoneManagerProtocolProtos.StorageTypeProto.DISK,
+ ClientVersion.DEFAULT_VERSION.toProtoValue());
Review Comment:
@errose28 All the existing tests were using `ClientVersion` 0 to execute [this block](https://github.com/xBis7/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java#L275-L284). In our case we needed to test the [else](https://github.com/xBis7/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java#L285-L298) statement that needs to use the latest `ClientVersion` in order to execute. For consistency and not making any changes to the existing tests, we used `ClientVersion.DEFAULT_VERSION`. Also see this [comment here](https://github.com/apache/ozone/pull/3750#discussion_r971350019) as @neils-dev mentioned.
--
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] xBis7 commented on a diff in pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3750:
URL: https://github.com/apache/ozone/pull/3750#discussion_r979858348
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/bucket/TestBucketRequest.java:
##########
@@ -69,6 +69,9 @@ public void setup() throws Exception {
OzoneConfiguration ozoneConfiguration = new OzoneConfiguration();
ozoneConfiguration.set(OMConfigKeys.OZONE_OM_DB_DIRS,
folder.newFolder().getAbsolutePath());
+ ozoneConfiguration.set(OMConfigKeys.OZONE_DEFAULT_BUCKET_LAYOUT,
+ OMConfigKeys.OZONE_BUCKET_LAYOUT_FILE_SYSTEM_OPTIMIZED);
Review Comment:
@neils-dev We don't need to make `ozoneConfiguration` a class variable because we are accessing it in the subclasses through `ozoneManager.getConfiguration()`. I made the changes you suggested.
--
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] kerneltime commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1260165226
The config for the default bucket type is an OM config, and it is valid for the admin to not expect it to be updated for the client. Technically it does not even need to be part of the client config.
--
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] xBis7 commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
xBis7 commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1263637473
@sadanand48 You are right, thanks the fix! I tested it using docker with an encrypted bucket and non encrypted one and it works for both. I 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
[GitHub] [ozone] xBis7 commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
xBis7 commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1264048188
This is the Jira for the unit tests. We could also make part of it to update the existing test for using the latest client version.
https://issues.apache.org/jira/browse/HDDS-7280
--
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 #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3750:
URL: https://github.com/apache/ozone/pull/3750#discussion_r981779944
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/TestOMClientRequestWithUserInfo.java:
##########
@@ -95,7 +96,8 @@ public void testUserInfo() throws Exception {
String volumeName = UUID.randomUUID().toString();
OzoneManagerProtocolProtos.OMRequest omRequest =
OMRequestTestUtils.createBucketRequest(bucketName, volumeName, true,
- OzoneManagerProtocolProtos.StorageTypeProto.DISK);
+ OzoneManagerProtocolProtos.StorageTypeProto.DISK,
+ ClientVersion.DEFAULT_VERSION.toProtoValue());
Review Comment:
If a version needs to be specified, the latest version should be used, not the earliest, unless this is doing client cross compat testing, which I don't think it is. That would be `ClientVersion.CURRENT`
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/TestOMClientRequestWithUserInfo.java:
##########
@@ -95,7 +96,8 @@ public void testUserInfo() throws Exception {
String volumeName = UUID.randomUUID().toString();
OzoneManagerProtocolProtos.OMRequest omRequest =
OMRequestTestUtils.createBucketRequest(bucketName, volumeName, true,
- OzoneManagerProtocolProtos.StorageTypeProto.DISK);
+ OzoneManagerProtocolProtos.StorageTypeProto.DISK,
+ ClientVersion.DEFAULT_VERSION.toProtoValue());
Review Comment:
Why is this change concerned with client version?
--
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] kerneltime commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1260130642
If I understand this correctly. The admin would change the config for OM to say that from now all buckets that do not explicitly state the config should be OBS related. The client still has an old config and creates the bucket as an FSO bucket. The user never wanted to override the OM default of OBS, and OM wanted a different default (OBS). Even though no one wanted an FSO bucket, an FSO bucket is created. The motivation for this change is a log printed at the end of creating a bucket. The right fix is to add the bucket type as a response to create a bucket request which is logged or for the CLI to get the bucket info and print it.
--
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] neils-dev commented on pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
neils-dev commented on PR #3750:
URL: https://github.com/apache/ozone/pull/3750#issuecomment-1260163094
> The admin would change the config for OM to say that from now all buckets that do not explicitly state the config should be OBS related. The client still has an old config and creates the bucket as an FSO bucket.
@kerneltime , thats not exactly it. In this case the admin changes the OM `ozone-site` config to a different default layout - say OBS like you set. Then the client still has a config with the FSO default. In this case, if the client does not specify the layout on create bucket requests, the default from the client will be used, FSO (from its client config). Which should correspond to what the admin intends with this client until the config changes to correspond to the OM. In this case, FSO is the default for the client until it is reconfigured. This only if in the `createBucket` request the client does not have a client specific default layout config and does not specify it for the `createBucket.`
--
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] xBis7 commented on a diff in pull request #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3750:
URL: https://github.com/apache/ozone/pull/3750#discussion_r982131232
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/TestOMClientRequestWithUserInfo.java:
##########
@@ -95,7 +96,8 @@ public void testUserInfo() throws Exception {
String volumeName = UUID.randomUUID().toString();
OzoneManagerProtocolProtos.OMRequest omRequest =
OMRequestTestUtils.createBucketRequest(bucketName, volumeName, true,
- OzoneManagerProtocolProtos.StorageTypeProto.DISK);
+ OzoneManagerProtocolProtos.StorageTypeProto.DISK,
+ ClientVersion.DEFAULT_VERSION.toProtoValue());
Review Comment:
@errose28 All the existing tests were using `ClientVersion` 0 to execute [this block](https://github.com/xBis7/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java#L275-L284). In our case we needed to test the [else](https://github.com/xBis7/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java#L285-L298) statement that needs to use the latest `ClientVersion` in order to execute. For consistency and not making any changes to the existing tests, we used `ClientVersion.DEFAULT_VERSION`. Also see this [comment](https://github.com/apache/ozone/pull/3750#discussion_r971350019) here that @neils-dev mentioned.
--
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 #3750: HDDS-7217. OM logs wrong bucket layout
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on code in PR #3750:
URL: https://github.com/apache/ozone/pull/3750#discussion_r971154801
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -632,6 +632,10 @@ public void createBucket(
StorageType storageType = bucketArgs.getStorageType() == null ?
StorageType.DEFAULT : bucketArgs.getStorageType();
BucketLayout bucketLayout = bucketArgs.getBucketLayout();
+ if (bucketLayout == null) {
+ bucketLayout = BucketLayout
+ .fromString(conf.get(OMConfigKeys.OZONE_DEFAULT_BUCKET_LAYOUT));
Review Comment:
OMConfigKeys.OZONE_DEFAULT_BUCKET_LAYOUT is basically a OM server config and we are using it in client code but I guess we should be safe as OMConfigKeys is defined in the `common` module?
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/bucket/TestOMBucketCreateRequestWithFSO.java:
##########
@@ -74,6 +112,23 @@ private OMBucketCreateRequest doPreExecute(String volumeName,
return new OMBucketCreateRequest(modifiedRequest);
}
+ private OMBucketCreateRequest
Review Comment:
nit: We could avoid some code duplication here by creating method with `doPreExecute(String volumeName,String bucketName,ClientVersion cv)`
--
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