[improve][broker] Reduce the CPU pressure from the transaction buffer in rolling restarts #23062
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BewareMyPower
requested review from
lhotari,
hangc0276,
codelipenghui,
poorbarcode,
liangyepianzhou,
mattisonchao and
zymap
BewareMyPower
added
type/enhancement
BewareMyPower
changed the title
BewareMyPower
force-pushed
the
bewaremypower/transaction-buffer-optimize
branch
from
31d847c to
79430fc
Compare
|
This improvement for the reader side can be verified by the following example. public class LoadTopicTest extends ProducerConsumerBase {
@BeforeClass
@Override
protected void setup() throws Exception {
conf.setDefaultNumberOfNamespaceBundles(1);
conf.setNumTransactionReplayThreadPoolSize(1);
conf.setTransactionCoordinatorEnabled(true);
super.isTcpLookup = true;
super.internalSetup();
super.producerBaseSetup();
}
@AfterClass
@Override
protected void cleanup() throws Exception {
super.internalCleanup();
}
@Test
public void test() throws Exception {
final var topic = "test";
admin.topics().createPartitionedTopic(topic, 5);
@Cleanup final var producer = pulsarClient.newProducer().topic(topic + "-partition-0").create();
Thread.sleep(1000);
}
}Save the outputs into Even though the After this change, the logs become |
|
Mark it as draft to avoid repeated creation of reader. It's better to maintain a long live reader. |
…ders and writers in rolling restarts
### Motivation
During the rolling restarts, the namespace bundle ownerships will
change. Assuming there is a producer created on a single topic, and the
ownership was transferred to the new broker. Assuming the namespace
bundle has N topics and the namespace is `tenant/ns`,
1. All N topics in the same bundle of that topic will be loaded.
2. For each topic, the managed ledger will be initialized, when the
transaction coordinator is enabled, a `TopicTransactionBuffer` will
be created.
2.1 A Pulsar producer will be created on
`tenant/ns/__transaction_buffer_snapshot` concurrently.
2.2 A Pulsar reader will be created on
`tenant/ns/__transaction_buffer_snapshot` concurrently.
3. Once all N readers are created, the owner of the snapshot topic will
start dispatching messages to N readers. Each dispatcher will read
messages from BookKeeper concurrently and might fail with too many
requests error because BK can only have
`maxPendingReadRequestsPerThread` pending read requests (default: 10000).
We have a `numTransactionReplayThreadPoolSize` config to limit the
concurrency of transaction snapshot readers. However, it only limits the
read loop. For example, if it's configured with 1, only 1 reader could
read messages at the same time. However, N readers will be created
concurrently. Each when one of these reader explicitly calls `readNext`,
all N dispatchers at brokers side will dispatch messages to N readers.
The behaviors above brings much CPU pressure on the owner broker,
especially for a small cluster with only two brokers.
### Modifications
- Synchronize the reader creation, read loop and the following process
on its result. Maintain only one reader for each namespace.
BewareMyPower
force-pushed
the
bewaremypower/transaction-buffer-optimize
branch
from
9378fa9 to
ffcc578
Compare
15 tasks
|
@liangyepianzhou @congbobo184 @gaoran10 Please take a look |
BewareMyPower
force-pushed
the
bewaremypower/transaction-buffer-optimize
branch
from
ef22430 to
3b192a5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #23062 +/- ##
============================================
- Coverage 73.57% 73.47% -0.11%
- Complexity 32624 33530 +906
============================================
Files 1877 1919 +42
Lines 139502 144086 +4584
Branches 15299 15741 +442
============================================
+ Hits 102638 105860 +3222
- Misses 28908 30101 +1193
- Partials 7956 8125 +169
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Merge it first. |
nikhil-ctds
pushed a commit
to datastax/pulsar
that referenced
this pull request
Aug 8, 2024
… in rolling restarts (apache#23062) (cherry picked from commit 40c8c23) (cherry picked from commit 53fb549)
srinath-ctds
pushed a commit
to datastax/pulsar
that referenced
this pull request
Aug 12, 2024
… in rolling restarts (apache#23062) (cherry picked from commit 40c8c23) (cherry picked from commit 53fb549)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
During the rolling restarts, the namespace bundle ownerships will change. Assuming there is a producer created on a single topic, and the ownership was transferred to the new broker. Assuming the namespace bundle has N topics and the namespace is
tenant/ns,TopicTransactionBufferwill be created. A Pulsar reader will be created ontenant/ns/__transaction_buffer_snapshotconcurrently.maxPendingReadRequestsPerThreadpending read requests (default: 10000).We have a
numTransactionReplayThreadPoolSizeconfig to limit the concurrency of transaction snapshot readers. However, it only limits the read loop. For example, if it's configured with 1, only 1 reader could read messages at the same time. However, N readers will be created concurrently. Each when one of these reader explicitly callsreadNext, all N dispatchers at brokers side will dispatch messages to N readers.The behaviors above brings much CPU pressure on the owner broker, especially for a small cluster with only two brokers.
Modifications
Synchronize the reader creation, read loop and the following process on its result. Maintain only one reader for each namespace. The reader is now not closed unless there is no snapshot read request in 1 minute.
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: BewareMyPower#32