tisonkun commented on code in PR #2030:
URL: https://github.com/apache/zookeeper/pull/2030#discussion_r1254554851
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/FileTxnLog.java:
##########
@@ -308,6 +308,48 @@ public synchronized boolean append(TxnHeader hdr, Record
txn, TxnDigest digest)
return true;
}
+ @Override
+ public synchronized boolean append(Request request) throws IOException {
Review Comment:
We're duplicate code here.
Perhaps we can use one `append(Request request)` for all the overloads and
changes the other usage (which are all test usage I saw).
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java:
##########
@@ -164,6 +165,18 @@ public boolean isThrottlable() {
&& this.type != OpCode.createSession;
}
+ private transient byte[] serializeData;
+
+ @SuppressFBWarnings(value = "EI_EXPOSE_REP")
+ public byte[] getSerializeData() {
+ return serializeData;
+ }
+
+ @SuppressFBWarnings(value = "EI_EXPOSE_REP2")
+ public void setSerializeData(byte[] serializeData) {
+ this.serializeData = serializeData;
+ }
Review Comment:
After reading around the code, I suggest using a `getSerializeData`
interface and do the cache internally. This should supersede
`SerializeUtils.serializeRequest` and we have better encapsulation.
I leave this style comment here because we're hacking some existing code
while I believe that we don't have to owe some tech debt here.
--
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: notifications-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
