gardenia commented on code in PR #9363:
URL: https://github.com/apache/ozone/pull/9363#discussion_r3309560993
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -917,6 +917,35 @@ message SnapshotDiffJobProto {
optional double keysProcessedPct = 13;
}
+/**
+ * Per request type entities to hold arguments
+ * captured for CompletedRequestInfo
+ */
+message RenameKeyOperationArgs {
+ optional string toKeyName = 1;
+}
+
+message CreateFileOperationArgs {
+ optional bool isRecursive = 1;
+ optional bool isOverwrite = 2;
+}
+
+/**
+ * CompletedRequestInfo table entry
+ */
+message CompletedRequestInfo {
+
+ optional int64 trxLogIndex = 1;
+ optional Type cmdType = 2; // Type of the command
+ optional string volumeName = 3;
+ optional string bucketName = 4;
+ optional string keyName = 5;
+ optional uint64 creationTime = 6;
+
+ optional RenameKeyOperationArgs renameKeyArgs = 7;
+ optional CreateFileOperationArgs createFileArgs = 8;
+}
Review Comment:
@peterxcli Thanks for your comments.
I'm just checking I fully understand your intent.
1. in the diffs of your suggested change the RenameKeyOperationArgs and
CreateFileOperationArgs are removed but then they are used in your "oneof". Is
this just a diff anomaly i.e. is the substance of your suggestion:
```
diff --git
a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
index 12708817c3..bcf464c75d 100644
--- a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
+++ b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
@@ -942,8 +942,10 @@ message CompletedRequestInfo {
optional string keyName = 5;
optional uint64 creationTime = 6;
- optional RenameKeyOperationArgs renameKeyArgs = 7;
- optional CreateFileOperationArgs createFileArgs = 8;
+ oneof operationArgs {
+ RenameKeyOperationArgs renameKeyArgs = 7;
+ CreateFileOperationArgs createFileArgs = 8;
+ }
}
message OzoneObj {
```
I'm fine with using the "oneof" that but there is no existing precedent of
using "oneof" in OmClientProtocol.proto so I'd just want us to ensure there are
no protocol compatibility issues with doing this (or project specific
guidelines that would disallow it).
>then I think large portion of the code in OmCompletedRequestInfo can be
remove, especially the entire >OperationArgs class hierarchy can go
To be clear, are you suggesting that at the moment the
OmCompletedRequestInfo class would directly return proto instances rather than
have this "middleman" family of inline wrapper / runtime POJO classes?
e.g. :
```
- public - public OperationArgs getOpArgs() {
- return opArgs;
+ public OzoneManagerProtocolProtos.RenameKeyOperationArgs
getRenameKeyArgs() {
+ return renameKeyArgs;
+ }
+
+ public OzoneManagerProtocolProtos.CreateFileOperationArgs
getCreateFileArgs() {
+ return createFileArgs;
} getOpArgs() {
```
I have roughed out a full patch
([ozone_eventnotification_proto_changes.txt](https://github.com/user-attachments/files/28301016/ozone_eventnotification_proto_changes.txt))
of my interpretation of your comments - can you please have a quick look and
see if it aligns with your intent. If so I will put up a separate PR. Thanks.
--
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]
