jerrypeng commented on a change in pull request #11627:
URL: https://github.com/apache/pulsar/pull/11627#discussion_r688085063
##########
File path:
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java
##########
@@ -243,7 +245,7 @@ protected ConsumerImpl(PulsarClientImpl client, String
topic, ConsumerConfigurat
this.initialStartMessageId = this.startMessageId;
this.startMessageRollbackDurationInSec =
startMessageRollbackDurationInSec;
AVAILABLE_PERMITS_UPDATER.set(this, 0);
- this.subscribeTimeout = System.currentTimeMillis() +
client.getConfiguration().getOperationTimeoutMs();
+ this.subscribeTimeout = System.currentTimeMillis() +
client.getConfiguration().getLookupTimeoutMs();
Review comment:
The time to subscribe will include time to do a lookup + the time to
create a connection (if the connection to the broker is not established yet).
However, with out current code we are including the connection establishment
time within our lookup time. This makes this timeout here confusing and hard
to reason about as it may or may not include the time to establish a
connection. Also establishing a connection has its own timeout which defaults
to 10 seconds. I think we should clearly separate the two timeouts so one is
not just overlapping with the other and we can clearly understand if subscribe
failed because of a lookup timeout or a connection timeout.
Same for producers.
--
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]
