Make Consumer thread safe and lock-free #10352

315157973 · 2021-04-24T16:40:44Z

Motivation

Lock-free solution for #10240

eolivelli

This way we are limiting the number of concurrent reads on the client, by the size of the executor.

315157973 · 2021-04-25T11:27:06Z

The performance test results are in two folders 2.6.1 and 2.8.0
https://github.com/315157973/openmessaging-benchmark/tree/executor/bin
Now the dependency in pom is 2.8.0, if you want to switch to 2.6.1, you only need to change the dependency to 2.6.1 version of pulsar-client-all.

Test command: bin/benchmark --drivers driver-pulsar/pulsar.yaml workloads/max-rate-1-topic-20-partitions-20p-20c-1kb.yaml

5 IO Thread
It seems that the performance has been improved by 50%, which is similar to the previous test results

client 2.6.1 :

client:2.8.0-snapshot (use executor)

315157973 · 2021-04-26T08:43:11Z

Wow, all unit tests are done at once

315157973 · 2021-04-26T08:51:47Z

I will re-trigger CI several times to see if there will be some occasional problems

315157973 · 2021-04-27T04:36:51Z

In the case of adding lock, there are still the following flaky tests
https://github.com/apache/pulsar/pull/10240/checks?check_run_id=2435235663#step:9:783

In the case of adding thread pool
https://github.com/apache/pulsar/pull/10352/checks?check_run_id=2440197210#step:9:782

Therefore, the performance of adding a thread pool and adding a lock is the same. I haven't seen other flaky errors when using the thread pool.

pulsar-client/src/test/java/org/apache/pulsar/client/impl/ClientTestFixtures.java

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java

# Conflicts: # pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java

315157973 · 2021-04-28T13:22:16Z

@merlimat @sijie @rdhabalia @codelipenghui @eolivelli PTAL, thanks

eolivelli

Overall is good to me.
I left a comment.

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java

eolivelli

LGTM

MarvinCai · 2021-04-30T11:37:30Z

nice

linlinnn · 2021-05-04T03:32:19Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java

-                }
-                singleMessagePayload.release();
+                    singleMessagePayload.release();
+                    tryTriggerListener();


Overall is good, just left a comment.
I think we don't need move tryTriggerListener into cycle, because we cannot use Listener while calling receive or receiveAsync.

The behavior here is consistent, the previous logic is

if(){ // do some thing }else{ // do some thing } triggerListener

I just put the triggerListener into the if and else respectively.

If I don’t do this, triggerListener will execute in parallel with the if-else logic

You put triggerListener into the cycle
Maybe have a bad influence when the batchSize is big
we enqueue all message and trigger once before, now we enqueue one message and trigger one time.

What about just

if(){ // do some thing }else{ // do some thing } internalPinnedExecutor.execute(() -> { triggerListener(); });

eolivelli · 2021-05-04T06:47:34Z

waiting for @linlinnn approval before merging

linlinnn

LGTM, thanks.

eolivelli · 2021-05-04T10:33:11Z

thank you @linlinnn for your comment.

let's wait for CI to complete

### Motivation Lock-free solution for apache#10240

Lock-free solution for #10240 (cherry picked from commit def1932)

Lock-free solution for apache#10240 (cherry picked from commit def1932)

…ving pending batch receives requests ### Motivation The consumer will apply the default batch receive policy even if the user will not use the batch receive API. https://github.com/apache/pulsar/blob/6704f12104219611164aa2bb5bbdfc929613f1bf/pulsar-client-api/src/main/java/org/apache/pulsar/client/api/BatchReceivePolicy.java#L60-L61 This will consume lots of CPU if the client have many consumers (100k consumers) The Pulsar perf tool can also reproduce the problem if run the test with many consumers ### Modification If there is no pending batch receive operation for a consumer, no need to trigger the batch timeout task periodically. We can only start the timeout check after adding batch receive request to pending request queue. Remove the lock in MultiTopicsConsumerImpl as apache#10352 does ### Verification Added new test to verify the batch receive timeout task will not start if no batch receive request

…ving pending batch receives requests (#16160) ### Motivation The consumer will apply the default batch receive policy even if the user will not use the batch receive API. https://github.com/apache/pulsar/blob/6704f12104219611164aa2bb5bbdfc929613f1bf/pulsar-client-api/src/main/java/org/apache/pulsar/client/api/BatchReceivePolicy.java#L60-L61 This will consume lots of CPU if the client have many consumers (100k consumers) The Pulsar perf tool can also reproduce the problem if run the test with many consumers ### Modification If there is no pending batch receive operation for a consumer, no need to trigger the batch timeout task periodically. We can only start the timeout check after adding batch receive request to pending request queue. Remove the lock in MultiTopicsConsumerImpl as #10352 does ### Verification Added new test to verify the batch receive timeout task will not start if no batch receive request

…ving pending batch receives requests (#16160) The consumer will apply the default batch receive policy even if the user will not use the batch receive API. https://github.com/apache/pulsar/blob/6704f12104219611164aa2bb5bbdfc929613f1bf/pulsar-client-api/src/main/java/org/apache/pulsar/client/api/BatchReceivePolicy.java#L60-L61 This will consume lots of CPU if the client have many consumers (100k consumers) The Pulsar perf tool can also reproduce the problem if run the test with many consumers If there is no pending batch receive operation for a consumer, no need to trigger the batch timeout task periodically. We can only start the timeout check after adding batch receive request to pending request queue. Remove the lock in MultiTopicsConsumerImpl as #10352 does Added new test to verify the batch receive timeout task will not start if no batch receive request (cherry picked from commit a0ccdc9)

…ving pending batch receives requests (apache#16160) The consumer will apply the default batch receive policy even if the user will not use the batch receive API. https://github.com/apache/pulsar/blob/6704f12104219611164aa2bb5bbdfc929613f1bf/pulsar-client-api/src/main/java/org/apache/pulsar/client/api/BatchReceivePolicy.java#L60-L61 This will consume lots of CPU if the client have many consumers (100k consumers) The Pulsar perf tool can also reproduce the problem if run the test with many consumers If there is no pending batch receive operation for a consumer, no need to trigger the batch timeout task periodically. We can only start the timeout check after adding batch receive request to pending request queue. Remove the lock in MultiTopicsConsumerImpl as apache#10352 does Added new test to verify the batch receive timeout task will not start if no batch receive request (cherry picked from commit a0ccdc9) (cherry picked from commit 6ed4ed0)

…ving pending batch receives requests (#16160) The consumer will apply the default batch receive policy even if the user will not use the batch receive API. https://github.com/apache/pulsar/blob/6704f12104219611164aa2bb5bbdfc929613f1bf/pulsar-client-api/src/main/java/org/apache/pulsar/client/api/BatchReceivePolicy.java#L60-L61 This will consume lots of CPU if the client have many consumers (100k consumers) The Pulsar perf tool can also reproduce the problem if run the test with many consumers If there is no pending batch receive operation for a consumer, no need to trigger the batch timeout task periodically. We can only start the timeout check after adding batch receive request to pending request queue. Remove the lock in MultiTopicsConsumerImpl as #10352 does Added new test to verify the batch receive timeout task will not start if no batch receive request (cherry picked from commit a0ccdc9)

lock-free

a5d8a16

315157973 mentioned this pull request Apr 24, 2021

[consumer] Revert "Remove consumer unnecessary locks (#9261)" #10240

Merged

eolivelli reviewed Apr 24, 2021

View reviewed changes

315157973 added 6 commits April 25, 2021 19:51

Change each locked place to use thread pool

335522d

Fix unit test

d9c7e8f

remove test

efa0fd4

fix unit test

cb8fd20

fix unit test

9e0719d

Merge remote-tracking branch 'upstream/master' into lock-free

08852ec

315157973 added 2 commits April 27, 2021 00:14

Trigger CI to see if there are occasional problems

87355ee

Trigger CI to see if there are occasional problems

9a1324f

eolivelli reviewed Apr 27, 2021

View reviewed changes

pulsar-client/src/test/java/org/apache/pulsar/client/impl/ClientTestFixtures.java Outdated Show resolved Hide resolved

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java Show resolved Hide resolved

315157973 added 3 commits April 27, 2021 15:08

Shutdown executor

e797a03

Merge branch 'master' into lock-free

3d22d4c

# Conflicts: # pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java

Trigger CI to see if there are occasional problems

2b8bb8e

315157973 marked this pull request as ready for review April 28, 2021 13:13

eolivelli requested changes Apr 28, 2021

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java Show resolved Hide resolved

Apply suggestion

c1517ff

eolivelli approved these changes Apr 29, 2021

View reviewed changes

sijie approved these changes May 4, 2021

View reviewed changes

sijie requested review from codelipenghui and merlimat May 4, 2021 02:49

sijie assigned 315157973 May 4, 2021

sijie added the area/client label May 4, 2021

sijie added this to the 2.8.0 milestone May 4, 2021

linlinnn reviewed May 4, 2021

View reviewed changes

move triggerListener

9773d7f

linlinnn approved these changes May 4, 2021

View reviewed changes

sijie merged commit def1932 into apache:master May 5, 2021

lhotari mentioned this pull request May 10, 2021

lh use conscrypt for jetty lhotari/pulsar#39

Closed

315157973 deleted the lock-free branch May 11, 2021 09:02

hangc0276 mentioned this pull request May 11, 2021

[BUG] Pulsar consumer may skip messages streamnative/kop#463

Closed

eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request May 11, 2021

Make Consumer thread safe and lock-free (apache#10352)

f8f10ec

### Motivation Lock-free solution for apache#10240

codelipenghui pushed a commit that referenced this pull request Jul 1, 2021

Make Consumer thread safe and lock-free (#10352)

2720863

Lock-free solution for #10240 (cherry picked from commit def1932)

dlg99 pushed a commit to dlg99/pulsar that referenced this pull request Dec 2, 2021

Make Consumer thread safe and lock-free (apache#10352)

0e599b0

Lock-free solution for apache#10240 (cherry picked from commit def1932)

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Dec 2, 2021

Make Consumer thread safe and lock-free (apache#10352)

0b3172e

Lock-free solution for apache#10240 (cherry picked from commit def1932)

codelipenghui mentioned this pull request Jun 21, 2022

[improve][java-client] Only trigger the batch receive timeout when having pending batch receives requests #16160

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Consumer thread safe and lock-free #10352

Make Consumer thread safe and lock-free #10352

315157973 commented Apr 24, 2021

eolivelli left a comment

315157973 commented Apr 25, 2021 •

edited

Loading

315157973 commented Apr 26, 2021

315157973 commented Apr 26, 2021

315157973 commented Apr 27, 2021

315157973 commented Apr 28, 2021

eolivelli left a comment

eolivelli left a comment

MarvinCai commented Apr 30, 2021

linlinnn May 4, 2021 •

edited

Loading

315157973 May 4, 2021 •

edited

Loading

linlinnn May 4, 2021 •

edited

Loading

eolivelli commented May 4, 2021

linlinnn left a comment

eolivelli commented May 4, 2021

Make Consumer thread safe and lock-free #10352

Make Consumer thread safe and lock-free #10352

Conversation

315157973 commented Apr 24, 2021

Motivation

eolivelli left a comment

Choose a reason for hiding this comment

315157973 commented Apr 25, 2021 • edited Loading

315157973 commented Apr 26, 2021

315157973 commented Apr 26, 2021

315157973 commented Apr 27, 2021

315157973 commented Apr 28, 2021

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

MarvinCai commented Apr 30, 2021

linlinnn May 4, 2021 • edited Loading

Choose a reason for hiding this comment

315157973 May 4, 2021 • edited Loading

Choose a reason for hiding this comment

linlinnn May 4, 2021 • edited Loading

Choose a reason for hiding this comment

eolivelli commented May 4, 2021

linlinnn left a comment

Choose a reason for hiding this comment

eolivelli commented May 4, 2021

315157973 commented Apr 25, 2021 •

edited

Loading

linlinnn May 4, 2021 •

edited

Loading

315157973 May 4, 2021 •

edited

Loading

linlinnn May 4, 2021 •

edited

Loading