[Reopen][Issue 5597] Retry when getPartitionedTopicMetadata failed #5844

murong00 · 2019-12-12T10:10:02Z

Motivation

Add backoff retries when getting partitioned metadata from brokers.

The change in #5734 (copy from #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.

jiazhai

Thanks @murong00 for figuring out the issue. Would you please help add a unit test for the retry logic to make the code protected.

jiazhai · 2019-12-16T07:48:44Z

@murong00 Seems there was a test case testGetPartitionedTopicDataTimeout() , maybe we should improve it to make it really protecting the code.

murong00 · 2019-12-16T09:41:34Z

@jiazhai ok, will improve it later.

…-5844

murong00 · 2019-12-18T10:17:45Z

@jiazhai fix the unit test, please help to review again.

jiazhai · 2019-12-19T03:27:32Z

pulsar-broker/src/test/java/org/apache/pulsar/stats/client/PulsarBrokerStatsClientTest.java

+        String urlsWithUnreached = "http://localhost:51000,localhost:" + BROKER_WEBSERVICE_PORT;
        if (isTcpLookup) {
-            url = "pulsar://localhost:51000,localhost:51001";
+            urlsWithUnreached = "pulsar://localhost:51000,localhost::" + BROKER_PORT;


localhost::" + BROKER_PORT . there are 2 ":" here.

done, thanks.

jiazhai · 2019-12-19T03:55:21Z

pulsar-broker/src/test/java/org/apache/pulsar/stats/client/PulsarBrokerStatsClientTest.java


-    @Test (timeOut = 4000)
-    public void testGetPartitionedTopicDataTimeout() {
+    @Test


could we create a new test case, the original one is to test timeout, while your new test is try to test retry and success. also how about add some comments or change the test case name to make it more clear? e.g. according to the test steps, the name like 'testMultiHostUrlRetrySuccess' would be better. And some of the test cases are not related with PulsarBrokerStatsClientTest, It would be better to move them to or create a more related test suite, such as PulsarMultiHostClientTest?

nice advice, have fixed it.

jiazhai · 2019-12-19T03:55:57Z

Thanks @murong00 for the help, over all looks good to me. left some minor comments.

murong00 · 2019-12-20T01:21:41Z

run java8 tests
run cpp tests

sijie · 2019-12-20T05:18:32Z

run java8 tests
run cpp tests

murong00 · 2019-12-20T09:36:52Z

need fix NPE in ConnectionTimeoutTest:

2019-12-20\T\05:33:18.457 [ERROR] testLowTimeout(org.apache.pulsar.client.impl.ConnectionTimeoutTest)  Time elapsed: 31.139 s  <<< FAILURE!
java.lang.NullPointerException
	at org.apache.pulsar.client.impl.ConnectionTimeoutTest.testLowTimeout(ConnectionTimeoutTest.java:53)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:124)
	at org.testng.internal.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:54)
	at org.testng.internal.InvokeMethodRunnable.run(InvokeMethodRunnable.java:44)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

murong00 · 2019-12-23T02:52:58Z

run java8 tests
run cpp tests

sijie · 2019-12-24T02:30:38Z

run cpp tests
run java8 tests

sijie · 2019-12-24T10:03:59Z

run cpp tests
run java8 tests

murong00 · 2019-12-25T10:12:28Z

@sijie The retry logic may skip the limit of lookup throttle which can be set on client or server side (e.g. maxConcurrentLookupRequest,maxLookupRequests), for example unit test testLookupThrottlingForClientByClient() may fail since all consumer is connected.

murong00 · 2019-12-25T10:13:17Z

Meanwhile, fixed some unit test by setting operationTimeout to 1s (default 30s) in order to reduce unit testing time. Please help to take a look.

sijie · 2019-12-25T10:59:49Z

retest this please

murong00 · 2019-12-26T06:29:09Z

run java8 tests

jiazhai · 2019-12-30T02:11:22Z

run java8 tests

error:
org.apache.pulsar.io.PulsarFunctionE2ETest.testPulsarFunctionStats
org.apache.pulsar.io.PulsarFunctionE2ETest.testPulsarSinkStatsWithFile
org.apache.pulsar.io.PulsarFunctionE2ETest.testPulsarSinkStatsWithUrl
org.apache.pulsar.io.PulsarFunctionE2ETest.testPulsarSourceStatsWithFile
org.apache.pulsar.io.PulsarFunctionE2ETest.testPulsarSourceStatsWithUrl

sijie · 2020-01-01T01:12:06Z

run integration tests

… branch-5844

murong00 · 2020-01-02T08:35:16Z

run java8 tests

sijie · 2020-01-02T10:13:53Z

run java8 tests

murong00 · 2020-01-03T01:32:14Z

run java8 tests

tuteng · 2020-01-03T02:26:05Z

run java8 tests

jiazhai · 2020-01-03T03:09:41Z

run java8 tests

…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.

retry when getPartitionedTopicMetadata failed.

67c228e

murong00 requested review from jiazhai, sijie and wolfstudy December 12, 2019 10:11

jiazhai reviewed Dec 16, 2019

View reviewed changes

murong00 added 2 commits December 18, 2019 18:03

Merge branch 'master' of https://github.com/apache/pulsar into branch…

562217e

…-5844

Improve unit test to cover retry logic

acdc566

jiazhai reviewed Dec 19, 2019

View reviewed changes

Reorganize related unit test

9b48e7f

jiazhai approved these changes Dec 19, 2019

View reviewed changes

sijie approved these changes Dec 20, 2019

View reviewed changes

sijie assigned murong00 Dec 20, 2019

sijie added area/client type/bug The PR fixed a bug or issue reported a bug labels Dec 20, 2019

sijie added this to the 2.5.0 milestone Dec 20, 2019

Fix NPE in a unit test

d2d6c35

Added lookup throttle judgment and fixed some unit test

423f7da

Merge remote-tracking branch 'apache/master' into branch-5844

370c88b

murong00 added 4 commits January 2, 2020 15:16

fix some unit test

587cca4

Merge branch 'branch-5844' of https://github.com/murong00/pulsar into…

ff7cfd5

… branch-5844

fix some unit test

c081339

Merge branch 'branch-5844' of https://github.com/murong00/pulsar into…

8c35217

… branch-5844

tuteng merged commit c7094c9 into apache:master Jan 3, 2020

murong00 deleted the branch-5844 branch March 8, 2020 02:19

[Reopen][Issue 5597] Retry when getPartitionedTopicMetadata failed #5844

[Reopen][Issue 5597] Retry when getPartitionedTopicMetadata failed #5844

Uh oh!

Conversation

murong00 commented Dec 12, 2019

Motivation

Modifications

Uh oh!

jiazhai left a comment

Choose a reason for hiding this comment

Uh oh!

jiazhai commented Dec 16, 2019

Uh oh!

murong00 commented Dec 16, 2019

Uh oh!

murong00 commented Dec 18, 2019

Uh oh!

jiazhai Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

murong00 Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

jiazhai Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

murong00 Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

jiazhai commented Dec 19, 2019

Uh oh!

murong00 commented Dec 20, 2019

Uh oh!

sijie commented Dec 20, 2019

Uh oh!

murong00 commented Dec 20, 2019

Uh oh!

murong00 commented Dec 23, 2019

Uh oh!

sijie commented Dec 24, 2019

Uh oh!

sijie commented Dec 24, 2019

Uh oh!

murong00 commented Dec 25, 2019

Uh oh!

murong00 commented Dec 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sijie commented Dec 25, 2019

Uh oh!

murong00 commented Dec 26, 2019

Uh oh!

jiazhai commented Dec 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sijie commented Jan 1, 2020

Uh oh!

murong00 commented Jan 2, 2020

Uh oh!

sijie commented Jan 2, 2020

Uh oh!

murong00 commented Jan 3, 2020

Uh oh!

tuteng commented Jan 3, 2020

Uh oh!

jiazhai commented Jan 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

murong00 commented Dec 25, 2019 •

edited

Loading

jiazhai commented Dec 30, 2019 •

edited

Loading