zookeeper
zookeeper copied to clipboard
ZOOKEEPER-4575: ZooKeeperServer#processPacket take record instead of bytes
This is the first step mentioned in ZOOKEEPER-102 and we should not play with ByteBuffer among the request handling code path.
Verifying that we're in a good state. Will push further refactor logics.
cc @eolivelli @symat @anmolnar
CI failed. Although I don't think it's related. Will retry later.
$ java -Xmx32m -version
openjdk version "11.0.16.1" 2022-08-12 LTS
OpenJDK Runtime Environment (build 11.0.16.1+1-LTS)
OpenJDK 64-Bit Server VM (build 11.0.16.1+1-LTS, mixed mode)
$ javac -J-Xmx32m -version
javac 11.0.16.1
install
12.82s$ if [ "${TRAVIS_CPU_ARCH}" == "arm64" ]; then sudo apt-get install maven; fi
0.08s$ mvn clean apache-rat:check verify -DskipTests spotbugs:check checkstyle:check -Pfull-build
Error: JAVA_HOME is not defined correctly.
We cannot execute /usr/lib/jvm/bellsoft-java11-arm64/bin/java
The command "mvn clean apache-rat:check verify -DskipTests spotbugs:check checkstyle:check -Pfull-build" exited with 1.
ping @eolivelli @symat @anmolnar @maoling
I tried to re-trigger CI. Seems to be some JAVA_HOME setup issue on the CI server?
Should we fear of any performance degradation? I think it is unlikely, but I haven't spent enough time on the PR to be sure and want to hear your opinion.
@symat I don't have to completed performance coverage on this change. From the code change perspective:
-
RequestRecord
(ByteBufferRequestRecord
) is a semantically identical refactoring over current codebase. - About changing about the deserialization part during
PrepProcessor
, see https://github.com/apache/zookeeper/pull/1905#discussion_r962137523.
Not related to this PR necessarily, but it would be nice to come up with some simple reproducible perf test later. Especially because this is part of a larger refactoring and in the end we should see if we have any degradation with Jute and also to compare Jute with any other protocol we choose later.
CI still fails with the same java environment problem. I wonder if it affects also other PRs. Could maybe a rebase help here?
@symat Thanks for your follow-up. I'm merging the lastest master.
:warning: 52 God Classes were detected by Lift in this project. Visit the Lift web console for more details.
@symat unfortunately, it seems no help :(
Let me try to update the travis config file...
@symat Fixed now. Please help with merging :)
cc @eolivelli
cool, thanks @tisonkun ! @eolivelli - do you think it is OK if I go ahead and merge this? Having travis fix mixed up with the refactoring might not be nice, but on the other hand we won't cherry pick this one to other branches anyway.
+1 to merge
merged to the master branch. Thank you @tisonkun for your contribution!
Thank you!