xBis7 commented on code in PR #3969:
URL: https://github.com/apache/ozone/pull/3969#discussion_r1030593984
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMDirectoryCreateResponse.java:
##########
@@ -49,14 +50,16 @@ public class OMDirectoryCreateResponse extends
OmKeyResponse {
private OmKeyInfo dirKeyInfo;
private List<OmKeyInfo> parentKeyInfos;
private Result result;
+ private OmBucketInfo bucketInfo;
public OMDirectoryCreateResponse(@Nonnull OMResponse omResponse,
@Nonnull OmKeyInfo dirKeyInfo,
- @Nonnull List<OmKeyInfo> parentKeyInfos, @Nonnull Result result,
- @Nonnull BucketLayout bucketLayout) {
+ @Nonnull List<OmKeyInfo> parentKeyInfos, @Nonnull OmBucketInfo
bucketInfo,
+ @Nonnull Result result, @Nonnull BucketLayout bucketLayout) {
Review Comment:
When adding a new parameter to a method, it's best to add it at the end.
This will make debugging easier and diffs much cleaner.
```suggestion
@Nonnull BucketLayout bucketLayout, @Nonnull OmBucketInfo bucketInfo) {
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMDirectoryCreateResponseWithFSO.java:
##########
@@ -49,15 +50,19 @@ public class OMDirectoryCreateResponseWithFSO extends
OmKeyResponse {
private Result result;
private long volumeId;
private long bucketId;
+ private OmBucketInfo bucketInfo;
+ @SuppressWarnings("checkstyle:ParameterNumber")
public OMDirectoryCreateResponseWithFSO(@Nonnull OMResponse omResponse,
@Nonnull long volumeId, @Nonnull long bucketId,
@Nonnull OmDirectoryInfo dirInfo,
- @Nonnull List<OmDirectoryInfo> pDirInfos, @Nonnull Result result,
+ @Nonnull List<OmDirectoryInfo> pDirInfos,
+ @Nonnull OmBucketInfo bucketInfo, @Nonnull Result result,
@Nonnull BucketLayout bucketLayout) {
super(omResponse, bucketLayout);
this.dirInfo = dirInfo;
this.parentDirInfos = pDirInfos;
+ this.bucketInfo = bucketInfo;
Review Comment:
This should be below result.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequestWithFSO.java:
##########
@@ -172,12 +173,15 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
// total number of keys created.
numKeysCreated = missingParentInfos.size() + 1;
+ OmBucketInfo omBucketInfo =
+ getBucketInfo(omMetadataManager, volumeName, bucketName);
+ omBucketInfo.incrUsedNamespace(numKeysCreated);
result = OMDirectoryCreateRequest.Result.SUCCESS;
omClientResponse =
new OMDirectoryCreateResponseWithFSO(omResponse.build(),
- volumeId, bucketId, dirInfo, missingParentInfos, result,
- getBucketLayout());
+ volumeId, bucketId, dirInfo, missingParentInfos,
+ omBucketInfo.copyObject(), result, getBucketLayout());
Review Comment:
Change the order of the parameters.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMDirectoryCreateResponse.java:
##########
@@ -49,14 +50,16 @@ public class OMDirectoryCreateResponse extends
OmKeyResponse {
private OmKeyInfo dirKeyInfo;
private List<OmKeyInfo> parentKeyInfos;
private Result result;
+ private OmBucketInfo bucketInfo;
public OMDirectoryCreateResponse(@Nonnull OMResponse omResponse,
@Nonnull OmKeyInfo dirKeyInfo,
- @Nonnull List<OmKeyInfo> parentKeyInfos, @Nonnull Result result,
- @Nonnull BucketLayout bucketLayout) {
+ @Nonnull List<OmKeyInfo> parentKeyInfos, @Nonnull OmBucketInfo
bucketInfo,
+ @Nonnull Result result, @Nonnull BucketLayout bucketLayout) {
super(omResponse, bucketLayout);
this.dirKeyInfo = dirKeyInfo;
this.parentKeyInfos = parentKeyInfos;
+ this.bucketInfo = bucketInfo;
Review Comment:
Nit: Since we are changing the order of the parameters and adding
`bucketInfo` at the end, this should also be moved at the end.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -1172,6 +1173,58 @@ private void bucketQuotaTestHelper(int keyLength,
ReplicationConfig repConfig)
store.getVolume(volumeName).getBucket(bucketName).getUsedBytes());
}
+ @ParameterizedTest
+ @MethodSource("bucketLayouts")
+ public void testBucketUsedNamespace(BucketLayout layout) throws IOException {
+ String volumeName = UUID.randomUUID().toString();
+ String bucketName = UUID.randomUUID().toString();
+ OzoneVolume volume = null;
+ String value = "sample value";
+ int valueLength = value.getBytes(UTF_8).length;
+ store.createVolume(volumeName);
+ volume = store.getVolume(volumeName);
+ BucketArgs bucketArgs = BucketArgs.newBuilder()
+ .setBucketLayout(layout)
+ .build();
+ volume.createBucket(bucketName, bucketArgs);
+ OzoneBucket bucket = volume.getBucket(bucketName);
+ String keyName = UUID.randomUUID().toString();
+ store.getVolume(volumeName).getBucket(bucketName);
+
+ writeKey(bucket, keyName, ONE, value, valueLength);
+ Assert.assertEquals(1L,
+ store.getVolume(volumeName).getBucket(bucketName).getUsedNamespace());
+ // Test create a file twice will not increase usedNamespace twice
+ writeKey(bucket, keyName, ONE, value, valueLength);
+ Assert.assertEquals(1L,
+ store.getVolume(volumeName).getBucket(bucketName).getUsedNamespace());
+
+ bucket.deleteKey(keyName);
+ Assert.assertEquals(0L,
+ store.getVolume(volumeName).getBucket(bucketName).getUsedNamespace());
+
+ RpcClient client = new RpcClient(cluster.getConf(), null);
+ String directoryName = UUID.randomUUID().toString();
+
+ client.createDirectory(volumeName, bucketName, directoryName);
+ Assert.assertEquals(1L,
+ store.getVolume(volumeName).getBucket(bucketName).getUsedNamespace());
+ // Test create a directory twice will not increase usedNamespace twice
+ client.createDirectory(volumeName, bucketName, directoryName);
+ Assert.assertEquals(1L,
+ store.getVolume(volumeName).getBucket(bucketName).getUsedNamespace());
+
Review Comment:
It would be best to create multiple keys and directories and check in every
step that we have the expected value in `usedNamespace`. Also we should check
for the case where we are providing the directory directly with the key.
For example, if we create `dir1/key1` and `dir1/key2` without creating
`dir1` in advance, then unless we have an OBS bucket, the system will create
`dir1` and then two keys under it. In this case, `usedNamespace` should be 3.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java:
##########
@@ -215,9 +216,13 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
OMFileRequest.addKeyTableCacheEntries(omMetadataManager, volumeName,
bucketName, Optional.of(dirKeyInfo),
Optional.of(missingParentInfos), trxnLogIndex);
+ OmBucketInfo omBucketInfo =
+ getBucketInfo(omMetadataManager, volumeName, bucketName);
+ omBucketInfo.incrUsedNamespace(numMissingParents + 1L);
Review Comment:
Should we be adding the number of missing keys to the used namespace? I can
see that below call to `logResult()` is updating the metrics with the number.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -1172,6 +1173,58 @@ private void bucketQuotaTestHelper(int keyLength,
ReplicationConfig repConfig)
store.getVolume(volumeName).getBucket(bucketName).getUsedBytes());
}
+ @ParameterizedTest
+ @MethodSource("bucketLayouts")
+ public void testBucketUsedNamespace(BucketLayout layout) throws IOException {
+ String volumeName = UUID.randomUUID().toString();
+ String bucketName = UUID.randomUUID().toString();
+ OzoneVolume volume = null;
+ String value = "sample value";
+ int valueLength = value.getBytes(UTF_8).length;
+ store.createVolume(volumeName);
+ volume = store.getVolume(volumeName);
Review Comment:
```suggestion
OzoneVolume volume = store.getVolume(volumeName);
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/file/TestOMDirectoryCreateResponse.java:
##########
@@ -87,7 +98,7 @@ public void testAddToDBBatch() throws Exception {
OMDirectoryCreateResponse omDirectoryCreateResponse =
new OMDirectoryCreateResponse(omResponse, omKeyInfo,
- new ArrayList<>(), Result.SUCCESS, getBucketLayout());
+ new ArrayList<>(), omBucketInfo, Result.SUCCESS,
getBucketLayout());
Review Comment:
Order of parameters.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMDirectoryCreateResponseWithFSO.java:
##########
@@ -49,15 +50,19 @@ public class OMDirectoryCreateResponseWithFSO extends
OmKeyResponse {
private Result result;
private long volumeId;
private long bucketId;
+ private OmBucketInfo bucketInfo;
+ @SuppressWarnings("checkstyle:ParameterNumber")
public OMDirectoryCreateResponseWithFSO(@Nonnull OMResponse omResponse,
@Nonnull long volumeId, @Nonnull long bucketId,
@Nonnull OmDirectoryInfo dirInfo,
- @Nonnull List<OmDirectoryInfo> pDirInfos, @Nonnull Result result,
+ @Nonnull List<OmDirectoryInfo> pDirInfos,
+ @Nonnull OmBucketInfo bucketInfo, @Nonnull Result result,
Review Comment:
Same here, `@Nonnull OmBucketInfo bucketInfo` should be the last parameter
of the method.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java:
##########
@@ -215,9 +216,13 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
OMFileRequest.addKeyTableCacheEntries(omMetadataManager, volumeName,
bucketName, Optional.of(dirKeyInfo),
Optional.of(missingParentInfos), trxnLogIndex);
+ OmBucketInfo omBucketInfo =
+ getBucketInfo(omMetadataManager, volumeName, bucketName);
+ omBucketInfo.incrUsedNamespace(numMissingParents + 1L);
result = Result.SUCCESS;
omClientResponse = new OMDirectoryCreateResponse(omResponse.build(),
- dirKeyInfo, missingParentInfos, result, getBucketLayout());
+ dirKeyInfo, missingParentInfos, omBucketInfo.copyObject(),
+ result, getBucketLayout());
Review Comment:
Change the order of the parameters.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/file/TestOMDirectoryCreateResponseWithFSO.java:
##########
@@ -84,10 +89,16 @@ public void testAddToDBBatch() throws Exception {
.setStatus(OzoneManagerProtocolProtos.Status.OK)
.setCmdType(OzoneManagerProtocolProtos.Type.CreateDirectory)
.build();
+ ThreadLocalRandom random = ThreadLocalRandom.current();
+ long usedNamespace = Math.abs(random.nextLong(Long.MAX_VALUE));
+ OmBucketInfo omBucketInfo = TestOMResponseUtils.createBucket(
+ volumeName, bucketName);
+ omBucketInfo = omBucketInfo.toBuilder()
+ .setUsedNamespace(usedNamespace).build();
OMDirectoryCreateResponseWithFSO omDirectoryCreateResponseWithFSO =
new OMDirectoryCreateResponseWithFSO(omResponse, volumeId, bucketId,
- omDirInfo, new ArrayList<>(),
+ omDirInfo, new ArrayList<>(), omBucketInfo,
Review Comment:
Order of parameters
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -1172,6 +1173,58 @@ private void bucketQuotaTestHelper(int keyLength,
ReplicationConfig repConfig)
store.getVolume(volumeName).getBucket(bucketName).getUsedBytes());
}
+ @ParameterizedTest
+ @MethodSource("bucketLayouts")
+ public void testBucketUsedNamespace(BucketLayout layout) throws IOException {
+ String volumeName = UUID.randomUUID().toString();
+ String bucketName = UUID.randomUUID().toString();
+ OzoneVolume volume = null;
Review Comment:
Nit: This is redundant. We can declare `OzoneVolume` along with the
initialization below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
