lhotari commented on code in PR #23884:
URL: https://github.com/apache/pulsar/pull/23884#discussion_r1930010283
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -95,6 +96,9 @@ public abstract class AbstractTopic implements Topic,
TopicPolicyListener {
protected static final long POLICY_UPDATE_FAILURE_RETRY_TIME_SECONDS = 60;
protected final String topic;
+ @Getter
+ @Setter
+ protected volatile CompletableFuture<Optional<Topic>> createFuture;
Review Comment:
I wonder if it would make more sense to call this field `cacheKey`?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -95,6 +96,9 @@ public abstract class AbstractTopic implements Topic,
TopicPolicyListener {
protected static final long POLICY_UPDATE_FAILURE_RETRY_TIME_SECONDS = 60;
protected final String topic;
+ @Getter
+ @Setter
+ protected volatile CompletableFuture<Optional<Topic>> createFuture;
Review Comment:
It would be useful to document the purpose of this `createFuture`. Without
documentation, it could be hard to understand the reason why this field exists.
Please add javadocs. It could be useful to use ordinary getter and setter
methods so that javadocs could be added.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -2409,42 +2411,7 @@ public AuthorizationService getAuthorizationService() {
return authorizationService;
}
- public CompletableFuture<Void> removeTopicFromCache(Topic topic) {
- Optional<CompletableFuture<Optional<Topic>>> createTopicFuture =
findTopicFutureInCache(topic);
- if (createTopicFuture.isEmpty()){
- return CompletableFuture.completedFuture(null);
- }
- return removeTopicFutureFromCache(topic.getName(),
createTopicFuture.get());
- }
-
- private Optional<CompletableFuture<Optional<Topic>>>
findTopicFutureInCache(Topic topic){
- if (topic == null){
- return Optional.empty();
- }
- final CompletableFuture<Optional<Topic>> createTopicFuture =
topics.get(topic.getName());
- // If not exists in cache, do nothing.
- if (createTopicFuture == null){
- return Optional.empty();
- }
- // If the future in cache is not yet complete, the topic instance in
the cache is not the same with the topic.
- if (!createTopicFuture.isDone()){
- return Optional.empty();
- }
- // If the future in cache has exception complete,
- // the topic instance in the cache is not the same with the topic.
- if (createTopicFuture.isCompletedExceptionally()){
- return Optional.empty();
- }
- Optional<Topic> optionalTopic = createTopicFuture.join();
- Topic topicInCache = optionalTopic.orElse(null);
- if (topicInCache == null || topicInCache != topic){
- return Optional.empty();
- } else {
- return Optional.of(createTopicFuture);
- }
- }
-
- private CompletableFuture<Void> removeTopicFutureFromCache(String topic,
+ public CompletableFuture<Void> removeTopicFutureFromCache(String topic,
CompletableFuture<Optional<Topic>> createTopicFuture) {
Review Comment:
Please add javadoc to this method.
Instead of passing `String topic, CompletableFuture<Optional<Topic>>
createTopicFuture`, arguments to this method, I think that it could be a single
`AbstractTopic topic` parameter since AbstractTopic contains the getter for
createTopicFuture. This would properly encapsulate the implementation details.
The name of the method could be reconsidered. For example
`removeTopicFromCache` and documenting in the javadoc that it removes a
specific instance from the cache by cache key which is the topic future.
--
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]
