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/07/12 09:30:27 UTC
[GitHub] [pulsar] AnonHxy opened a new pull request, #16540: [test][broker]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
AnonHxy opened a new pull request, #16540:
URL: https://github.com/apache/pulsar/pull/16540
Master Issue: https://github.com/apache/pulsar/issues/16521
### Motivation
Fixes https://github.com/apache/pulsar/issues/16521
### Modifications
Fix UT BrokerServiceTest.testLookupThrottlingForClientByClient
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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] poorbarcode commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r941980683
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -1056,7 +1105,12 @@ public void testLookupThrottlingForClientByClient() throws Exception {
long reqId10 = reqId++;
ByteBuf request10 = Commands.newLookup(topicName, true, reqId10);
f3 = pool.getConnection(resolver.resolveHost())
- .thenCompose(clientCnx -> clientCnx.newLookup(request10, reqId10));
+ .thenCompose(clientCnx -> {
+ CompletableFuture<?> future = clientCnx.newLookup(request10, reqId10);
+ // pending other responses in `ClientCnx` until now
+ latch[0].countDown();
Review Comment:
Why not code like this (Simpler and better understood):
```java
latch[0] = new CountDownLatch(1);
f1 = pool.getConnection(resolver.resolveHost()).thenCompose(clientCnx -> clientCnx.newLookup(request8,reqId8));
f2 = pool.getConnection(resolver.resolveHost()).thenCompose(clientCnx -> clientCnx.newLookup(request9, reqId9));
f3 = pool.getConnection(resolver.resolveHost()).thenCompose(clientCnx -> clientCnx.newLookup(request9, reqId9));
latch[0].countDown();
f1.get();
f2.get();
f3.get();
fail("At least one should fail");
```
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -979,7 +980,31 @@ public void testLookupThrottlingForClientByClient() throws Exception {
EventLoopGroup eventLoop = EventLoopUtil.newEventLoopGroup(20, false,
new DefaultThreadFactory("test-pool", Thread.currentThread().isDaemon()));
long reqId = 0xdeadbeef;
- try (ConnectionPool pool = new ConnectionPool(conf, eventLoop)) {
+
+ // using an array in order to reset a new CountDownLatch
+ CountDownLatch[] latch = new CountDownLatch[1];
Review Comment:
Is there has thread safety issue with multiple threads accessing `CountDownLatch[] latch`? Now the code should be fine, I feel like maybe 'AtomicReference' is better
--
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] poorbarcode commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r941992837
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -1056,7 +1105,12 @@ public void testLookupThrottlingForClientByClient() throws Exception {
long reqId10 = reqId++;
ByteBuf request10 = Commands.newLookup(topicName, true, reqId10);
f3 = pool.getConnection(resolver.resolveHost())
- .thenCompose(clientCnx -> clientCnx.newLookup(request10, reqId10));
+ .thenCompose(clientCnx -> {
+ CompletableFuture<?> future = clientCnx.newLookup(request10, reqId10);
+ // pending other responses in `ClientCnx` until now
+ latch[0].countDown();
Review Comment:
Hi @AnonHxy and this one comment :)
--
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] poorbarcode commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r940518329
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -979,7 +981,32 @@ public void testLookupThrottlingForClientByClient() throws Exception {
EventLoopGroup eventLoop = EventLoopUtil.newEventLoopGroup(20, false,
new DefaultThreadFactory("test-pool", Thread.currentThread().isDaemon()));
long reqId = 0xdeadbeef;
- try (ConnectionPool pool = new ConnectionPool(conf, eventLoop)) {
+
+ Semaphore clientCnxSemaphore = new Semaphore(1);
+ AtomicInteger count = new AtomicInteger(2);
+ try (ConnectionPool pool = new ConnectionPool(conf, eventLoop, () -> new ClientCnx(conf, eventLoop) {
+ @Override
+ protected void handleLookupResponse(CommandLookupTopicResponse lookupResult) {
+ try {
+ clientCnxSemaphore.acquire();
Review Comment:
If the client handled the first request quickly enough, there would never be three concurrent requests.
I think we just need to block the client so that the client doesn't process the response quickly. Maybe CountDownLatch is a better fit, just lick this:
```java
// request 1 :
countDownLatch.await()
// request 2 :
countDownLatch.await()
// request 3 :
countDownLatch.await()
// After countDown is executed, we have built three concurrent requests
countDownLacth.countDown()
```
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -979,7 +981,32 @@ public void testLookupThrottlingForClientByClient() throws Exception {
EventLoopGroup eventLoop = EventLoopUtil.newEventLoopGroup(20, false,
new DefaultThreadFactory("test-pool", Thread.currentThread().isDaemon()));
long reqId = 0xdeadbeef;
- try (ConnectionPool pool = new ConnectionPool(conf, eventLoop)) {
+
+ Semaphore clientCnxSemaphore = new Semaphore(1);
+ AtomicInteger count = new AtomicInteger(2);
Review Comment:
Why do we need this `count` ?
--
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] AnonHxy commented on pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#issuecomment-1181677280
/pulsarbot run-failure-checks
--
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] AnonHxy commented on pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#issuecomment-1210234253
> left a comment:
>
> I feel the origin log "At least one should fail" is better
Updated
--
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] AnonHxy commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r941401934
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -979,7 +981,32 @@ public void testLookupThrottlingForClientByClient() throws Exception {
EventLoopGroup eventLoop = EventLoopUtil.newEventLoopGroup(20, false,
new DefaultThreadFactory("test-pool", Thread.currentThread().isDaemon()));
long reqId = 0xdeadbeef;
- try (ConnectionPool pool = new ConnectionPool(conf, eventLoop)) {
+
+ Semaphore clientCnxSemaphore = new Semaphore(1);
+ AtomicInteger count = new AtomicInteger(2);
+ try (ConnectionPool pool = new ConnectionPool(conf, eventLoop, () -> new ClientCnx(conf, eventLoop) {
+ @Override
+ protected void handleLookupResponse(CommandLookupTopicResponse lookupResult) {
+ try {
+ clientCnxSemaphore.acquire();
Review Comment:
Oh, I get your point now @poorbarcode , thanks for your explantion.
I have thought using `CountDownLatch` here before, But `CountDownLatch` has a problem that it can not reset it's count, which means we can not reuse it with multi cases in this test. So I chosen using `Semaphore` here.
--
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] poorbarcode commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r941408602
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -979,7 +981,32 @@ public void testLookupThrottlingForClientByClient() throws Exception {
EventLoopGroup eventLoop = EventLoopUtil.newEventLoopGroup(20, false,
new DefaultThreadFactory("test-pool", Thread.currentThread().isDaemon()));
long reqId = 0xdeadbeef;
- try (ConnectionPool pool = new ConnectionPool(conf, eventLoop)) {
+
+ Semaphore clientCnxSemaphore = new Semaphore(1);
+ AtomicInteger count = new AtomicInteger(2);
+ try (ConnectionPool pool = new ConnectionPool(conf, eventLoop, () -> new ClientCnx(conf, eventLoop) {
+ @Override
+ protected void handleLookupResponse(CommandLookupTopicResponse lookupResult) {
+ try {
+ clientCnxSemaphore.acquire();
Review Comment:
In current implementation: `semaphore` will always permit the first request, there would never be three concurrent requests. Maybe we can use `await` & `notifyAll` instead `CountDownLatch`
--
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] AnonHxy commented on pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#issuecomment-1210235516
@codelipenghui @Technoboy- PTAL also~
--
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] Technoboy- commented on pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#issuecomment-1181770038
/pulsarbot run-failure-checks
--
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] poorbarcode commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r941980683
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -1056,7 +1105,12 @@ public void testLookupThrottlingForClientByClient() throws Exception {
long reqId10 = reqId++;
ByteBuf request10 = Commands.newLookup(topicName, true, reqId10);
f3 = pool.getConnection(resolver.resolveHost())
- .thenCompose(clientCnx -> clientCnx.newLookup(request10, reqId10));
+ .thenCompose(clientCnx -> {
+ CompletableFuture<?> future = clientCnx.newLookup(request10, reqId10);
+ // pending other responses in `ClientCnx` until now
+ latch[0].countDown();
Review Comment:
Why not code like this (Simpler and better understood):
```java
latch[0] = new CountDownLatch(1);
f1 = pool.getConnection(resolver.resolveHost()).thenCompose(clientCnx -> clientCnx.newLookup(request8,reqId8));
f2 = pool.getConnection(resolver.resolveHost()).thenCompose(clientCnx -> clientCnx.newLookup(request9, reqId9));
f3 = pool.getConnection(resolver.resolveHost()).thenCompose(clientCnx -> clientCnx.newLookup(request9, reqId9));
latch[0].countDown();
f1.get();
f2.get();
f3.get();
```
--
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] poorbarcode commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r942071075
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -1056,7 +1105,12 @@ public void testLookupThrottlingForClientByClient() throws Exception {
long reqId10 = reqId++;
ByteBuf request10 = Commands.newLookup(topicName, true, reqId10);
f3 = pool.getConnection(resolver.resolveHost())
- .thenCompose(clientCnx -> clientCnx.newLookup(request10, reqId10));
+ .thenCompose(clientCnx -> {
+ CompletableFuture<?> future = clientCnx.newLookup(request10, reqId10);
+ // pending other responses in `ClientCnx` until now
+ latch[0].countDown();
Review Comment:
I'm sorry I forgot the `thenCompose`
--
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] AnonHxy commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r941204324
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -979,7 +981,32 @@ public void testLookupThrottlingForClientByClient() throws Exception {
EventLoopGroup eventLoop = EventLoopUtil.newEventLoopGroup(20, false,
new DefaultThreadFactory("test-pool", Thread.currentThread().isDaemon()));
long reqId = 0xdeadbeef;
- try (ConnectionPool pool = new ConnectionPool(conf, eventLoop)) {
+
+ Semaphore clientCnxSemaphore = new Semaphore(1);
+ AtomicInteger count = new AtomicInteger(2);
+ try (ConnectionPool pool = new ConnectionPool(conf, eventLoop, () -> new ClientCnx(conf, eventLoop) {
+ @Override
+ protected void handleLookupResponse(CommandLookupTopicResponse lookupResult) {
+ try {
+ clientCnxSemaphore.acquire();
Review Comment:
But if so, I think there still exists a small chance that all of the three requests can request success, which will make the test fail.
Because they are request concurrently, then one of the requests, e.g., request3, still have the chance that begin sending request after request1 and request2 completed @poorbarcode
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -979,7 +981,32 @@ public void testLookupThrottlingForClientByClient() throws Exception {
EventLoopGroup eventLoop = EventLoopUtil.newEventLoopGroup(20, false,
new DefaultThreadFactory("test-pool", Thread.currentThread().isDaemon()));
long reqId = 0xdeadbeef;
- try (ConnectionPool pool = new ConnectionPool(conf, eventLoop)) {
+
+ Semaphore clientCnxSemaphore = new Semaphore(1);
+ AtomicInteger count = new AtomicInteger(2);
Review Comment:
After I thinking about this, there is no need to add `count` @poorbarcode , I will remove it.
--
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] poorbarcode commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r941986550
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -979,7 +980,31 @@ public void testLookupThrottlingForClientByClient() throws Exception {
EventLoopGroup eventLoop = EventLoopUtil.newEventLoopGroup(20, false,
new DefaultThreadFactory("test-pool", Thread.currentThread().isDaemon()));
long reqId = 0xdeadbeef;
- try (ConnectionPool pool = new ConnectionPool(conf, eventLoop)) {
+
+ // using an array in order to reset a new CountDownLatch
+ CountDownLatch[] latch = new CountDownLatch[1];
Review Comment:
Is there has thread safety issue with multiple threads accessing `CountDownLatch[] latch`? Now the code should be fine, I feel like maybe `AtomicReference` is better
--
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] AnonHxy commented on pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#issuecomment-1182691763
Close this PR because this flaky test has already been fixed
--
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] AnonHxy commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r941972953
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -979,7 +981,32 @@ public void testLookupThrottlingForClientByClient() throws Exception {
EventLoopGroup eventLoop = EventLoopUtil.newEventLoopGroup(20, false,
new DefaultThreadFactory("test-pool", Thread.currentThread().isDaemon()));
long reqId = 0xdeadbeef;
- try (ConnectionPool pool = new ConnectionPool(conf, eventLoop)) {
+
+ Semaphore clientCnxSemaphore = new Semaphore(1);
+ AtomicInteger count = new AtomicInteger(2);
+ try (ConnectionPool pool = new ConnectionPool(conf, eventLoop, () -> new ClientCnx(conf, eventLoop) {
+ @Override
+ protected void handleLookupResponse(CommandLookupTopicResponse lookupResult) {
+ try {
+ clientCnxSemaphore.acquire();
Review Comment:
`await` & `notifyAll` Still have the same problem that is thread can only `await` once.
> CountDownLatch has a problem that it can not reset it's count, which means we can not reuse it with multi cases in this test
Maybe we can used an array `CountDownLatch[]` to reset a new CountDownLatch object. I have updated it, please take a look again @poorbarcode, thanks.
--
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] codelipenghui commented on pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#issuecomment-1208108211
@poorbarcode Could you please review this PR?
--
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] AnonHxy commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r942063830
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -1056,7 +1105,12 @@ public void testLookupThrottlingForClientByClient() throws Exception {
long reqId10 = reqId++;
ByteBuf request10 = Commands.newLookup(topicName, true, reqId10);
f3 = pool.getConnection(resolver.resolveHost())
- .thenCompose(clientCnx -> clientCnx.newLookup(request10, reqId10));
+ .thenCompose(clientCnx -> {
+ CompletableFuture<?> future = clientCnx.newLookup(request10, reqId10);
+ // pending other responses in `ClientCnx` until now
+ latch[0].countDown();
Review Comment:
I think `f1` and `f2` and `f3` could all complete successfully if we do so. The reason is that:
`clientCnx.newLookup` will be called in another thread, so `latch[0].countDown()` could happen before all `newLookup` requeset sending, that will cause the latch has no effect in this test.
In order to make sure `f3.get()` fail, we must block handing response in `clientCnx` until the third `newLookup` have send. @poorbarcode
--
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] poorbarcode commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r941329929
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -979,7 +981,32 @@ public void testLookupThrottlingForClientByClient() throws Exception {
EventLoopGroup eventLoop = EventLoopUtil.newEventLoopGroup(20, false,
new DefaultThreadFactory("test-pool", Thread.currentThread().isDaemon()));
long reqId = 0xdeadbeef;
- try (ConnectionPool pool = new ConnectionPool(conf, eventLoop)) {
+
+ Semaphore clientCnxSemaphore = new Semaphore(1);
+ AtomicInteger count = new AtomicInteger(2);
+ try (ConnectionPool pool = new ConnectionPool(conf, eventLoop, () -> new ClientCnx(conf, eventLoop) {
+ @Override
+ protected void handleLookupResponse(CommandLookupTopicResponse lookupResult) {
+ try {
+ clientCnxSemaphore.acquire();
Review Comment:
> Because they are request concurrently, then one of the requests, e.g., request3, still have the chance that begin sending request after request1 and request2 completed
No, Before we execute `countDownLacth.countDown`, the third request has already failed. see:
https://github.com/apache/pulsar/blob/be57e714c4b9de4e34f159285a44bd935b1d5367/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L790
I think you misunderstood me，Here's what I'm trying to say:
```
// override the clientCnx
static class CountDownLatchCnx extends ClientCnx{
@Override
void handleLookupResponse(){
// countDownLatch.await();
}
}
consumer1.lookup(...)
consumer2.lookup(...)
consumer3.lookup(...) // this request will be fail.
countDownLatch.countDown();
```
--
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] poorbarcode commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r941980683
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -1056,7 +1105,12 @@ public void testLookupThrottlingForClientByClient() throws Exception {
long reqId10 = reqId++;
ByteBuf request10 = Commands.newLookup(topicName, true, reqId10);
f3 = pool.getConnection(resolver.resolveHost())
- .thenCompose(clientCnx -> clientCnx.newLookup(request10, reqId10));
+ .thenCompose(clientCnx -> {
+ CompletableFuture<?> future = clientCnx.newLookup(request10, reqId10);
+ // pending other responses in `ClientCnx` until now
+ latch[0].countDown();
Review Comment:
Why not code like this (Simpler and better understood)？:
```java
latch[0] = new CountDownLatch(1);
f1 = pool.getConnection(resolver.resolveHost()).thenCompose(clientCnx -> clientCnx.newLookup(request8,reqId8));
f2 = pool.getConnection(resolver.resolveHost()).thenCompose(clientCnx -> clientCnx.newLookup(request9, reqId9));
f3 = pool.getConnection(resolver.resolveHost()).thenCompose(clientCnx -> clientCnx.newLookup(request9, reqId9));
latch[0].countDown();
f1.get();
f2.get();
f3.get();
fail("At least one should fail");
```
--
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] AnonHxy commented on pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#issuecomment-1210627406
/pulsarbot run-failure-checks
--
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] AnonHxy commented on pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#issuecomment-1181600627
/pulsarbot run-failure-checks
--
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] AnonHxy closed pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
AnonHxy closed pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
URL: https://github.com/apache/pulsar/pull/16540
--
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] codelipenghui merged pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #16540:
URL: https://github.com/apache/pulsar/pull/16540
--
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] AnonHxy commented on a diff in pull request #16540: [test]Fix Flaky-test: BrokerServiceTest.testLookupThrottlingForClientByClient
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #16540:
URL: https://github.com/apache/pulsar/pull/16540#discussion_r941992883
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java:
##########
@@ -979,7 +980,31 @@ public void testLookupThrottlingForClientByClient() throws Exception {
EventLoopGroup eventLoop = EventLoopUtil.newEventLoopGroup(20, false,
new DefaultThreadFactory("test-pool", Thread.currentThread().isDaemon()));
long reqId = 0xdeadbeef;
- try (ConnectionPool pool = new ConnectionPool(conf, eventLoop)) {
+
+ // using an array in order to reset a new CountDownLatch
+ CountDownLatch[] latch = new CountDownLatch[1];
Review Comment:
`AtomicReference` is better. Have updated. @poorbarcode
--
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