adoroszlai commented on code in PR #6751:
URL: https://github.com/apache/ozone/pull/6751#discussion_r1623242456
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1301,9 +1301,16 @@ message DeleteKeyArgs {
repeated string keys = 3;
}
+message DeleteKeyErrors {
+ required string key = 1;
+ required string errorCode = 2;
+ required string errorMsg = 3;
+}
+
message DeleteKeysResponse {
optional DeleteKeyArgs unDeletedKeys = 1;
optional bool status = 2;
+ repeated DeleteKeyErrors deleteKeyErrors = 3;
Review Comment:
nit: The structure represents a single error. Being `repeated` in the
response makes it `...Errors`. Also, since this response is specific to
deleting keys, field name can be shortened.
```suggestion
repeated DeleteKeyError errors = 3;
```
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java:
##########
@@ -675,6 +675,16 @@ public void deleteKeys(List<String> keyList) throws
IOException {
proxy.deleteKeys(volumeName, name, keyList);
}
+ /**
+ * Deletes the given list of keys from the bucket.
+ * @param keyList List of the key name to be deleted.
+ * @param isQuiet flag to not throw exception if delete fails
+ * @throws IOException
+ */
+ public Map<String, Pair<String, String>> deleteKeysQuiet(List<String>
keyList, Boolean isQuiet) throws IOException {
Review Comment:
Please:
- omit `Quiet` from the method name, since it has a parameter for quietness
- use primitive type `boolean` instead of `Boolean` object
- rename `isQuiet` parameter to `quiet` (`is` prefix applies to methods
usually)
Applies to all similar code.
```suggestion
public Map<String, Pair<String, String>> deleteKeys(List<String> keyList,
boolean quiet) throws IOException {
```
Please also consider creating a wrapper object for the error code, error
message pair, instead of using `Pair`. Having a custom type allows it to
evolve, if necessary. It also makes the code more readable, by using more
specific method names instead of `getLeft`, `getRight`.
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java:
##########
@@ -358,6 +359,18 @@ public void deleteKey(String key) throws IOException {
keyDetails.remove(key);
}
+ @Override
+ public Map<String, Pair<String, String>> deleteKeysQuiet(List<String>
keyList, Boolean isQuiet) throws IOException {
+ Map<String, Pair<String, String>> keyErrorMap = new HashMap<>();
+ for (String key : keyList) {
+ if (!keyDetails.containsKey(key)) {
+ keyErrorMap.put(key, Pair.of("KEY_NOT_FOUND", "Key does not exist"));
+ }
+ keyDetails.remove(key);
Review Comment:
nit:
```suggestion
if (!keyDetails.containsKey(key)) {
keyErrorMap.put(key, Pair.of("KEY_NOT_FOUND", "Key does not exist"));
} else {
keyDetails.remove(key);
}
```
or even:
```suggestion
if (keyDetails.remove(key) == null) {
keyErrorMap.put(key, Pair.of("KEY_NOT_FOUND", "Key does not exist"));
}
```
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1301,9 +1301,16 @@ message DeleteKeyArgs {
repeated string keys = 3;
}
+message DeleteKeyErrors {
+ required string key = 1;
+ required string errorCode = 2;
+ required string errorMsg = 3;
Review Comment:
Please make these `optional`, also for flexibility.
https://protobuf.dev/programming-guides/dos-donts/#add-required
--
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]
