Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Transparent batching of ZK operations #13043

Merged
merged 11 commits into from
Dec 2, 2021
Merged

Transparent batching of ZK operations #13043

merged 11 commits into from
Dec 2, 2021

Conversation

merlimat
Copy link
Contributor

Motivation

It is more efficient to do operations to ZK in batch, using the ZooKeeper.multi() operation instead of individual calls.
This reduces the RPC traffic between ZK client and servers and it also reduces the number of write transactions, since each multi() correspond to a single ZK transaction, containing multiple write operations.

Modifications

  • Added a base class AbstractBatchedMetadataStore which contains the batching logic
  • Added configurable batching parameters
  • Using 2 different transaction sets, one for read operations and the other just for write operations
  • Added code of (very) basic benchmark

Preliminary results

This is done with the included basic benchmark, making request to the embedded ZK running on a laptop:

image

image

@merlimat merlimat added the type/feature The PR added a new feature or issue requested a new feature label Nov 30, 2021
@merlimat merlimat added this to the 2.10.0 milestone Nov 30, 2021
@merlimat merlimat self-assigned this Nov 30, 2021
@merlimat merlimat added doc-required Your PR changes impact docs and you will update later. and removed doc-label-missing labels Nov 30, 2021
@apache apache deleted a comment from github-actions bot Nov 30, 2021
@merlimat merlimat mentioned this pull request Nov 30, 2021
Closed
2 tasks
eolivelli
eolivelli reviewed Nov 30, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java Outdated Show resolved Hide resolved
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java Show resolved Hide resolved
...ata/src/main/java/org/apache/pulsar/metadata/impl/batching/AbstractBatchedMetadataStore.java Outdated Show resolved Hide resolved
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataBenchmark.java
import org.testng.annotations.Test;

@Slf4j
public class MetadataBenchmark extends MetadataStoreTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How many resources does this test use? How does it take to complete?
Does it make sense to keep this running on CI?

I believe that if the test is no heavyweight we can keep it, but I wanted to double check with you

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just takes a couple of seconds for each type and it doesn't take many resources, just keeping 2 threads busy.

@lhotari
Copy link
Member

lhotari commented Nov 30, 2021

Awesome!

lhotari
lhotari reviewed Nov 30, 2021
View reviewed changes
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! One minor comment about exception handling.

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java Outdated Show resolved Hide resolved
codelipenghui
codelipenghui reviewed Dec 2, 2021
View reviewed changes
...ata/src/main/java/org/apache/pulsar/metadata/impl/batching/AbstractBatchedMetadataStore.java
batchOperation(Collections.singletonList(op));
return;
}
if (queue.size() > maxOperations && flushInProgress.compareAndSet(false, true)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will not trigger a flush if reach the size limitation? Looks like the current implementation is split into multiple batches when flushing, if we have enough data should be flush not the operations, we will wait for maxDelayMillis.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change LGTM and just left a minor comment, I think it should not be a big problem here, but will simplify implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, there are several optimizations that are still possible in the batching logic :) I'd say to continue in subsequent PRs.

Jason918
Jason918 approved these changes Dec 2, 2021
View reviewed changes
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@merlimat merlimat merged commit 0497f67 into apache:master Dec 2, 2021
@merlimat merlimat deleted the zk-batching branch December 2, 2021 02:57
@yuruguo yuruguo mentioned this pull request Dec 3, 2021
Merged
1 task
@Anonymitaet
Copy link
Member

For the doc side, I've discussed with @codelipenghui, the following changes will be made after 2.9.0 is released:

  1. Add parameters to the Pulsar configuration page.

  2. Add descriptions and two images in this PR (result comparison) to ZooKeeper.

@merlimat any thoughts? Thanks

fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
@merlimat
Transparent batching of ZK operations (apache#13043)
37ef27a 
* Transparent batching of ZK operations

* Addressed comments

* Handle default switch case

* Fixed issues in MockZookeeper with usage of multi()

* Wrap Throwable with MetadataStoreException

* Fixed getChildren in MockZookeeper

* Handle cases in which ZK is failing the partial request, but with error=OK

* Fixed test to wait for put result

* Fixed test that was not waiting on put future to complete
@momo-jun momo-jun mentioned this pull request Jan 14, 2022
Merged
1 task
@Jason918 Jason918 mentioned this pull request Jan 21, 2022
Closed
@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Jan 24, 2022
@poorbarcode poorbarcode mentioned this pull request May 11, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhotari lhotari lhotari left review comments

@eolivelli eolivelli eolivelli left review comments

@Jason918 Jason918 Jason918 approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@ivankelly ivankelly Awaiting requested review from ivankelly

@rdhabalia rdhabalia Awaiting requested review from rdhabalia

@jerrypeng jerrypeng Awaiting requested review from jerrypeng

@hangc0276 hangc0276 Awaiting requested review from hangc0276

@BewareMyPower BewareMyPower Awaiting requested review from BewareMyPower

@massakam massakam Awaiting requested review from massakam

Assignees

@merlimat merlimat

Labels
doc-complete Your PR changes impact docs and the related docs have been already added. type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@merlimat @lhotari @Anonymitaet @Jason918 @eolivelli @codelipenghui