BewareMyPower commented on PR #23049:
URL: https://github.com/apache/pulsar/pull/23049#issuecomment-2236152274
+0 to this PR. I didn't left comments or request changes just to avoid
blocking the merge.
Generally, an API that returns `CompletableFuture<T>` should not be time
costed, so we should not switch to another thread to call it.
```java
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 `filterSystemTopic`. A proper change should be
```bash
return getListOfTopics(namespaceName,
mode).thenApplyAsync(TopicList::filterSystemTopic);
```
The code is much more simple and clear:
1. Call `getListOfTopics` in the current thread since it's an asynchronous
method that should not cost much time
2. Call `filterSystemTopic` in a separated thread because it's a pure
computing method that might cost time and might be called in the same thread
when the asynchronous method completes immediately.
After a private chat with @poorbarcode, he think `getListOfTopics` could
also block the current thread. Because it could call
`getListOfNonPersistentTopics`:
```java
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 `thenCompose` to `thenComposeAsync`. But @poorbarcode thought we have
to change many places for the sake.
From my perspective, we should keep the same coding style:
- When writing a method that returns `CompletableFuture`, ensure it's not
blocked. **Never switch it to another thread. If the callback could be called
in the same thread and take much time, switch the callback instead of the
method itself**
- When registering a method, you can choose to switch to another thread if
it costs much time.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
