Copilot commented on code in PR #24975:
URL: https://github.com/apache/pulsar/pull/24975#discussion_r2518215702
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -1517,8 +1517,23 @@ private CompletableFuture<Void> delete(boolean
failIfHasSubscriptions,
"Topic has " + producers.size() + " connected
producers"));
}
} else if (currentUsageCount() > 0) {
- return FutureUtil.failedFuture(new TopicBusyException(
- "Topic has " + currentUsageCount() + " connected
producers/consumers"));
+ StringBuilder errorMsg = new StringBuilder("Topic has");
+ errorMsg.append(" ").append(currentUsageCount()).append("
clients connected.");
+ int consumerCount =
subscriptions.values().stream().map(sub -> sub.getConsumers().size())
+ .reduce(0, Integer::sum);
+ int replicatorCount = 0;
+ int producerCount = 0;
+ if (!producers.isEmpty()) {
+ replicatorCount = producers.values().stream().filter(p
-> p.isRemote()).map(p -> 1)
+ .reduce(0, Integer::sum);
+ if (producers.size() > replicatorCount) {
+ producerCount = producers.size() - replicatorCount;
+ }
+ }
+ errorMsg.append("
Including").append(consumerCount).append(" consumers,")
+ .append(" ").append(producerCount).append("
producers,").append(" and")
+ .append(" ").append(replicatorCount).append("
replicators.");
Review Comment:
The error message has formatting issues with missing spaces. The code
produces:
`"Topic has 1 clients connected. Including0 consumers, 0 producers, and 1
replicators."`
There's no space between "Including" and the number. Should be:
```java
errorMsg.append(" Including ").append(consumerCount).append(" consumers,")
.append(" ").append(producerCount).append(" producers,")
.append(" and ").append(replicatorCount).append(" replicators.");
```
This would produce:
`"Topic has 1 clients connected. Including 0 consumers, 0 producers, and 1
replicators."`
```suggestion
errorMsg.append(" Including
").append(consumerCount).append(" consumers,")
.append(" ").append(producerCount).append("
producers,").append(" and ")
.append(replicatorCount).append(" replicators.");
```
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -1517,8 +1517,23 @@ private CompletableFuture<Void> delete(boolean
failIfHasSubscriptions,
"Topic has " + producers.size() + " connected
producers"));
}
} else if (currentUsageCount() > 0) {
- return FutureUtil.failedFuture(new TopicBusyException(
- "Topic has " + currentUsageCount() + " connected
producers/consumers"));
+ StringBuilder errorMsg = new StringBuilder("Topic has");
+ errorMsg.append(" ").append(currentUsageCount()).append("
clients connected.");
Review Comment:
Grammar issue: "clients" should be singular "client" when
`currentUsageCount()` equals 1. Consider using:
```java
errorMsg.append(" ").append(currentUsageCount())
.append(currentUsageCount() == 1 ? " client" : " clients")
.append(" connected,");
```
This would properly handle both singular and plural cases.
```suggestion
errorMsg.append(" ").append(currentUsageCount())
.append(currentUsageCount() == 1 ? " client" : "
clients")
.append(" connected.");
```
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -1517,8 +1517,23 @@ private CompletableFuture<Void> delete(boolean
failIfHasSubscriptions,
"Topic has " + producers.size() + " connected
producers"));
}
} else if (currentUsageCount() > 0) {
- return FutureUtil.failedFuture(new TopicBusyException(
- "Topic has " + currentUsageCount() + " connected
producers/consumers"));
+ StringBuilder errorMsg = new StringBuilder("Topic has");
+ errorMsg.append(" ").append(currentUsageCount()).append("
clients connected.");
+ int consumerCount =
subscriptions.values().stream().map(sub -> sub.getConsumers().size())
+ .reduce(0, Integer::sum);
+ int replicatorCount = 0;
+ int producerCount = 0;
+ if (!producers.isEmpty()) {
+ replicatorCount = producers.values().stream().filter(p
-> p.isRemote()).map(p -> 1)
+ .reduce(0, Integer::sum);
Review Comment:
Inefficient stream operation: `.map(p -> 1).reduce(0, Integer::sum)` is
equivalent to simply counting. Consider using:
```java
replicatorCount = (int)
producers.values().stream().filter(Producer::isRemote).count();
```
This is clearer and more efficient.
```suggestion
replicatorCount = (int)
producers.values().stream().filter(p -> p.isRemote()).count();
```
--
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]
