ivandika3 commented on code in PR #5668:
URL: https://github.com/apache/ozone/pull/5668#discussion_r1461312716
##########
hadoop-ozone/interface-client/src/main/resources/proto.lock:
##########
@@ -4915,6 +4921,12 @@
"name": "partName",
"type": "string",
"required": true
+ },
+ {
+ "id": 3,
+ "name": "eTag",
+ "type": "string",
+ "required": true
Review Comment:
Not really familiar with `proto.lock`, but in the `Part` proto definition,
the existing `partName` and `partName` are changed to optional. Shall we make
the `proto.lock` consistent as well?
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmMultipartUploadCompleteList.java:
##########
@@ -56,8 +56,8 @@ public Map<Integer, String> getMultipartMap() {
*/
public List<Part> getPartsList() {
List<Part> partList = new ArrayList<>();
- multipartMap.forEach((partNumber, partName) -> partList.add(Part
-
.newBuilder().setPartName(partName).setPartNumber(partNumber).build()));
+ multipartMap.forEach((partNumber, eTag) -> partList.add(Part
+ .newBuilder().setETag(eTag).setPartNumber(partNumber).build()));
Review Comment:
Related to the protobuf compatibillity (see further reviews below), is it
possible to set both partName and eTag to the same value?
```java
multipartMap.forEach((partNumber, eTag) -> partList.add(Part
.newBuilder().setPartName(eTag).setETag(eTag).setPartNumber(partNumber).build()));
```
Can also put a comment to explain the rationale behind setting the eTag for
both partName and eTag fields.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFSWithObjectStoreCreate.java:
##########
@@ -302,10 +304,13 @@ public void
testMPUFailDuetoDirectoryCreationBeforeComplete()
// This should succeed, as we check during creation of part or during
// complete MPU.
+ ozoneOutputStream.getMetadata().put("ETag",
+ DatatypeConverter.printHexBinary(MessageDigest.getInstance("Md5")
Review Comment:
We can unify the "Md5" using a constant variable.
Can be addressed in another ticket if needed.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestOpenKeyCleanupService.java:
##########
@@ -478,6 +479,8 @@ private void createIncompleteMPUKey(String volumeName,
String bucketName,
.setReplicationConfig(RatisReplicationConfig.getInstance(
HddsProtos.ReplicationFactor.ONE))
.setLocationInfoList(Collections.emptyList())
+ .addMetadata("ETag", DigestUtils.md5Hex(UUID.randomUUID()
Review Comment:
Nit: We can unify the "ETag" using a constant variable.
Can be addressed in another ticket if needed.
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1601,7 +1602,8 @@ message MultipartUploadCompleteResponse {
message Part {
required uint32 partNumber = 1;
- required string partName = 2;
+ optional string partName = 2;
Review Comment:
Not really familiar with protobuf compatibilities requirements, but from the
protobuf documentation, I saw that **Required is forever**. So it might not be
a good idea to change `partName` from `required` to `optional`.
> It is nearly impossible to safely change a field from required to
optional. If there is any chance that a stale reader exists, it will consider
messages without this field to be incomplete and may reject or drop them.
This might break if the new S3G contacts the old Ozone Manager without the
`partName` specified. We can try to avoid it by deploying new OM before S3G and
never rolling back the OM, but this might be error-prone.
Reference: https://protobuf.dev/programming-guides/proto2/
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmPartInfo.java:
##########
@@ -52,9 +55,13 @@ public long getSize() {
return size;
}
+ public String geteTag() {
Review Comment:
Nit: Let's standardize to `getETag`, similar to other methods
--
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]
