Repository navigation
HDDS-16364. Exclude multipart open keys from Recon open key reports - #11685
lpavanvenkat wants to merge 4 commits into
Conversation
…ads in Recon open key reports
|
@priyeshkaratha @ArafatKhan2198 @devmadhuu , pls do review this pr, Thanks. |
There was a problem hiding this comment.
Thanks @lpavanvenkat for the patch. Now that CI checks have completed, please keep this PR the way it is, unless someone requests code changes. I definitely do not want another round of CI to be triggered for this alone. However, in the future please add documentation in ozone-site. Leftover docs are being removed from this repo in #10829.
chungen0126
left a comment
There was a problem hiding this comment.
Thanks @lpavanvenkat for working on this. Just took a quick look and left a couple of comments.
| * Returns the (unreplicated, replicated) size of the committed parts of a single MPU, read from the embedded | ||
| * parts for legacy-schema MPUs and from {@code multipartPartsTable} for split-schema MPUs. | ||
| */ | ||
| public static Pair<Long, Long> getCommittedPartsSize(OmMultipartKeyInfo multipartKeyInfo, |
There was a problem hiding this comment.
According to HDDS-15942, please avoid using Pair.
| * Returns the (unreplicated, replicated) size of the committed parts of the multipart upload that a multipart | ||
| * open key belongs to, or zero if the upload can no longer be found. | ||
| */ | ||
| private Pair<Long, Long> getMultipartUploadSize(String dbKey, OmKeyInfo keyInfo) throws IOException { |
There was a problem hiding this comment.
According to HDDS-15942, please avoid using Pair.
Since changes to avoid using |
|
@lpavanvenkat largely changes looks fine, just one comment and a nit. Could we handle older FSO multipart open-key records whose keyName contains only the filename? Before HDDS-10649, the open-file write path stored the leaf name. For an unfinished upload of The lookup returns null and reports zero despite committed parts being present. The new FSO test uses the full Could we resolve the full key path for these records and add a test? If support is intentionally deferred, please document this additional limitation and track the follow-up. One optional test suggestion: cover erasure-coded uploads. |
|
Hi @devmadhuu , thanks for catching this edge case! However, do we really need to handle this for backward compatibility? These records only exist on clusters upgraded from a release before ozone 2.0 with incomplete FSO multipart uploads. MultipartUploadCleanupService aborts any MPU older than These uploads were initiated before the upgrade, they are gone within about 31 days after upgrade. Is it worth adding more code complexity just to avoid a monitoring inaccuracy during that window? |
|
@lpavanvenkat @devmadhuu Normally if key is part of open key / open file table, its never recorded about datasize, as it comes only after its committed. And for MPU, its counted only after part is committed. Till that point, that open part is never counted to MPU. I think, after part is committed to MPU, it will show the bytes used as MPU. IMO, open key/ open file, should not be counted as part of MPU in summary, but be counted as open key/file only.
|
|
I have similar concerns to Sumit. Recon is a debugging tool so it is most effective when it reports what is literally stored. Open key stats should be for anything in the open key table, and MPU stats should be for anything in the MPU tables. This helps us effectively troubleshoot issues with background services that work on those tables. Specifically for this PR, that would mean all open keys (including uncommitted MPU parts) are reported as open key usage. Once a part is committed, it is no longer tracked as an open key and instead tracked using separate stats for committed but incomplete MPU parts. |
|
I agree with both Sumit and Ethan on this point that they both should be kept separate. |
|
@errose28 @sumitagrawl agreed on keeping open key and MPU stats separate. My original proposal for HDDS-16364 follows that: Problem: at MPU initiate, OM writes a multipart open key with size 0 and no blocks, which stays until the upload is completed or aborted. Recon counts it as a regular open key, while committed parts are reported by /keys/open/mpu/summary. For example, start 2 multipart uploads and commit 2 parts of 5 MiB to each, without completing either upload. Recon then reports: /keys/open/summary: 2 open keys, 0 bytes Fix (Recon only): exclude multipart open keys (isMultipartKey=true) from /keys/open and /keys/open/summary, and report uploads only via /keys/open/mpu/summary. With the same 2 uploads, while a third part of one upload is still being written, Recon would report: /keys/open/summary: 1 open key (the part being written) This matches OM: getExpiredOpenKeys (used by OpenKeyCleanupService) skips these keys, and expired ones are cleaned up only by MultipartUploadCleanupService. The tradeoff is that the stored open key totals must be recomputed after upgrade, via a Recon upgrade action that queues a task reinitialization (as existing upgrade actions do). Would you prefer this, or leaving the open key view as is? |
further changes requested by others
Thanks @lpavanvenkat for giving clarity and explanation. IMO above makes sense to filter out committed key parts from openkey summary using flag @sumitagrawl @errose28 any concerns on this ? |
priyeshkaratha
left a comment
There was a problem hiding this comment.
Hi @lpavanvenkat Please check the inline comments provided. Also the two endpoints can report different totals.
- /keys/open adds up live per-row lookups.
- /keys/open/summary reads the counters maintained by Recon's background stats task.
Please check this.
| * Returns the (unreplicated, replicated) size of the committed parts of a single MPU, read from the embedded | ||
| * parts for legacy-schema MPUs and from {@code multipartPartsTable} for split-schema MPUs. | ||
| */ | ||
| public static Pair<Long, Long> getCommittedPartsSize(OmMultipartKeyInfo multipartKeyInfo, |
There was a problem hiding this comment.
This method re-implements what applyLegacyPartSizes and applyPartSize do, and its replicated-size logic differs (OMMultipartUploadUtils.getReplicatedSize versus QuotaUtil per part). It's easy for the two to drift apart, for example in how EC padding is handled. One shared helper would fix this.
| public Map<String, Long> getOpenKeySummaryWithMPU() { | ||
| Map<String, Long> keysSummary = getOpenKeySummary(); | ||
| Map<String, Long> mpuSummary = getMPUKeySummary(); | ||
| keysSummary.merge("totalReplicatedDataSize", mpuSummary.get("totalReplicatedDataSize"), Long::sum); |
There was a problem hiding this comment.
Only /keys/open/summary uses getOpenKeySummaryWithMPU. Other callers of getOpenKeySummary(), such as the cluster-state and overview pages, still leave out multipart part sizes. Users would see different open-key totals depending on the page.
Thanks @devmadhuu. Small clarification: committed parts are already out of the open key table, since OM deletes a part's open key on commit. The filter would drop only the size-0 multipart open key that OM writes at initiate (isMultipartKey=true). Parts still being written stay counted as open keys. |
|
Thanks @lpavanvenkat for the clarification. That explanation makes sense. It looks different than the PR description though:
This seems to indicate that the 10mb of committed but not completed parts were not counted towards open key size before this PR, but that they are after the PR. Your latest comment says that the 10mb of completed parts would not count towards the open key size.
|
Thanks @errose28, you're right, they don't match. The PR description describes the current code, which adds committed part sizes to /keys/open and /keys/open/summary. The proposal replaces that: committed parts are counted only in /keys/open/mpu/summary, and the size-0 multipart open key is excluded from /keys/open and /keys/open/summary. For one MPU with 10 MiB of committed parts and no part being written, the proposal would give:
If this works for you, I'll rework the PR to this approach and update the description. |
|
Looks good to me. I think we can bring the code and descriptions in sync with this proposal. |
Conflicts: hadoop-hdds/docs/content/interface/ReconApi.md Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open issues
mpuSummary.get("totalReplicatedDataSize")/mpuSummary.get("totalDataSize")can return null (or… · NewgetMultipartUploadSize(...)performs extra DB reads (and for split-schema, scans parts) per… · New This computes unreplicated size with a stream pass overparts.values()and then likely iterates… · New
What changed in this PR
Align Recon open key reporting by including committed multipart upload part sizes in /keys/open rows and /keys/open/summary, and fix MPU size stats correctness across Recon restarts.
Changes:
- Add committed MPU part sizes to
/keys/openrow sizes and page totals by resolving upload IDs and summing committed parts. - Update
/keys/open/summaryto include MPU committed part sizes without changing open-key counts. - Fix split-schema MPU stats after Recon restart by caching replication config on
multipartInfoTableDELETE events; add targeted unit tests.
| File | Description |
|---|---|
| hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/tasks/TestOmTableInsightTask.java | Adds unit test covering split-schema MPU delete processing after Recon restart (empty cache). |
| hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestOmDBInsightEndPoint.java | Adds unit tests validating open key summary + MPU sizes and multipart open key row sizing behavior. |
| hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/MultipartInfoInsightHandler.java | Adds committed-parts size helper and caches replication config on delete to keep MPU stats correct after restart. |
| hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ReconGlobalMetricsService.java | Introduces getOpenKeySummaryWithMPU() to add MPU bytes to open key summary sizes. |
| hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightEndpoint.java | Uses the new summary method and computes multipart open key row size by summing committed parts. |
Review effort: Lite
| keysSummary.merge("totalReplicatedDataSize", mpuSummary.get("totalReplicatedDataSize"), Long::sum); | ||
| keysSummary.merge("totalUnreplicatedDataSize", mpuSummary.get("totalDataSize"), Long::sum); |
| Pair<Long, Long> uploadSize = getMultipartUploadSize(dbKey, keyInfo); | ||
| keyEntityInfo.setSize(uploadSize.getLeft()); | ||
| keyEntityInfo.setReplicatedSize(uploadSize.getRight()); | ||
| } else { | ||
| keyEntityInfo.setSize(keyInfo.getDataSize()); | ||
| keyEntityInfo.setReplicatedSize(keyInfo.getReplicatedSize()); | ||
| } | ||
| keyEntityInfo.setCreationTime(keyInfo.getCreationTime()); |
| SortedMap<Integer, OmMultipartPartInfo> parts = | ||
| OMMultipartUploadUtils.scanParts(omMetadataManager, multipartKeyInfo.getUploadID()); | ||
| long dataSize = parts.values().stream().mapToLong(OmMultipartPartInfo::getDataSize).sum(); | ||
| return Pair.of(dataSize, OMMultipartUploadUtils.getReplicatedSize(parts, multipartKeyInfo.getReplicationConfig())); |
author said: "I'll rework the PR to this approach", it will need to be re-reviewed afterwards
|
@errose28 @sumitagrawl @devmadhuu @priyeshkaratha @smengcl, I've reworked the PR to the approach agreed above and updated the description. Please take another look. Summary of the change:
On the earlier review comments:
I tested an upgrade from master on a compose cluster. The details are in the "How was this patch tested?" section. Thanks! |
smengcl
left a comment
There was a problem hiding this comment.
Thanks @lpavanvenkat . It almost looks good to me
| private static final Logger LOG = | ||
| LoggerFactory.getLogger(OMDBInsightEndpoint.class); | ||
| /** Excludes multipart open keys, which are reported by {@code /keys/open/mpu/summary} instead. */ | ||
| private static final Predicate<OmKeyInfo> NOT_MULTIPART_OPEN_KEY = |
There was a problem hiding this comment.
A question, not blocking: with this filter, Recon no longer lists individual incomplete MPUs anywhere. Before this change, /keys/open at least showed each upload's key name and creation time (with 0 bytes), while /keys/open/mpu/summary only gives totals. If an operator sees a large MPU total, how would they find which uploads are holding the space? If a per-upload listing is planned as a follow-up, could we file a JIRA for it and link it here?
There was a problem hiding this comment.
Thanks @smengcl. Yes, this is intentional: with this PR, incomplete MPUs are reported only by /keys/open/mpu/summary, as described above. A per-upload view is planned in HDDS-17802: a toggle on the Overview open keys card to switch between open keys and multipart uploads, and an "Open Multipart Uploads" tab on OM DB Insights that lists each upload with its key, upload ID, parts, committed size and age, backed by a new list endpoint. Until then, uploads can be listed per bucket through the S3 ListMultipartUploads API.


What changes were proposed in this pull request?
Recon reports incomplete S3 multipart uploads (MPUs) twice. When an MPU is initiated, OM writes a multipart open key
(
isMultipartKey=true, size 0) to the open key/file table, and tracks the upload inmultipartInfoTableandmultipartPartsTable. Recon counted the multipart open key as a regular open key, so the same upload appeared bothin the open key endpoints, with 0 bytes, and in
/keys/open/mpu/summary, with the size of its committed parts.Following the review discussion, multipart open keys are now excluded from the open key endpoints, and incomplete
MPUs are reported only by
/keys/open/mpu/summary. For one MPU with 10 MiB of committed parts:/keys/open/keys/open/summary/keys/open/mpu/summaryOpen keys of parts that are still being written (
isMultipartKey=false) are regular open keys and are still listedand counted.
Changes:
/keys/open:ReconUtils.extractKeysFromTablehas a new overload that takes a filter, andOMDBInsightEndpointpasses!OMMultipartUploadUtils.isMultipartKeySet(keyInfo)for the OBS scan and both FSOscans. The filter is applied during the scan, so excluded keys do not count towards
limitand paging is notaffected.
/keys/open/summary:OpenKeysInsightHandlerskips multipart open keys on put and delete events and in thefull recount (
getTableSizeAndCount).OPEN_KEYS_EXCLUDE_MULTIPART_UPLOADS(7) and its upgrade actionOpenKeysExcludeMultipartUploadsUpgradeActionqueue a task reinitialization to recompute them. If the reinitialization cannot be queued at startup (for example,
because Recon has not loaded its OM DB copy yet), the controller retries it on the next OM sync, the same way as
the NSSummary rebuild upgrade action.
MultipartInfoInsightHandlercaches each split-layout MPU's replicationconfig to compute the replicated size of part events (HDDS-16363). The cache was filled only on
multipartInfoTablePUT/UPDATE, so after a Recon restart, aborting or completing an MPU that was already openskipped every part delete (
No cached replication config ... skipping part size update), leaving its bytes in/keys/open/mpu/summary. OM deletes themultipartInfoTablerow before itsmultipartPartsTablerows in thesame batch, so the handler now also caches the config on the
multipartInfoTableDELETE event.The earlier approach in this PR (adding committed part sizes to the open key endpoints) has been removed.
Known limitation, not changed here:
multipartInfoTablefor them), so their multipart open keys are still reported as open keys.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16364
How was this patch tested?
TestOmDBInsightEndPoint#testGetOpenKeyInfoExcludesMultipartOpenKeys: OBS and FSO listings exclude multipartopen keys, and paging with
limitstill returns full pages.TestOmTableInsightTask#testProcessExcludesMultipartOpenKeys: put and delete events of multipart open keys donot change the open key stats, while regular open keys are still counted.
TestOmTableInsightTask#testReprocessExcludesMultipartOpenKeys: the full recount skips multipart open keys.TestOmTableInsightTask#testProcessForSplitSchemaMPUDeletedAfterRestart: a new task instance (empty cache)processes an MPU delete followed by its part deletes and the sizes return to 0. It fails without the fix.
TestOpenKeysExcludeMultipartUploadsUpgradeActionandTestReconVersionManager: the upgrade action queues thereinitialization, does not fail the upgrade if queueing fails, and is registered for version 7.
checkstyle.sh: 0 violations.rat.sh: no unapproved files.ozonecompose cluster (3 datanodes, FSO bucket, 2 MPUs with 2 parts of 5 MiB each),starting on master and then upgrading only Recon to this patch with its data kept:
/keys/open/summaryreported 2 open keys with 0 bytes and/keys/openlisted both MPUs./keys/open/summaryreported 0 open keys and/keys/openreturned 204./keys/open/mpu/summarystill reported 2 MPUs and 20 MiB./keys/openlisted only the part's open key and/keys/open/summaryreported 1 open key and 20 MiB. After the part was committed, the open key count returned to 0 and the MPU
summary showed 20 MiB.
Co-authored-by: with Cursor (Claude Opus 5.5)