[improve] [broker] high CPU usage caused by list topics under namespace #23049
Conversation
|
/pulsarbot rerun-failure-checks |
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
Outdated
Show resolved
Hide resolved
|
Besides, from the flame graph, the most CPU cost behavior is Actually, in this case, a simple string operation on a topic name string is enough. |
Improved |
|
+0 to this PR. I didn't left comments or request changes just to avoid blocking the merge. Generally, an API that returns CompletableFuture<List<String>> res = new CompletableFuture<>();
// Switch thread to avoid blocking other threads who are calling the current method.
pulsar.getExecutor().execute(() -> {
getListOfTopics(namespaceName, mode).thenApply(list -> {
return TopicList.filterSystemTopic(list);
}).whenComplete((topics, ex) -> {
if (ex != null) {
res.completeExceptionally(ex);
} else {
res.complete(topics);
}
});
});
return res;The code above is an example of bad coding style. The key method that costs CPU is return getListOfTopics(namespaceName, mode).thenApplyAsync(TopicList::filterSystemTopic, pulsar.getExecutor());The code is much more simple and clear:
After a private chat with @poorbarcode, he think return PulsarWebResource.checkLocalOrGetPeerReplicationCluster(pulsar, namespaceName, true)
.thenCompose(peerClusterData -> {
/* callback... */The callback could cost much time. If it's called in the same thread, the current thread could also be occupied for some time. For this case, I suggested changing From my perspective, we should keep the same coding style:
In short, switch the thread only when necessary. If we started to calling an asynchronous method in another method, then the coding style will be confusing. There are many other asynchronous methods, how should we determine whether to move to another thread. Eventually, we agree to disagree so I'm +0 to this PR and I won't block merging it. |
I think you are right, improved the code ❤️ |
poorbarcode
requested review from
BewareMyPower
and removed request for
BewareMyPower
pulsar-common/src/main/java/org/apache/pulsar/common/naming/SystemTopicNames.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
Show resolved
Hide resolved
|
Good job |
poorbarcode
force-pushed
the
improve/getUserTopics
branch
from
cd2ec2e
to
43e0e4a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #23049 +/- ##
============================================
- Coverage 73.57% 73.43% -0.14%
- Complexity 32624 33176 +552
============================================
Files 1877 1915 +38
Lines 139502 143943 +4441
Branches 15299 15725 +426
============================================
+ Hits 102638 105708 +3070
- Misses 28908 30128 +1220
- Partials 7956 8107 +151
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…ce (apache#23049) (cherry picked from commit 3e4f338) (cherry picked from commit 3f7206c)
…ce (apache#23049) (cherry picked from commit 3e4f338) (cherry picked from commit 3f7206c)
Motivation
NamespaceService. getListOfTopicscost too many CPU resourcesbroker_cpu.html.txt
Modifications
Performance review
See details in #23052
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: x