Repository navigation
Conversation
|
@errose28 please take a look |
chungen0126
left a comment
chungen0126
left a comment
There was a problem hiding this comment.
Thanks @rich7420 for working on this. Could you please clarify the purpose of each test? Although they are quick unit tests, having this many variations feels redundant.
| @EnumSource(value = OzoneManagerVersion.class, names = {"DEFAULT_VERSION", "S3_LIST_MULTIPART_UPLOADS_PAGINATION", | ||
| "ATOMIC_CREATE_IF_NOT_EXISTS", "S3_DERIVED_KEY", "CONDITIONAL_DELETE", "ZDU", "UNKNOWN_VERSION"}) |
There was a problem hiding this comment.
Do we really need to test all the versions?
There was a problem hiding this comment.
Trimmed to the versions just before and at the gate.
| List<OMRequest> requests = new ArrayList<>(); | ||
| RpcClient client = createConditionalRequestClient(requests, version); | ||
| try { | ||
| for (String etag : Arrays.asList(null, "etag", "*", "")) { |
There was a problem hiding this comment.
Do we need to test all four senarios? I think testing Arrays.asList(null, "etag") is enough.
| @EnumSource(value = OzoneManagerVersion.class, names = {"DEFAULT_VERSION", "S3_LIST_MULTIPART_UPLOADS_PAGINATION", | ||
| "ATOMIC_CREATE_IF_NOT_EXISTS", "S3_DERIVED_KEY", "CONDITIONAL_DELETE", "ZDU", "UNKNOWN_VERSION"}) |
There was a problem hiding this comment.
Same here. Do we really need to test all the versions?
There was a problem hiding this comment.
Trimmed to the versions just before and at the gate.
|
Good point, a lot of them were testing the same thing. Trimmed them down in d45b89f, so each case now covers a different check. |
|
Trimmed this down to the client-side gates. The OM-side checkFeatureEnabled calls only read ozone.om.features.disabled, which no test sets for these features, so they don't protect against an older OM anyway. HDDS-17754 removes calls like that. |
# Conflicts: # hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
What changes were proposed in this pull request?
Conditional delete and conditional MPU completion can silently run unconditionally against an older OM, which ignores the condition fields it does not know.
RpcClientnow refuses them when the OM is too old. Conditional delete needs the new pre-ZDUCONDITIONAL_DELETEversion, so it stays available before ZDU finalization. Conditional MPU completion needsATOMIC_CREATE_IF_NOT_EXISTS, which shipped with it in 2.2.0.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16734
How was this patch tested?
On JDK 21 at
af061868e0:TestRpcClient(190) passed. Its conditional delete and MPU completion cases fail without the gates and pass with them.TestOMVersionManager(26) passed, and so did the OM request tests for create, delete and MPU completion.ozone-clientandozone-manager.