[xalestar]

What changes were proposed in this pull request?

ozone admin om list-open-files fails with NumberFormatException when the listed range contains an open S3 multipart upload. OmMetadataManagerImpl.listOpenFiles parses the last segment of every open key as a client ID, but the open key that S3InitiateMultipartUploadRequest writes for an upload ends with the upload ID instead. The exception is not an IOException, so the client sees a RemoteException and retries, and the whole listing fails.

This change detects an upload row by its key with the existing OMMultipartUploadUtils.getUploadIdFromDbKey and lists it with client ID 0, which no real client has. The check is key based rather than based on the isMultipartKey flag, because the open keys of parts being written also carry that flag and do end with a real client ID, and upload rows written before HDDS-9017 do not carry it. Listing the row instead of skipping it keeps the result consistent with totalOpenKeyCount and leaves pagination unchanged.

The fix and the test are based on the patch Siyao Meng attached to the JIRA issue.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-17251

How was this patch tested?

Added testListOpenFilesWithMultipartUpload to TestOmMetadataManager, for every bucket layout, with and without a continuation token. It lists a plain open key, an upload row without the isMultipartKey flag, and the open key of a part being written.

• mvn test -pl hadoop-ozone/ozone-manager -Dtest=TestOmMetadataManager: 33 tests pass.
• With the change to OmMetadataManagerImpl reverted, the new test fails for all 3 bucket layouts with NumberFormatException.
• mvn checkstyle:check -pl hadoop-ozone/ozone-manager: clean.
• Fork CI: https://github.com/xalestar/ozone/actions/runs/40111938707

[adoroszlai]

Thanks @xalestar for the patch. Please enable workflows in your fork.

[F64116045]

Thanks @xalestar, the fix looks good. Just a minor comment

Btw, it would be nice to attach the fork CI link to the PR description.

[F64116045]

nit: I think you use 0 to mean "no client ID" for an upload
Since there's already a comment right above, maybe we could just extend it to make this explicit?

[xalestar]

Thanks, extended the comment to state that client ID 0 means the upload has no client ID.

[xalestar]

Thanks @adoroszlai, enabled. The fork CI link is now in the PR description.

[peterxcli]

+1 LGTM. @xalestar if possible, could you explore whether we can add a class that parses the dbKey into a structured open key record, and stores some info that can be used easily? For example, key = new OpenKey(dbKey), key.isUpload(), key.getClientId().

[F64116045]

thanks for the fix, looks good +1!

[xalestar]

+1 LGTM. @xalestar if possible, could you explore whether we can add a class that parses the dbKey into a structured open key record, and stores some info that can be used easily? For example, key = new OpenKey(dbKey), key.isUpload(), key.getClientId().

Thanks @peterxcli! Yes, I'll look into an OpenKey parser. Should I do it in this PR, or in a separate PR after this one is merged?

[peterxcli]

Thanks @peterxcli! Yes, I'll look into an OpenKey parser. Should I do it in this PR, or in a separate PR after this one is merged?

separate is good, thank you!

[peterxcli]

Thanks @xalestar @F64116045 @adoroszlai!