zookeeper icon indicating copy to clipboard operation
zookeeper copied to clipboard

ZOOKEEPER-4575: ZooKeeperServer#processPacket take record instead of bytes

Open tisonkun opened this issue 2 years ago • 4 comments

This is the first step mentioned in ZOOKEEPER-102 and we should not play with ByteBuffer among the request handling code path.

tisonkun avatar Jul 17 '22 10:07 tisonkun

Verifying that we're in a good state. Will push further refactor logics.

tisonkun avatar Jul 17 '22 10:07 tisonkun

cc @eolivelli @symat @anmolnar

tisonkun avatar Jul 20 '22 01:07 tisonkun

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.

tisonkun avatar Sep 03 '22 12:09 tisonkun

ping @eolivelli @symat @anmolnar @maoling

tisonkun avatar Sep 14 '22 01:09 tisonkun

I tried to re-trigger CI. Seems to be some JAVA_HOME setup issue on the CI server?

symat avatar Sep 27 '22 09:09 symat

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:

  1. RequestRecord (ByteBufferRequestRecord) is a semantically identical refactoring over current codebase.
  2. About changing about the deserialization part during PrepProcessor, see https://github.com/apache/zookeeper/pull/1905#discussion_r962137523.

tisonkun avatar Oct 02 '22 10:10 tisonkun

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.

symat avatar Oct 03 '22 06:10 symat

CI still fails with the same java environment problem. I wonder if it affects also other PRs. Could maybe a rebase help here?

symat avatar Oct 03 '22 07:10 symat

@symat Thanks for your follow-up. I'm merging the lastest master.

tisonkun avatar Oct 04 '22 06:10 tisonkun

:warning: 52 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

sonatype-lift[bot] avatar Oct 04 '22 06:10 sonatype-lift[bot]

@symat unfortunately, it seems no help :(

tisonkun avatar Oct 05 '22 01:10 tisonkun

Let me try to update the travis config file...

tisonkun avatar Oct 05 '22 01:10 tisonkun

@symat Fixed now. Please help with merging :)

cc @eolivelli

tisonkun avatar Oct 05 '22 05:10 tisonkun

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.

symat avatar Oct 05 '22 07:10 symat

+1 to merge

eolivelli avatar Oct 05 '22 11:10 eolivelli

merged to the master branch. Thank you @tisonkun for your contribution!

symat avatar Oct 05 '22 15:10 symat

Thank you!

tisonkun avatar Oct 05 '22 16:10 tisonkun