-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[cli] Enable CLI to publish non-batched messages #12641
Conversation
@@ -107,6 +107,9 @@ | |||
description = "Rate (in msg/sec) at which to produce," + | |||
" value 0 means to produce messages as fast as possible.") | |||
private double publishRate = 0; | |||
|
|||
@Parameter(names = { "-db", "--disable-batching" }, description = "Disable batch sending of messages") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It better to named --batch-enabled
? and the default is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally, boolean type options do not require a value, so the default value should be false.
# By default batching is enabled
$ ./bin/pulsar-client produce -m hello persistent://public/default/t1
# Batching is enabled in this case as well
$ ./bin/pulsar-client produce -m hello --batch-enabled persistent://public/default/t1
We can set the default value to true by specifying arity = 1
, but it may be a bit verbose.
https://jcommander.org/#_boolean
# Batching is enabled
$ ./bin/pulsar-client produce -m hello --batch-enabled true persistent://public/default/t1
# Batching is disabled
$ ./bin/pulsar-client produce -m hello --batch-enabled false persistent://public/default/t1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that makes sense.
How about keeping the same configurations with the perf producer?
@Parameter(names = { "-b",
"--batch-time-window" }, description = "Batch messages in 'x' ms window (Default: 1ms)")
public double batchTimeMillis = 1.0;
@Parameter(names = {
"-bm", "--batch-max-messages"
}, description = "Maximum number of messages per batch")
public int batchMaxMessages = DEFAULT_BATCHING_MAX_MESSAGES;
@Parameter(names = {
"-bb", "--batch-max-bytes"
}, description = "Maximum number of bytes per batch")
public int batchMaxBytes = 4 * 1024 * 1024
if (arguments.batchTimeMillis <= 0.0 && arguments.batchMaxMessages <= 0) {
producerBuilder.enableBatching(false);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the pulsar-client
command, one batch message always contains only one message, even if batching is enabled. This is because we are using the synchronous method send()
instead of the asynchronous method sendAsync()
.
pulsar/pulsar-client-tools/src/main/java/org/apache/pulsar/client/cli/CmdProduce.java
Line 297 in 3c770a1
message.send(); |
So, unlike the perf producer, I don't think such fine-grained settings are useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no particular benefit to enabling batching for the pulsar-client
command, so we have the option of always disabling it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that make sense.
Assert.assertTrue(msg.getMessageId() instanceof BatchMessageIdImpl); | ||
} else { | ||
Assert.assertEquals(new String(msg.getData()), "non-batched"); | ||
Assert.assertFalse(msg.getMessageId() instanceof BatchMessageIdImpl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that you have not checked if the message is from a batch message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this check not enough? Is there any other good way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, my mistake. It's good.
Assert.assertTrue(msg.getMessageId() instanceof BatchMessageIdImpl); | ||
} else { | ||
Assert.assertEquals(new String(msg.getData()), "non-batched"); | ||
Assert.assertFalse(msg.getMessageId() instanceof BatchMessageIdImpl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, my mistake. It's good.
* up/master: [Doc] Add explanations for setting geo-replication at topic level (apache#12633) commit chapter Tiered Storage (apache#12592) [pulsar-admin] Add remove-subscription-types-enabled command for namespace (apache#12392) Enable CLI to publish non-batched messages (apache#12641) [Doc] Add doc for tokenSettingPrefix (apache#12662) [pulsar-admin] Add corresponding get command for namespace (apache#12322) [pulsar-admin] Perfect judgment conditions of pulsar-admin (apache#12315) [broker] Avoid unnecessary recalculation of maxSubscriptionsPerTopic in AbstractTopic (apache#12658) [Transaction]Stop TB recovering with exception (apache#12636) [website][upgrade]feat: docs migration - 2.7.1 / client (apache#12612) [website][upgrade]feat: docs migration - 2.7.1 / performance (apache#12611) [website][upgrade]feat: docs migration - 2.7.1 / security (apache#12610) [Modernizer] Apply Modernizer plugin for pulsar broker common module and fix violation. (apache#12657) [Authorization] Support GET_METADATA topic op after enable auth (apache#12656) Fix StringIndexOutOfBoundsException in org.apache.pulsar.broker.resources.NamespaceResources#pathIsFromNamespace (apache#12659)
* up/master: [Doc] Add explanations for setting geo-replication at topic level (apache#12633) commit chapter Tiered Storage (apache#12592) [pulsar-admin] Add remove-subscription-types-enabled command for namespace (apache#12392) Enable CLI to publish non-batched messages (apache#12641) [Doc] Add doc for tokenSettingPrefix (apache#12662) [pulsar-admin] Add corresponding get command for namespace (apache#12322) [pulsar-admin] Perfect judgment conditions of pulsar-admin (apache#12315) [broker] Avoid unnecessary recalculation of maxSubscriptionsPerTopic in AbstractTopic (apache#12658) [Transaction]Stop TB recovering with exception (apache#12636) [website][upgrade]feat: docs migration - 2.7.1 / client (apache#12612) [website][upgrade]feat: docs migration - 2.7.1 / performance (apache#12611) [website][upgrade]feat: docs migration - 2.7.1 / security (apache#12610) [Modernizer] Apply Modernizer plugin for pulsar broker common module and fix violation. (apache#12657) [Authorization] Support GET_METADATA topic op after enable auth (apache#12656) Fix StringIndexOutOfBoundsException in org.apache.pulsar.broker.resources.NamespaceResources#pathIsFromNamespace (apache#12659) # Conflicts: # site2/website-next/versioned_sidebars/version-2.7.2-sidebars.json
### Motivation Currently, messages produced by the `pulsar-client` command are always batched. However, zero queue consumers cannot receive these batched messages. I think it would be useful to be able to easily produce non-batched messages. ### Modifications Added an option to disable batching to the `pulsar-client` command: ```sh $ ./bin/pulsar-client produce -m hello -n 10 --disable-batching persistent://public/default/t1 ``` (cherry picked from commit a1bad71)
### Motivation Currently, messages produced by the `pulsar-client` command are always batched. However, zero queue consumers cannot receive these batched messages. I think it would be useful to be able to easily produce non-batched messages. ### Modifications Added an option to disable batching to the `pulsar-client` command: ```sh $ ./bin/pulsar-client produce -m hello -n 10 --disable-batching persistent://public/default/t1 ```
### Motivation Currently, messages produced by the `pulsar-client` command are always batched. However, zero queue consumers cannot receive these batched messages. I think it would be useful to be able to easily produce non-batched messages. ### Modifications Added an option to disable batching to the `pulsar-client` command: ```sh $ ./bin/pulsar-client produce -m hello -n 10 --disable-batching persistent://public/default/t1 ``` (cherry picked from commit a1bad71)
Motivation
Currently, messages produced by the
pulsar-client
command are always batched. However, zero queue consumers cannot receive these batched messages. I think it would be useful to be able to easily produce non-batched messages.Modifications
Added an option to disable batching to the
pulsar-client
command:Verifying this change
Does this pull request potentially affect one of the following parts:
Documentation
doc