Copilot commented on code in PR #11217:
URL: https://github.com/apache/ozone/pull/11217#discussion_r4006746338
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3268,6 +3270,144 @@ public ListSnapshotResponse listSnapshot(
}
}
+ @Override
+ public SnapshotCountResponse snapshotCount(String bucketFilter)
+ throws IOException {
+ Map<String, String> auditMap = new LinkedHashMap<>();
+ auditMap.put("bucketFilter", StringUtils.defaultString(bucketFilter));
+
+ try {
+ BucketFilter parsedFilter = BucketFilter.parse(bucketFilter);
+ Map<String, BucketSnapshotCount> bucketCounts = new LinkedHashMap<>();
+ Map<String, Boolean> bucketAclCache = new LinkedHashMap<>();
+
+ try (TableIterator<String, ? extends Table.KeyValue<String,
SnapshotInfo>> keyIter =
+ metadataManager.getSnapshotInfoTable().iterator()) {
+ while (keyIter.hasNext()) {
+ SnapshotInfo snapshotInfo = keyIter.next().getValue();
+ String volumeName = snapshotInfo.getVolumeName();
+ String bucketName = snapshotInfo.getBucketName();
+ if (!parsedFilter.matches(volumeName, bucketName)) {
+ continue;
+ }
+
+ if (getAclsEnabled() && !hasListAccess(volumeName, bucketName,
bucketAclCache)) {
+ continue;
+ }
+
+ String bucketKey = volumeName + "/" + bucketName;
+ BucketSnapshotCount bucketCount =
+ bucketCounts.computeIfAbsent(bucketKey, unused -> new
BucketSnapshotCount(volumeName, bucketName));
+ bucketCount.increment(snapshotInfo.getSnapshotStatus());
Review Comment:
The added handler test fabricates a zero-count bucket, but this
implementation only creates a `BucketSnapshotCount` while iterating a
`SnapshotInfo` row. An existing bucket with no snapshots has no such row, so
`--bucket vol1/bucket2` returns an empty `buckets` object instead of the zero
entry asserted by `testCountBucketFilterIncludesMatchingEmptyBucket`. Either
seed the filtered existing bucket in the server response or align the test and
documented contract.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3268,6 +3270,144 @@ public ListSnapshotResponse listSnapshot(
}
}
+ @Override
+ public SnapshotCountResponse snapshotCount(String bucketFilter)
+ throws IOException {
+ Map<String, String> auditMap = new LinkedHashMap<>();
+ auditMap.put("bucketFilter", StringUtils.defaultString(bucketFilter));
+
+ try {
+ BucketFilter parsedFilter = BucketFilter.parse(bucketFilter);
+ Map<String, BucketSnapshotCount> bucketCounts = new LinkedHashMap<>();
+ Map<String, Boolean> bucketAclCache = new LinkedHashMap<>();
+
+ try (TableIterator<String, ? extends Table.KeyValue<String,
SnapshotInfo>> keyIter =
+ metadataManager.getSnapshotInfoTable().iterator()) {
+ while (keyIter.hasNext()) {
+ SnapshotInfo snapshotInfo = keyIter.next().getValue();
Review Comment:
This raw table iterator reads only persisted RocksDB rows and does not merge
the table cache. Snapshot create/delete requests place the new or
status-updated `SnapshotInfo` in that cache before the double buffer flushes
it, while `OmMetadataManagerImpl.listSnapshot` uses the cache-aware
`ListIterator.MinHeapIterator`; therefore this command can undercount new
snapshots and report deleted snapshots as active until a flush. Use the
cache-aware merge path (or explicitly merge/filter cache entries) here.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3268,6 +3270,144 @@ public ListSnapshotResponse listSnapshot(
}
}
+ @Override
+ public SnapshotCountResponse snapshotCount(String bucketFilter)
+ throws IOException {
+ Map<String, String> auditMap = new LinkedHashMap<>();
+ auditMap.put("bucketFilter", StringUtils.defaultString(bucketFilter));
+
+ try {
+ BucketFilter parsedFilter = BucketFilter.parse(bucketFilter);
+ Map<String, BucketSnapshotCount> bucketCounts = new LinkedHashMap<>();
+ Map<String, Boolean> bucketAclCache = new LinkedHashMap<>();
+
+ try (TableIterator<String, ? extends Table.KeyValue<String,
SnapshotInfo>> keyIter =
+ metadataManager.getSnapshotInfoTable().iterator()) {
+ while (keyIter.hasNext()) {
+ SnapshotInfo snapshotInfo = keyIter.next().getValue();
+ String volumeName = snapshotInfo.getVolumeName();
+ String bucketName = snapshotInfo.getBucketName();
+ if (!parsedFilter.matches(volumeName, bucketName)) {
+ continue;
+ }
+
+ if (getAclsEnabled() && !hasListAccess(volumeName, bucketName,
bucketAclCache)) {
+ continue;
+ }
+
+ String bucketKey = volumeName + "/" + bucketName;
+ BucketSnapshotCount bucketCount =
+ bucketCounts.computeIfAbsent(bucketKey, unused -> new
BucketSnapshotCount(volumeName, bucketName));
+ bucketCount.increment(snapshotInfo.getSnapshotStatus());
+ }
+ }
+
+ long totalActive = 0;
+ long totalDeleted = 0;
+ long total = 0;
+ List<SnapshotBucketCount> bucketCountList = new
ArrayList<>(bucketCounts.size());
+ for (BucketSnapshotCount bucketCount : bucketCounts.values()) {
+ bucketCountList.add(bucketCount.toResponse());
+ totalActive += bucketCount.active;
+ totalDeleted += bucketCount.deleted;
+ total += bucketCount.total;
+ }
+
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(OMAction.LIST_SNAPSHOT,
auditMap));
+ return new SnapshotCountResponse(totalActive, totalDeleted, total,
bucketCountList);
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(OMAction.LIST_SNAPSHOT,
auditMap, ex));
+ throw ex;
+ }
+ }
+
+ private boolean hasListAccess(String volumeName, String bucketName,
Map<String, Boolean> bucketAclCache)
+ throws IOException {
+ String bucketKey = volumeName + "/" + bucketName;
+ Boolean hasAccess = bucketAclCache.get(bucketKey);
+ if (hasAccess != null) {
+ return hasAccess;
+ }
+
+ try {
+ omMetadataReader.checkAcls(ResourceType.BUCKET, StoreType.OZONE,
ACLType.LIST, volumeName, bucketName, null);
+ bucketAclCache.put(bucketKey, true);
+ return true;
+ } catch (OMException ex) {
+ if (ex.getResult() == OMException.ResultCodes.ACCESS_DENIED) {
Review Comment:
`OmMetadataReader.checkAcls` throws
`OMException.ResultCodes.PERMISSION_DENIED` when access is denied, not
`ACCESS_DENIED` (see its implementation). With ACLs enabled, encountering the
first bucket the caller cannot list therefore aborts the global count request
instead of skipping that bucket. Catch the result code emitted by `checkAcls`
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: [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]
