Optimize paginated getChildren #8

huizhilu · 2020-11-16T19:31:20Z

Description

Resolves #7

Changes for the PR:

On zk server, use a formulate to pre-calculate the children's packet length when adding the children to the returning list, so the children packet is ensured within the jute.maxbuffer limit. Formula: totalLength = (CHILD_EXTRA_BYTES + childLength) * numChildren + BASE_BYTES. As an example and tested in zk 3.6.x, the formula is like: total packet length = (4 + childNameLength) * numChildren + 100
Offer an option to return all children in the paginated getChildren API: when maxReturned == MAX_INT, minCzxid == -1. The pagination requests will be handled in the getChildren API, until there is no more page of children left.
Optimize the sorting logic for paginated getChildren in DataTree: only when pagination is needed, children are sorted. 99% of the case for returning all children, the children could be returned in the 1st page. No need to sort them to avoid performance downgrade.
Returning a list of child name string for the API, instead of a list of PathWithStat.
Add tests to cover additional logic.

Run Test

zookeeper-server

[INFO] Apache ZooKeeper - Server .......................... SUCCESS [20:05 min]

[INFO] Results:
[INFO]
[WARNING] Tests run: 3011, Failures: 0, Errors: 0, Skipped: 3
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  13:09 min
[INFO] Finished at: 2020-12-14T19:28:25-08:00
[INFO] ------------------------------------------------------------------------

junkaixue

Reviewed first part. Will review the later ones.

zookeeper-jute/src/main/resources/zookeeper.jute

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

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

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

jiajunwang

There is no test. And I think some of the business logic has not been completed yet. Please comment if I missed anything.

Please add enough tests to cover the new logic.

zookeeper-jute/src/main/resources/zookeeper.jute

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

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

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

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

huizhilu

Tried to split the PR into smaller ones. Since the logic on client and server in the original patch is not independent, I can not split the changes and make the tests pass. So I decided to put all the changes in one PR that we could review the logic.

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

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

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

zookeeper-jute/src/main/resources/zookeeper.jute

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

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

jiajunwang

It seems that I click Approve by mistake...

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

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

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

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

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

kaisun2000

Shall we add a todo to have a test for the the case that I raised as a followup, namely not clearing the hash set having may miss some data?

huizhilu · 2020-12-15T01:53:02Z

Shall we add a todo to have a test for the the case that I raised as a followup, namely not clearing the hash set having may miss some data?

If we want to add a unit test, we have to mock the required states for the API to simulate the read/write race condition. It'll take some time for me to complete the test. Since the logic is carefully reviewed and manually tested, I will merge the code first and create an issue (#9) for adding the unit test later.

The optimizes the paginated getChildren API to satisfy use cases better. The changes include: - On zk server, uses a formulate to pre-calculate the children's packet length when adding the children to the returning list, so the children packet is ensured within the jute.maxbuffer limit. - Adds an API getAllChildrenPaginated to fetch all children by leveraging the paginated getChildren API. - Optimize the sorting logic for paginated getChildren in DataTree: only when pagination is needed, children are sorted. 99% of the case for returning all children, the children could be returned in the 1st page. No need to sort them to avoid performance downgrade. - Returning a list of child name string for the API, instead of a list of PathWithStat - Add more tests to cover the new logic

…ory (linkedin#8)

huizhilu force-pushed the pagination branch from e2ee4de to bdac8d9 Compare November 18, 2020 09:12

huizhilu marked this pull request as ready for review November 18, 2020 09:13

huizhilu force-pushed the pagination branch from bdac8d9 to 064c1fe Compare November 19, 2020 01:59

huizhilu requested a review from junkaixue November 19, 2020 01:59

huizhilu force-pushed the pagination branch 3 times, most recently from 5b799eb to 7fc3e33 Compare November 19, 2020 05:05

junkaixue reviewed Nov 19, 2020

View reviewed changes

huizhilu force-pushed the pagination branch 7 times, most recently from 1aa85ac to 4ac2e1a Compare November 20, 2020 17:57

Optimize paginated getChildren

4ee53c4

huizhilu force-pushed the pagination branch 3 times, most recently from 37adb17 to 60d5e51 Compare November 20, 2020 23:48

huizhilu requested a review from junkaixue November 20, 2020 23:57

jiajunwang suggested changes Nov 23, 2020

View reviewed changes

Add tests

4ae2cee

huizhilu force-pushed the pagination branch from 60d5e51 to 4ae2cee Compare November 24, 2020 07:03

huizhilu commented Nov 24, 2020

View reviewed changes

Simplify code and remove lastPageOffset

956968e

huizhilu force-pushed the pagination branch from f74cfee to 956968e Compare November 24, 2020 08:56

kaisun2000 reviewed Nov 24, 2020

View reviewed changes

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

jiajunwang approved these changes Nov 25, 2020

View reviewed changes

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

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

jiajunwang suggested changes Nov 25, 2020

View reviewed changes

Simplify code for Zookeeper getChildren

c400bc1

huizhilu force-pushed the pagination branch from bfb636e to c400bc1 Compare November 30, 2020 18:44

junkaixue reviewed Nov 30, 2020

View reviewed changes

huizhilu force-pushed the pagination branch from dce1ae9 to 8645478 Compare December 4, 2020 05:39

NextPage returns the exact minCzxid and offset

898d08a

huizhilu force-pushed the pagination branch from 8645478 to 898d08a Compare December 4, 2020 19:42

kaisun2000 reviewed Dec 7, 2020

View reviewed changes

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

kaisun2000 reviewed Dec 7, 2020

View reviewed changes

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

kaisun2000 reviewed Dec 7, 2020

View reviewed changes

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

junkaixue reviewed Dec 8, 2020

View reviewed changes

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

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

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

Add retry reason in log

4ca8b70

huizhilu force-pushed the pagination branch from 7f606ea to 4ca8b70 Compare December 8, 2020 00:49

kaisun2000 approved these changes Dec 11, 2020

View reviewed changes

junkaixue approved these changes Dec 14, 2020

View reviewed changes

huizhilu mentioned this pull request Dec 15, 2020

Unit Test getAllChildrenPaginated for transaction children deleted case #9

Open

Fix checkstyle complaint

2a123bb

huizhilu merged commit 642bb52 into linkedin:li-dev/3.6.2-1-pagination Dec 15, 2020

rahulrane50 pushed a commit to linkedin/zkbridge that referenced this pull request Apr 25, 2024

Publishing zkbridge-core as zkbridge-server TAR/JAR files in artifact…

3c9d796

…ory (linkedin#8)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize paginated getChildren #8

Optimize paginated getChildren #8

huizhilu commented Nov 16, 2020 •

edited

Loading

junkaixue left a comment

jiajunwang left a comment

huizhilu left a comment

jiajunwang left a comment

kaisun2000 left a comment

huizhilu commented Dec 15, 2020

Optimize paginated getChildren #8

Optimize paginated getChildren #8

Conversation

huizhilu commented Nov 16, 2020 • edited Loading

Description

Run Test

junkaixue left a comment

Choose a reason for hiding this comment

jiajunwang left a comment

Choose a reason for hiding this comment

huizhilu left a comment

Choose a reason for hiding this comment

jiajunwang left a comment

Choose a reason for hiding this comment

kaisun2000 left a comment

Choose a reason for hiding this comment

huizhilu commented Dec 15, 2020

huizhilu commented Nov 16, 2020 •

edited

Loading