You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/01/04 02:07:36 UTC
[GitHub] [pulsar-client-go] merlimat opened a new pull request #162: Avoid
double cnx close notification on graceful server side close
merlimat opened a new pull request #162: Avoid double cnx close notification on graceful server side close
URL: https://github.com/apache/pulsar-client-go/pull/162
### Motivation
We're processing the "connection closed" event twice when the broker sends a "producer closed" or "consumer closed" command.
Basically after the producer reconnects, it will reconnect again on the same connection because it got the notification 2 times. One when the graceful close command and another when the connection is closed (eg: broker is shutting down).
This is not harmful, though it's logically wrong.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#162: Avoid double cnx close notification on graceful server side close
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #162: Avoid double cnx close notification on graceful server side close
URL: https://github.com/apache/pulsar-client-go/pull/162#discussion_r363045025
##########
File path: pulsar/internal/connection.go
##########
@@ -550,16 +550,19 @@ func (c *connection) handleCloseConsumer(closeConsumer *pb.CommandCloseConsumer)
consumerID := closeConsumer.GetConsumerId()
if consumer, ok := c.consumerHandler(consumerID); ok {
consumer.ConnectionClosed()
+ delete(c.listeners, consumerID)
Review comment:
Should we use this function DeleteConsumeHandler to delete the handler? We get the handler using the consumerHandlersLock so to be consistent should we delete with it too? Also does it matter if we delete before or after calling the ConnectionClosed()?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#162: Avoid double cnx close notification on graceful server side close
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #162: Avoid double cnx close notification on graceful server side close
URL: https://github.com/apache/pulsar-client-go/pull/162#discussion_r363045420
##########
File path: pulsar/internal/connection.go
##########
@@ -550,16 +550,19 @@ func (c *connection) handleCloseConsumer(closeConsumer *pb.CommandCloseConsumer)
consumerID := closeConsumer.GetConsumerId()
if consumer, ok := c.consumerHandler(consumerID); ok {
consumer.ConnectionClosed()
+ delete(c.listeners, consumerID)
Review comment:
Seems like this is a different map so it ok.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#162: Avoid double cnx close notification on graceful server side close
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #162: Avoid double cnx close notification on graceful server side close
URL: https://github.com/apache/pulsar-client-go/pull/162#discussion_r363045025
##########
File path: pulsar/internal/connection.go
##########
@@ -550,16 +550,19 @@ func (c *connection) handleCloseConsumer(closeConsumer *pb.CommandCloseConsumer)
consumerID := closeConsumer.GetConsumerId()
if consumer, ok := c.consumerHandler(consumerID); ok {
consumer.ConnectionClosed()
+ delete(c.listeners, consumerID)
Review comment:
Should we use this function DeleteConsumeHandler to delete the handler? We get the handler using the consumerHandlersLock so to be consistent should we delete with it too?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#162: Avoid double cnx close notification on graceful server side close
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #162: Avoid double cnx close notification on graceful server side close
URL: https://github.com/apache/pulsar-client-go/pull/162#discussion_r363045420
##########
File path: pulsar/internal/connection.go
##########
@@ -550,16 +550,19 @@ func (c *connection) handleCloseConsumer(closeConsumer *pb.CommandCloseConsumer)
consumerID := closeConsumer.GetConsumerId()
if consumer, ok := c.consumerHandler(consumerID); ok {
consumer.ConnectionClosed()
+ delete(c.listeners, consumerID)
Review comment:
Seems like this is a different map so it's ok.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] merlimat merged pull request #162: Avoid double
cnx close notification on graceful server side close
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #162: Avoid double cnx close notification on graceful server side close
URL: https://github.com/apache/pulsar-client-go/pull/162
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services