[Issue 5597][pulsar-client-java] retry when getPartitionedTopicMetadata failed - #5603
Conversation
| if (isTcpLookup) { | ||
| url = "pulsar://localhost:51000,localhost:51001"; | ||
| } | ||
| // PulsarClient client = newPulsarClient(url, 0); |
There was a problem hiding this comment.
remove it if the code is not needed any more.
| .operationTimeout(3, TimeUnit.SECONDS) | ||
| .build(); | ||
|
|
||
| Consumer<byte[]> consumer = client.newConsumer().topic(topicName).subscriptionName(subscriptionName) |
There was a problem hiding this comment.
line 181 will never be executed, no?
| .build(); | ||
|
|
||
| Consumer<byte[]> consumer = client.newConsumer().topic(topicName).subscriptionName(subscriptionName) | ||
| .acknowledgmentGroupTime(0, TimeUnit.SECONDS).subscribe(); |
There was a problem hiding this comment.
Add faill() after creating a consumer since the consumer will never be created, no?
| consumer.close(); | ||
| producer.close(); | ||
| client.close(); | ||
| } catch (PulsarClientException pce) { |
| TopicName topicName = TopicName.get(topic); | ||
| metadataFuture = lookup.getPartitionedTopicMetadata(topicName); | ||
| AtomicLong opTimeoutMs = new AtomicLong(conf.getOperationTimeoutMs()); | ||
| Backoff backoff = new BackoffBuilder() |
There was a problem hiding this comment.
Not direct related to this change. but we should consider adding backoff for other places as well.
sijie
added
area/client
type/enhancement
sijie
changed the title
|
@ltamber it is always a good practice to copy the description from issues to the pull request. so it will provide enough context to review your pull request. I have updated your description. Please take a look and let me know if you have any questions. |
|
@sijie thanks a lot, I will fix my code soon. |
| String requestUrl = new URL(serviceNameResolver.resolveHostUri().toURL(), path).toString(); | ||
| String remoteHostName = serviceNameResolver.resolveHostUri().getHost(); |
There was a problem hiding this comment.
@sijie maybe a bug here, the host in requestUrl not equal with remoteHostName if multi-broker service url provided, and will step forward two service url every request. please take a look.
There was a problem hiding this comment.
sorry I don't quite get this. Can you show me an example?
There was a problem hiding this comment.
the PulsarServiceNameResolver.resolveHostUri() called twice(every time it will return the next address) inside HttpClient.get() ,
if service url is http://ip1:port1,ip2:port2, we assume requestUrl is ip1:port1, the remoteHostName will be ip2, and the next time call HttpClient.get() the requestUrl is still ip1:port1, the remoteHostName is ip2. so HttpClient.get() will only request ip1:port1
There was a problem hiding this comment.
oh I see. yes. the original code has a bug there.
…pache#5844) Motivation Fixes apache#5597 Add backoff retries when getting partitioned metadata from brokers. The change in apache#5734 (copy from apache#5603) used the wrong time unit when inited Backoff which failed to trigger the retry logic as expected. Modifications Correct the time unit and add some useful log.
4 participants
Motivation
Fixes #5597
When using a multi-broker service url to create a producer, if the connection to the first broker failed, the creation will fail.
Modification
Add backoff retries when getting partitioned metadata from brokers.