Copilot commented on code in PR #11118:
URL: https://github.com/apache/ozone/pull/11118#discussion_r4056118531
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -536,38 +537,49 @@ private OzoneManagerProtocolProtos.PurgePathRequest
wrapPurgeRequest(
private List<OzoneManagerProtocolProtos.OMResponse>
submitPurgePathsWithBatching(List<PurgePathRequest> requests,
String snapTableKey, UUID expectedPreviousSnapshotId,
Map<VolumeBucketId, BucketNameInfo> bucketNameInfoMap) {
+ // Group purge paths by their owning bucket so that every submitted purge
transaction contains paths from a single
+ // bucket only. This keeps the apply side acquiring exactly one bucket
write lock per transaction; combined with
+ // apply-side chunking within that lock, a large background directory
purge cannot starve readers on other buckets.
+ Map<VolumeBucketId, List<PurgePathRequest>> requestsByBucket = new
LinkedHashMap<>();
+ for (PurgePathRequest req : requests) {
+ requestsByBucket.computeIfAbsent(new VolumeBucketId(req.getVolumeId(),
req.getBucketId()),
+ k -> new ArrayList<>()).add(req);
+ }
+
List<OzoneManagerProtocolProtos.OMResponse> responses = new ArrayList<>();
- List<PurgePathRequest> purgePathRequestBatch = new ArrayList<>();
- long batchBytes = 0;
+ for (List<PurgePathRequest> bucketRequests : requestsByBucket.values()) {
+ List<PurgePathRequest> purgePathRequestBatch = new ArrayList<>();
+ long batchBytes = 0;
+
+ for (PurgePathRequest req : bucketRequests) {
+ int reqSize = req.getSerializedSize();
+
+ // If adding this request would exceed the limit, flush the current
batch first
+ if (batchBytes + reqSize > ratisByteLimit &&
!purgePathRequestBatch.isEmpty()) {
+ OzoneManagerProtocolProtos.OMResponse resp =
+ submitPurgeRequest(snapTableKey, expectedPreviousSnapshotId,
bucketNameInfoMap, purgePathRequestBatch);
+ if (!resp.getSuccess()) {
Review Comment:
submitPurgeRequest(...) can return null (eg when ServiceException is caught
in submitPurgeRequest). The subsequent resp.getSuccess() will throw a
NullPointerException, which would crash the DirectoryDeletingService task
instead of cleanly retrying next run.
This issue also appears on line 576 of the same file.
--
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]
