[smengcl]

Generated-by: Claude Code (Opus 5.5)

What changes were proposed in this pull request?

OMKeysRenameRequest.validateAndUpdateCache checks each pair against the key table, whose cache already holds the result of the earlier pairs of the same transaction, and stores the accepted pairs in a HashMap from source name to the new OmKeyInfo. OMKeysRenameResponse.addToDBBatch iterates that map and writes, per entry, a delete of the source and a put of the destination. When pairs of one transaction share key names, the batch depends on the map's iteration order, and a source named twice keeps only its last pair. The committed key table then differs from the cache: depending on the names, a key is committed twice on the same blocks, or a key reported as renamed disappears and its blocks leak. Once the cache entries are evicted after the flush, or on restart, the committed state is what every read sees.

This change makes OMKeysRenameRequest.preExecute reject the transaction with INVALID_REQUEST when a pair names a key that an earlier pair of the same transaction uses as source or destination. Pairs with an empty name are skipped, as validateAndUpdateCache never applies them. The check needs only the key names, so it runs on the leader before the transaction is replicated. validateAndUpdateCache and the response are unchanged, so entries already in the Raft log are applied as before.

Behaviour change: such a transaction is now rejected as a whole with INVALID_REQUEST and nothing in it is renamed. The check looks at names only, so it is broader than the combinations that leave the DB different from the cache. It also rejects transactions that commit correctly today (for example k to k_b, then k_b to k_c, for some key names) and transactions that end in a partial rename today: with only a present, b to c fails for its missing source and a to b succeeds (PARTIAL_RENAME), while the whole request is now rejected. Transactions whose pairs share no key behave as before.

Keeping the pairs in their original order in the response was considered and not taken: the map keyed by source name loses the earlier of two pairs with one source, so it would need an ordered list of pairs passed from the request to the response, a larger change of both.

What is the link to the Apache JIRA

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

How was this patch tested?

• Parameterized TestOMKeysRenameRequest.testCommittedKeyTableMatchesCache: runs a transaction through preExecute and validateAndUpdateCache, commits its response, and compares each key in the DB with the cache. Six cases with pairs that share a key must be rejected with INVALID_REQUEST, among them a repeated source, a repeated destination and an overlap between nonadjacent pairs; a case with disjoint pairs is the control. Without the change, three cases leave the DB different from the cache and all six fail on the missing rejection.
• testPairsSharingKeyRejectedInsteadOfPartialRename: preExecute rejects the partial rename example above; the same request applied by validateAndUpdateCache alone still ends in PARTIAL_RENAME with a renamed to b, which is how such an entry already in the Raft log is applied.
• TestOMKeysRenameRequest, TestOMKeysRenameResponse, TestOMKeyRenameRequest, TestOMKeyRenameRequestWithFSO, TestOMKeyRenameResponse and TestOMKeyRenameResponseWithFSO (41 tests) pass locally on master; checkstyle is clean.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[jojochuang]

Could you extend the regression tests with these scenarios, asserting INVALID_REQUEST?

• Repeated source: a -> b, a -> c (directly covers response-map entry replacement).
• Repeated destination: a -> c, b -> c.
• Nonadjacent overlap: a -> b, c -> d, b -> e (ensures names from all earlier pairs remain tracked).

Please also cover and clarify the compatibility restriction for a batch that previously allowed a valid partial result: suppose a exists and b and c do not, with b -> c followed by a -> b. The first pair fails because its source is missing, while the second can successfully rename a to b. The new name-only preExecute check instead rejects the whole batch because b is shared, even though the first pair would make no metadata changes.

I understand that rejecting overlaps is intentional, but this is broader than rejecting combinations that actually corrupt persistence. Please add a test documenting that behavior and mention the change from partial success to whole-request rejection in the PR description.

Generated-by: Codex (GPT-6)

[smengcl]

Thanks, added all three as cases of testCommittedKeyTableMatchesCache, plus a -> b, c -> a, a -> d. a -> b, a -> c alone does not replace the response map entry: the second pair finds a already deleted in the cache and is reported as not renamed. The entry is replaced only when a is recreated in between, as in the added case. Without the fix that case and the first two existing ones leave the DB different from the cache; the repeated source, repeated destination and nonadjacent cases commit correctly and fail only on the missing rejection.

The partial rename case is the new testPairsSharingKeyRejectedInsteadOfPartialRename: preExecute rejects b -> c, a -> b with INVALID_REQUEST, and the same request applied by validateAndUpdateCache alone still ends in PARTIAL_RENAME with a renamed to b. The description now states the change from partial success to rejecting the whole request.

[smengcl]

Thanks @jojochuang for the review.