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 2022/01/14 07:32:01 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #12900: [Broker] Return from callback early if producerFuture is complete or …
michaeljmarshall opened a new pull request #12900:
URL: https://github.com/apache/pulsar/pull/12900
…connection is closed
### Motivation
When producer creation times out on the client side, the producer sends a `CloseProducer` command and then a `Producer` command. Each time it tries to create a producer, it registers a callback on the same future because `getOrCreateTopic` loads from a cache. If the future completes after the timeout, multiple callbacks will run. We don't need to run the callbacks if the producer's future is complete or if the connection is no longer active.
My only open question is whether or not we should add an info or a debug log in the conditional. Note that if the future was already completed, we would have logged its closure. We'd also log the closure of the connection.
### Modifications
* Update `ServerCnx#handlProducer` to return early if the producer's future is complete or if the connection is no longer active.
### Verifying this change
This change is covered by existing tests.
### Does this pull request potentially affect one of the following parts:
This change is not breaking in any way.
### Documentation
- [x] `no-need-doc`
This is an internal optimization.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #12900: [Broker] Return from callback early if producerFuture is complete or …
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #12900:
URL: https://github.com/apache/pulsar/pull/12900#issuecomment-974789646
adding a log line won't add noise and we can see that we fall into this case in case of weird behaviour
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #12900: [Broker] Return from callback early if producerFuture is complete or …
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #12900:
URL: https://github.com/apache/pulsar/pull/12900#issuecomment-974789646
adding a log line won't add noise and we can see that we fall into this case in case of weird behaviour
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #12900: [Broker] Return from callback early if producerFuture is complete or …
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12900:
URL: https://github.com/apache/pulsar/pull/12900#issuecomment-1032970881
Closing for now. I have an alternate solution. This was only an optimization.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari closed pull request #12900: [Broker] Return from callback early if producerFuture is complete or …
Posted by GitBox <gi...@apache.org>.
lhotari closed pull request #12900:
URL: https://github.com/apache/pulsar/pull/12900
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a change in pull request #12900: [Broker] Return from callback early if producerFuture is complete or …
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12900:
URL: https://github.com/apache/pulsar/pull/12900#discussion_r755757661
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -1284,11 +1284,20 @@ private void buildProducerAndAddTopic(Topic topic, long producerId, String produ
boolean userProvidedProducerName, TopicName topicName,
ProducerAccessMode producerAccessMode,
Optional<Long> topicEpoch, CompletableFuture<Producer> producerFuture){
+ // Skip acquiring the topic's lock within addProducer if the producer will fail creation anyway.
+ if (!isActive()) {
+ log.info("[{}] Connection is no longer active. Skipping creation for produceId={} to topic {}.",
+ remoteAddress, topicName, producerId);
+ return;
Review comment:
Right, that make sense.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall closed pull request #12900: [Broker] Return from callback early if producerFuture is complete or …
Posted by GitBox <gi...@apache.org>.
michaeljmarshall closed pull request #12900:
URL: https://github.com/apache/pulsar/pull/12900
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #12900: [Broker] Return from callback early if producerFuture is complete or …
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #12900:
URL: https://github.com/apache/pulsar/pull/12900#issuecomment-1031460319
Please rebase this PR since there are conflicts.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #12900: [Broker] Return from callback early if producerFuture is complete or …
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #12900:
URL: https://github.com/apache/pulsar/pull/12900#discussion_r755715515
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -1284,11 +1284,20 @@ private void buildProducerAndAddTopic(Topic topic, long producerId, String produ
boolean userProvidedProducerName, TopicName topicName,
ProducerAccessMode producerAccessMode,
Optional<Long> topicEpoch, CompletableFuture<Producer> producerFuture){
+ // Skip acquiring the topic's lock within addProducer if the producer will fail creation anyway.
+ if (!isActive()) {
+ log.info("[{}] Connection is no longer active. Skipping creation for produceId={} to topic {}.",
+ remoteAddress, topicName, producerId);
+ return;
Review comment:
I agree that the paradigm is to complete the `producerFuture` exceptionally and then remove it from the `producers` map. We cannot send an error response here because the connection is closed.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #12900: [Broker] Return from callback early if producerFuture is complete or …
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12900:
URL: https://github.com/apache/pulsar/pull/12900#issuecomment-976994703
@eolivelli - I updated the PR to add logging. Also, after thinking about this change a bit more, and it makes more sense to delay the "early return" until just before calling `topic.addProducer`. Any of the preceding calls could be delayed and could then lead to a client side timeout.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a change in pull request #12900: [Broker] Return from callback early if producerFuture is complete or …
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12900:
URL: https://github.com/apache/pulsar/pull/12900#discussion_r755647088
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -1284,11 +1284,20 @@ private void buildProducerAndAddTopic(Topic topic, long producerId, String produ
boolean userProvidedProducerName, TopicName topicName,
ProducerAccessMode producerAccessMode,
Optional<Long> topicEpoch, CompletableFuture<Producer> producerFuture){
+ // Skip acquiring the topic's lock within addProducer if the producer will fail creation anyway.
+ if (!isActive()) {
+ log.info("[{}] Connection is no longer active. Skipping creation for produceId={} to topic {}.",
+ remoteAddress, topicName, producerId);
+ return;
Review comment:
It seems that we need call `commandSender.sendErrorResponse` to send the response of this Producer command?
Also need complete `producerFuture` with exception.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #12900: [Broker] Return from callback early if producerFuture is complete or …
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #12900:
URL: https://github.com/apache/pulsar/pull/12900#issuecomment-1012844979
Closing and re-opening to get recent changes from master branch.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org