Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][tools] Only apply maxPendingMessagesAcrossPartitions if it presents #15283

Merged

Conversation

codelipenghui
Copy link
Contributor

Motivation

After #13344, we have changed the default value of maxPendingMessagesAcrossPartitions to 0,
for the performance producer, it will always apply the default value of maxPendingMessagesAcrossPartitions
while creating producers, so it will always use 0 for maxPendingMessagesAcrossPartitions.

But we have a check here

public void setMaxPendingMessagesAcrossPartitions(int maxPendingMessagesAcrossPartitions) {
checkArgument(maxPendingMessagesAcrossPartitions >= maxPendingMessages,
"maxPendingMessagesAcrossPartitions needs to be >= maxPendingMessages");
this.maxPendingMessagesAcrossPartitions = maxPendingMessagesAcrossPartitions;
}

This will lead the performance producer not able to create if users only set -o option

06:32:49.628 [pulsar-perf-producer-exec-1-1] ERROR org.apache.pulsar.testclient.PerformanceProducer - Got error
    java.lang.IllegalArgumentException: maxPendingMessagesAcrossPartitions needs to be >= maxPendingMessages
    	at org.apache.pulsar.shade.com.google.common.base.Preconditions.checkArgument(Preconditions.java:145) ~[pulsar-client-admin-2.10.0.2.jar:2.10.0.2]
    	at org.apache.pulsar.client.impl.conf.ProducerConfigurationData.setMaxPendingMessagesAcrossPartitions(ProducerConfigurationData.java:138) ~[pulsar-client-admin-2.10.0.2.jar:2.10.0.2]
    	at org.apache.pulsar.client.impl.ProducerBuilderImpl.maxPendingMessagesAcrossPartitions(ProducerBuilderImpl.java:148) ~[pulsar-client-admin-2.10.0.2.jar:2.10.0.2]
    	at org.apache.pulsar.testclient.PerformanceProducer.runProducer(PerformanceProducer.java:600) ~[pulsar-testclient-2.10.0.2.jar:2.10.0.2]
    	at org.apache.pulsar.testclient.PerformanceProducer.lambda$main$1(PerformanceProducer.java:464) ~[pulsar-testclient-2.10.0.2.jar:2.10.0.2]
    	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) [?:1.8.0_332]
    	at java.util.concurrent.FutureTask.run(FutureTask.java:266) [?:1.8.0_332]
    	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_332]
    	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_332]
    	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [netty-common-4.1.74.Final.jar:4.1.74.Final]
    	at java.lang.Thread.run(Thread.java:750) [?:1.8.0_332]

Modification

Change the performance producer to only call .maxPendingMessagesAcrossPartitions() if it presents.

Verification

Added a new test to only use -o to start the performance producer, use a consumer to consume data from
the topic to make sure the performance producer has been created successfully.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

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)

  • no-need-doc
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-added
    (Docs have been already added)

…ents

### Motivation

After apache#13344, we have changed the default value of maxPendingMessagesAcrossPartitions to `0`,
for the performance producer, it will always apply the default value of maxPendingMessagesAcrossPartitions
while creating producers, so it will always use `0` for maxPendingMessagesAcrossPartitions.

But we have a check here https://github.com/apache/pulsar/blob/0a91196dcc4d31ae647867ed319b8c1af0cb93c6/pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ProducerConfigurationData.java#L138-L142

This will lead the performance producer not able to create if users only set `-o` option

```
06:32:49.628 [pulsar-perf-producer-exec-1-1] ERROR org.apache.pulsar.testclient.PerformanceProducer - Got error
    java.lang.IllegalArgumentException: maxPendingMessagesAcrossPartitions needs to be >= maxPendingMessages
    	at org.apache.pulsar.shade.com.google.common.base.Preconditions.checkArgument(Preconditions.java:145) ~[pulsar-client-admin-2.10.0.2.jar:2.10.0.2]
    	at org.apache.pulsar.client.impl.conf.ProducerConfigurationData.setMaxPendingMessagesAcrossPartitions(ProducerConfigurationData.java:138) ~[pulsar-client-admin-2.10.0.2.jar:2.10.0.2]
    	at org.apache.pulsar.client.impl.ProducerBuilderImpl.maxPendingMessagesAcrossPartitions(ProducerBuilderImpl.java:148) ~[pulsar-client-admin-2.10.0.2.jar:2.10.0.2]
    	at org.apache.pulsar.testclient.PerformanceProducer.runProducer(PerformanceProducer.java:600) ~[pulsar-testclient-2.10.0.2.jar:2.10.0.2]
    	at org.apache.pulsar.testclient.PerformanceProducer.lambda$main$1(PerformanceProducer.java:464) ~[pulsar-testclient-2.10.0.2.jar:2.10.0.2]
    	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) [?:1.8.0_332]
    	at java.util.concurrent.FutureTask.run(FutureTask.java:266) [?:1.8.0_332]
    	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_332]
    	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_332]
    	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [netty-common-4.1.74.Final.jar:4.1.74.Final]
    	at java.lang.Thread.run(Thread.java:750) [?:1.8.0_332]
```

### Modification

Change the performance producer to only call `.maxPendingMessagesAcrossPartitions()` if it presents.

### Verification

Added new test to only use `-o` to start the performance producer, use a consumer to consume data from
the topic to make sure the performance producer has been created successfully.
@codelipenghui codelipenghui self-assigned this Apr 24, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 24, 2022
@codelipenghui codelipenghui added type/bug The PR fixed a bug or issue reported a bug area/tool release/2.10.1 and removed doc-not-needed Your PR changes do not impact docs labels Apr 24, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Apr 24, 2022
@github-actions
Copy link

@codelipenghui:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

1 similar comment
@github-actions
Copy link

@codelipenghui:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@RobertIndie RobertIndie added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Apr 24, 2022
@Technoboy- Technoboy- merged commit 188d4f4 into apache:master Apr 24, 2022
@codelipenghui codelipenghui deleted the penghui/fix-perf-maxOutstanding branch April 24, 2022 14:36
codelipenghui added a commit that referenced this pull request Apr 28, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 9, 2022
…ents (apache#15283)

(cherry picked from commit 188d4f4)
(cherry picked from commit bdb6203)
@codelipenghui codelipenghui restored the penghui/fix-perf-maxOutstanding branch May 17, 2022 01:20
@codelipenghui codelipenghui deleted the penghui/fix-perf-maxOutstanding branch May 17, 2022 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tool cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.10.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants