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

tisonkun · 2022-07-17T10:01:47Z

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

Signed-off-by: tison <wander4096@gmail.com>

This reverts commit 6fc15a7.

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2022-07-17T10:03:55Z

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

Signed-off-by: tison <wander4096@gmail.com>

zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java

…hange is OK Signed-off-by: tison <wander4096@gmail.com>

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java

zookeeper-server/src/main/java/org/apache/zookeeper/server/ContainerManager.java

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2022-07-20T01:28:25Z

cc @eolivelli @symat @anmolnar

zookeeper-server/src/main/java/org/apache/zookeeper/server/ByteBufferRequestRecord.java

eolivelli · 2022-08-24T14:29:09Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java

@@ -359,9 +355,6 @@ protected void pRequest2Txn(int type, long zxid, Request request, Record record,
        case OpCode.delete:
            zks.sessionTracker.checkSession(request.sessionId, request.getOwner());
            DeleteRequest deleteRequest = (DeleteRequest) record;
-            if (deserialize) {


is this a behaviour change ? were is the deserialisation happening ?
are we doing it more times now ?

No. Previously we need this boolean flag because, after deserializing MultiOperationRecord, the subrequest is an in-memory data structure - there are no more bytes in the request to be deserialized.

Now, we always deserialize in pRequestHelper, no serialization happens in pRequest2Txn. Besides, we always serialize at most once in RequestRecord:

public class ByteBufferRequestRecord { @Override public <T extends Record> T readRecord(Supplier<T> constructor) throws IOException { if (record != null) { return (T) record; } // deserialization } } public class SimpleRequestRecord { @Override public <T extends Record> T readRecord(Supplier<T> constructor) { return (T) record; } }

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2022-09-03T12:37:52Z

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.

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2022-09-14T01:58:17Z

ping @eolivelli @symat @anmolnar @maoling

symat

Thanks @tisonkun for the PR and sorry for the (very) late response. I went through the change, looks good to me. I like the new abstraction.

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 · 2022-09-27T09:59:53Z

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

eolivelli

Great work. @tisonkun !

tisonkun · 2022-10-02T10:47:00Z

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 ZOOKEEPER-4575: ZooKeeperServer#processPacket take record instead of bytes #1905 (comment).

symat

Thanks, looks good to me

symat · 2022-10-03T06:58:57Z

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 · 2022-10-03T07:00:32Z

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

tisonkun · 2022-10-04T06:27:11Z

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

sonatype-lift · 2022-10-04T06:38:59Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java

💬 2 similar findings have been found in this PR

THREAD_SAFETY_VIOLATION: Read/Write race. Non-private method PrepRequestProcessor.pRequest2Txn(...) indirectly reads without synchronization from auth.ProviderRegistry.initialized. Potentially races with write in method PrepRequestProcessor.pRequest2Txn(...).
Reporting because this access may occur on a background thread.

🔎 Expand here to view all instances of this finding

File Path Line Number

zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java 326

zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java 317

Visit the Lift Web Console to find more details in your report.

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2022-10-04T06:39:02Z

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

tisonkun · 2022-10-05T01:10:42Z

@symat unfortunately, it seems no help :(

tisonkun · 2022-10-05T01:14:29Z

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

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2022-10-05T05:40:52Z

@symat Fixed now. Please help with merging :)

cc @eolivelli

symat · 2022-10-05T07:17:28Z

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.

eolivelli · 2022-10-05T11:39:08Z

+1 to merge

symat · 2022-10-05T15:58:04Z

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

tisonkun · 2022-10-05T16:07:35Z

Thank you!

…bytes This is the first step mentioned in [ZOOKEEPER-102](https://issues.apache.org/jira/browse/ZOOKEEPER-102) and we should not play with ByteBuffer among the request handling code path. Author: tison <wander4096@gmail.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org> Closes apache#1905 from tisonkun/request-supplier

…bytes (#79) This is the first step mentioned in [ZOOKEEPER-102](https://issues.apache.org/jira/browse/ZOOKEEPER-102) and we should not play with ByteBuffer among the request handling code path. Author: tison <wander4096@gmail.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org> Closes apache#1905 from tisonkun/request-supplier Co-authored-by: tison <wander4096@gmail.com>

tisonkun added 6 commits July 17, 2022 16:02

cleanup a bit code

e719875

Signed-off-by: tison <wander4096@gmail.com>

tidy Request constructor

6fc15a7

Signed-off-by: tison <wander4096@gmail.com>

Revert "tidy Request constructor"

19e45b6

This reverts commit 6fc15a7.

introduce RequestRecord

1bd079f

Signed-off-by: tison <wander4096@gmail.com>

wire existing logic

bf5e292

Signed-off-by: tison <wander4096@gmail.com>

checkstyle

8abaf45

Signed-off-by: tison <wander4096@gmail.com>

tisonkun added 8 commits July 17, 2022 18:06

checkstyle

2485156

Signed-off-by: tison <wander4096@gmail.com>

encapsulate more bytes manipulations

4f725e0

Signed-off-by: tison <wander4096@gmail.com>

checkstyle

426ffdc

Signed-off-by: tison <wander4096@gmail.com>

move function to member method

63f867d

Signed-off-by: tison <wander4096@gmail.com>

keep deserialize parameter only to pRequest2Txn

233b9ab

Signed-off-by: tison <wander4096@gmail.com>

always deserialize before process

98aea1d

Signed-off-by: tison <wander4096@gmail.com>

properly handle deserialization error

d772af8

Signed-off-by: tison <wander4096@gmail.com>

suppress false positive spotbug report

b48af8d

Signed-off-by: tison <wander4096@gmail.com>

sonatype-lift bot reviewed Jul 18, 2022

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java Show resolved Hide resolved

sonatype-lift bot reviewed Jul 18, 2022

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java Outdated Show resolved Hide resolved

sonatype-lift bot reviewed Jul 18, 2022

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java Show resolved Hide resolved

fix test. deleteContainer only generated on the server side and the c…

6c6ec30

…hange is OK Signed-off-by: tison <wander4096@gmail.com>

sonatype-lift bot reviewed Jul 18, 2022

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java Show resolved Hide resolved

tisonkun commented Jul 18, 2022

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/server/ContainerManager.java Outdated Show resolved Hide resolved

tisonkun added 2 commits July 18, 2022 15:44

fix compatibility

a3ca9c4

Signed-off-by: tison <wander4096@gmail.com>

retrigger CI

18c5b25

Signed-off-by: tison <wander4096@gmail.com>

tisonkun changed the title ~~WIP - ZooKeeperServer#processPacket take record instead of bytes~~ ZOOKEEPER-4575: ZooKeeperServer#processPacket take record instead of bytes Jul 20, 2022

eolivelli requested changes Aug 24, 2022

View reviewed changes

tisonkun added 2 commits September 3, 2022 18:30

Merge branch 'master' into request-supplier

dd3463c

replace reflection with supplier

03b16e3

Signed-off-by: tison <wander4096@gmail.com>

tisonkun requested a review from eolivelli September 3, 2022 10:55

trigger ci

0eb5d5e

Signed-off-by: tison <wander4096@gmail.com>

symat requested changes Sep 27, 2022

View reviewed changes

eolivelli approved these changes Sep 29, 2022

View reviewed changes

symat approved these changes Oct 3, 2022

View reviewed changes

Merge branch 'master' into request-supplier

4cc0467

sonatype-lift bot reviewed Oct 4, 2022

View reviewed changes

tisonkun added 3 commits October 5, 2022 09:20

tidy travis config

a1c85fb

Signed-off-by: tison <wander4096@gmail.com>

try export JAVA_HOME on arm64

2defeb5

Signed-off-by: tison <wander4096@gmail.com>

try fix yaml multiple lines

21d2391

Signed-off-by: tison <wander4096@gmail.com>

asfgit closed this in 3daefac Oct 5, 2022

tisonkun deleted the request-supplier branch October 5, 2022 16:07

anuragmadnawat1 mentioned this pull request Nov 2, 2022

ZOOKEEPER-4575: ZooKeeperServer#processPacket take record instead of bytes anuragmadnawat1/zookeeper#79

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tisonkun commented Jul 17, 2022 •

edited

tisonkun commented Jul 17, 2022

tisonkun commented Jul 20, 2022

eolivelli Aug 24, 2022

tisonkun Sep 3, 2022 •

edited

tisonkun commented Sep 3, 2022

tisonkun commented Sep 14, 2022

symat left a comment

symat commented Sep 27, 2022

eolivelli left a comment

tisonkun commented Oct 2, 2022

symat left a comment

symat commented Oct 3, 2022

symat commented Oct 3, 2022

tisonkun commented Oct 4, 2022

sonatype-lift bot Oct 4, 2022

sonatype-lift bot commented Oct 4, 2022

tisonkun commented Oct 5, 2022

tisonkun commented Oct 5, 2022

tisonkun commented Oct 5, 2022

symat commented Oct 5, 2022

eolivelli commented Oct 5, 2022

symat commented Oct 5, 2022

tisonkun commented Oct 5, 2022

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

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

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

Conversation

tisonkun commented Jul 17, 2022 • edited

tisonkun commented Jul 17, 2022

tisonkun commented Jul 20, 2022

eolivelli Aug 24, 2022

Choose a reason for hiding this comment

tisonkun Sep 3, 2022 • edited

Choose a reason for hiding this comment

tisonkun commented Sep 3, 2022

tisonkun commented Sep 14, 2022

symat left a comment

Choose a reason for hiding this comment

symat commented Sep 27, 2022

eolivelli left a comment

Choose a reason for hiding this comment

tisonkun commented Oct 2, 2022

symat left a comment

Choose a reason for hiding this comment

symat commented Oct 3, 2022

symat commented Oct 3, 2022

tisonkun commented Oct 4, 2022

sonatype-lift bot Oct 4, 2022

Choose a reason for hiding this comment

sonatype-lift bot commented Oct 4, 2022

tisonkun commented Oct 5, 2022

tisonkun commented Oct 5, 2022

tisonkun commented Oct 5, 2022

symat commented Oct 5, 2022

eolivelli commented Oct 5, 2022

symat commented Oct 5, 2022

tisonkun commented Oct 5, 2022

tisonkun commented Jul 17, 2022 •

edited

tisonkun Sep 3, 2022 •

edited