poorbarcode commented on code in PR #24638:
URL: https://github.com/apache/pulsar/pull/24638#discussion_r2284297642
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2229,13 +2229,15 @@ protected void handleCloseConsumer(CommandCloseConsumer
closeConsumer) {
// create operation will complete, the new consumer will be
discarded.
log.info("[{}] Closed consumer before its creation was completed.
consumerId={}",
remoteAddress, consumerId);
+ consumers.remove(consumerId, consumerFuture);
commandSender.sendSuccessResponse(requestId);
return;
}
if (consumerFuture.isCompletedExceptionally()) {
log.info("[{}] Closed consumer that already failed to be created.
consumerId={}",
remoteAddress, consumerId);
+ consumers.remove(consumerId, consumerFuture);
Review Comment:
@BewareMyPower
@nodece wants to remove a consumer future that has not been done. Which is
the main change of this PR.
> @dao-jun I personally think we don't need to remove the Consumer Future
unless it's done.
Maybe we should introduce Subscription creation timeout for the case, to
avoid sub creation costs long time.
Agree with @dao-jun
---
@nodece
> When a consumer sends a subscribe command and then immediately sends a
close command while the subscription is still in progress, the broker logs
messages like:
Could you explain the progress of reproducing the issue? seems it is a
client-side bug, which should not call removing before the subscribing is done.
If the consumer can not confirm the result of the command that registering the
consumer, the client should close the socket
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2229,13 +2229,15 @@ protected void handleCloseConsumer(CommandCloseConsumer
closeConsumer) {
// create operation will complete, the new consumer will be
discarded.
log.info("[{}] Closed consumer before its creation was completed.
consumerId={}",
remoteAddress, consumerId);
+ consumers.remove(consumerId, consumerFuture);
commandSender.sendSuccessResponse(requestId);
return;
}
if (consumerFuture.isCompletedExceptionally()) {
log.info("[{}] Closed consumer that already failed to be created.
consumerId={}",
remoteAddress, consumerId);
+ consumers.remove(consumerId, consumerFuture);
Review Comment:
@BewareMyPower
@nodece wants to remove a consumer future that has not been done. Which is
the main change of this PR.
---
> @dao-jun I personally think we don't need to remove the Consumer Future
unless it's done.
Maybe we should introduce Subscription creation timeout for the case, to
avoid sub creation costs long time.
Agree with @dao-jun
---
@nodece
> When a consumer sends a subscribe command and then immediately sends a
close command while the subscription is still in progress, the broker logs
messages like:
Could you explain the progress of reproducing the issue? seems it is a
client-side bug, which should not call removing before the subscribing is done.
If the consumer can not confirm the result of the command that registering the
consumer, the client should close the socket
--
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: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
