BewareMyPower commented on code in PR #24658:
URL: https://github.com/apache/pulsar/pull/24658#discussion_r2296720503
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java:
##########
@@ -473,15 +478,16 @@ public boolean test(NamespaceBundle namespaceBundle) {
private void initPolicesCache(SystemTopicClient.Reader<PulsarEvent>
reader, CompletableFuture<Void> future) {
if (closed.get()) {
future.completeExceptionally(new
BrokerServiceException(getClass().getName() + " is closed."));
-
cleanCacheAndCloseReader(reader.getSystemTopic().getTopicName().getNamespaceObject(),
false);
+
cleanCache(reader.getSystemTopic().getTopicName().getNamespaceObject(), false,
true);
return;
}
reader.hasMoreEventsAsync().whenComplete((hasMore, ex) -> {
if (ex != null) {
log.error("[{}] Failed to check the move events for the system
topic",
reader.getSystemTopic().getTopicName(), ex);
future.completeExceptionally(ex);
-
cleanCacheAndCloseReader(reader.getSystemTopic().getTopicName().getNamespaceObject(),
false);
+
cleanCache(reader.getSystemTopic().getTopicName().getNamespaceObject(), false,
+ isAlreadyClosedException(ex));
Review Comment:
Yes, it makes sense to avoid creating the reader if the previous creation
failed and no ownership switching happened after that. But could you add a test
to mock this case? I mean, we should guarantee `getTopicPoliciesAsync` will
still return a value that might be outdated rather than fail in this case.
--
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]
