hemantk-12 commented on code in PR #4573:
URL: https://github.com/apache/ozone/pull/4573#discussion_r1187715811
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmMetrics.java:
##########
@@ -237,7 +238,7 @@ public void testBucketOps() throws Exception {
// inject exception to test for Failure Metrics on the read path
Mockito.doThrow(exception).when(mockBm).getBucketInfo(any(), any());
Mockito.doThrow(exception).when(mockBm).listBuckets(any(), any(),
- any(), anyInt());
+ any(), anyInt(), anyBoolean());
Review Comment:
```suggestion
any(), anyInt(), eq(false));
```
nit: You can use `eq(false)` to make sure correct parameter is passed.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1108,8 +1111,18 @@ public List<OmBucketInfo> listBuckets(final String
volumeName,
// We should return only the keys, whose keys match with prefix and
// the keys after the startBucket.
if (key.startsWith(seekPrefix) && key.compareTo(startKey) >= 0) {
- result.add(omBucketInfo);
- currentCount++;
+ if (!hasSnapshot) {
+ // Snapshot filter off
+ result.add(omBucketInfo);
+ currentCount++;
+ } else if (
Review Comment:
nit: use `StringUtils.isNotEmpty()` to avoid any future change in
`snapshotChainManager.getLatestPathSnapshot()` and it starts returning empty
string instead of null.
```suggestion:
} else if (StringUtils.isNotEmpty(
snapshotChainManager.getLatestPathSnapshot(volumeName +
OM_KEY_PREFIX + omBucketInfo.getBucketName())))
```
Calling it out that`snapshotChainManager.getLatestPathSnapshot()` can cause
some inconsistency (synchronization issue) either because of Snapshot deletion
or as mentioned here:
https://github.com/apache/ozone/pull/4573#discussion_r1183121609.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneVolume.java:
##########
@@ -365,7 +365,26 @@ public Iterator<? extends OzoneBucket> listBuckets(String
bucketPrefix) {
*/
public Iterator<? extends OzoneBucket> listBuckets(String bucketPrefix,
String prevBucket) {
- return new BucketIterator(bucketPrefix, prevBucket);
+ return listBuckets(bucketPrefix, prevBucket, false);
+ }
+
+ /**
+ * Returns Iterator to iterate over all buckets after prevBucket in the
+ * volum's snapshotted buckets.
Review Comment:
```suggestion
* volume's snapshotted buckets.
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -2271,6 +2278,16 @@ public void testListBucket()
volABucketCount++;
}
Assert.assertEquals(20, volABucketCount);
+
+ Iterator<? extends OzoneBucket> volABucketHasSnapshotIter =
+ volA.listBuckets("bucket-", null, true);
+ int volABucketHasSnapshotCount = 0;
+ while (volABucketHasSnapshotIter.hasNext()) {
+ volABucketHasSnapshotIter.next();
+ volABucketHasSnapshotCount++;
+ }
+ Assert.assertEquals(20, volABucketHasSnapshotCount);
+
Review Comment:
This is repetitive code. You can create helper function to fetch/count
buckets in the volume and assert that.
```
Assert.assertEquals(20, countBuckets(volA, "bucket-", null, true));
// Helper function
private int countBuckets(OzoneVolume volume,
String bucketPrefix,
String continuationToken,
boolean hasSnapshot) {
Iterator<? extends OzoneBucket> bucketIterator =
volume.listBuckets(bucketPrefix, continuationToken, hasSnapshot);
int bucketCount = 0;
while (bucketIterator.hasNext()) {
bucketIterator.next();
bucketCount++;
}
return bucketCount;
}
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -2247,19 +2247,26 @@ public void testListBucket()
OzoneVolume volA = store.getVolume(volumeA);
OzoneVolume volB = store.getVolume(volumeB);
+ String bucketName;
Review Comment:
I'll suggested to make it local variable inside the for loop.
--
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]
