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

[testclient] Improve parameter checking in pulsar-perf #11973

Merged
merged 3 commits into from
Sep 23, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[testclient] Improve parameter checking in perf
  • Loading branch information
yuruguo committed Sep 8, 2021
commit f001b281865e3f9d84a8757609a52a3d55f9ff36
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
import org.apache.pulsar.client.impl.conf.ClientConfigurationData;
import org.apache.pulsar.client.impl.conf.ConfigurationDataUtils;

import static com.google.common.base.Preconditions.checkArgument;

public class ClientBuilderImpl implements ClientBuilder {
ClientConfigurationData conf;

Expand Down Expand Up @@ -152,6 +154,7 @@ public ClientBuilder lookupTimeout(int lookupTimeout, TimeUnit unit) {

@Override
public ClientBuilder ioThreads(int numIoThreads) {
checkArgument(numIoThreads > 0, "ioThreads needs to be > 0");
conf.setNumIoThreads(numIoThreads);
return this;
}
Expand All @@ -164,6 +167,7 @@ public ClientBuilder listenerThreads(int numListenerThreads) {

@Override
public ClientBuilder connectionsPerBroker(int connectionsPerBroker) {
checkArgument(connectionsPerBroker >= 0, "connectionsPerBroker needs to be >= 0");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the java doc on the ClientBuilder interface, the value must be greater than 0.

Suggested change
checkArgument(connectionsPerBroker >= 0, "connectionsPerBroker needs to be >= 0");
checkArgument(connectionsPerBroker > 0, "connectionsPerBroker needs to be > 0");

Copy link
Contributor Author

@yuruguo yuruguo Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL again, THX!

I have reverted,see review

conf.setConnectionsPerBroker(connectionsPerBroker);
return this;
}
Expand Down Expand Up @@ -242,6 +246,7 @@ public ClientBuilder tlsProtocols(Set<String> tlsProtocols) {

@Override
public ClientBuilder statsInterval(long statsInterval, TimeUnit unit) {
checkArgument(statsInterval >= 0, "statsInterval needs to be >= 0");
conf.setStatsIntervalSeconds(unit.toSeconds(statsInterval));
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,18 +135,21 @@ public ProducerBuilder<T> producerName(String producerName) {

@Override
public ProducerBuilder<T> sendTimeout(int sendTimeout, @NonNull TimeUnit unit) {
checkArgument(sendTimeout >= 0, "sendTimeout needs to be >= 0");
conf.setSendTimeoutMs(sendTimeout, unit);
return this;
}

@Override
public ProducerBuilder<T> maxPendingMessages(int maxPendingMessages) {
checkArgument(maxPendingMessages >= 0, "maxPendingMessages needs to be >= 0");
conf.setMaxPendingMessages(maxPendingMessages);
return this;
}

@Override
public ProducerBuilder<T> maxPendingMessagesAcrossPartitions(int maxPendingMessagesAcrossPartitions) {
checkArgument(maxPendingMessagesAcrossPartitions >= 0, "maxPendingMessagesAcrossPartitions needs to be >= 0");
conf.setMaxPendingMessagesAcrossPartitions(maxPendingMessagesAcrossPartitions);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ public ReaderBuilder<T> cryptoFailureAction(ConsumerCryptoFailureAction action)

@Override
public ReaderBuilder<T> receiverQueueSize(int receiverQueueSize) {
checkArgument(receiverQueueSize >= 0, "receiverQueueSize needs to be >= 0");
conf.setReceiverQueueSize(receiverQueueSize);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import org.apache.pulsar.client.api.AuthenticationDataProvider;
import org.apache.pulsar.client.api.AuthenticationFactory;
import org.apache.pulsar.common.naming.TopicName;
import org.apache.pulsar.testclient.ParameterValidator;
import org.apache.pulsar.testclient.PerfClientUtils;
import org.apache.pulsar.testclient.utils.PaddingDecimalFormat;
import org.slf4j.Logger;
Expand Down Expand Up @@ -90,7 +91,7 @@ static class Arguments {
@Parameter(names = { "-s", "--size" }, description = "Message size in byte")
public int msgSize = 1024;

@Parameter(names = { "-t", "--num-topic" }, description = "Number of topics")
@Parameter(names = { "-t", "--num-topic" }, description = "Number of topics", validateWith = ParameterValidator.class)
public int numTopics = 1;

@Parameter(names = { "--auth_plugin" }, description = "Authentication plugin class name")
Expand All @@ -104,14 +105,14 @@ static class Arguments {
public String authParams;

@Parameter(names = { "-m",
"--num-messages" }, description = "Number of messages to publish in total. If 0, it will keep publishing")
"--num-messages" }, description = "Number of messages to publish in total. If <= 0, it will keep publishing")
public long numMessages = 0;

@Parameter(names = { "-f", "--payload-file" }, description = "Use payload from a file instead of empty buffer")
public String payloadFilename = null;

@Parameter(names = { "-time",
"--test-duration" }, description = "Test duration in secs. If 0, it will keep publishing")
"--test-duration" }, description = "Test duration in secs. If <= 0, it will keep publishing")
public long testTime = 0;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,10 @@ static class Arguments {
@Parameter(names = { "-s", "--size" }, description = "Message size")
public int msgSize = 1024;

@Parameter(names = { "-t", "--num-topic" }, description = "Number of managed ledgers")
@Parameter(names = { "-t", "--num-topic" }, description = "Number of managed ledgers", validateWith = ParameterValidator.class)
public int numManagedLedgers = 1;

@Parameter(names = { "--threads" }, description = "Number of threads writing")
@Parameter(names = { "--threads" }, description = "Number of threads writing", validateWith = ParameterValidator.class)
public int numThreads = 1;

@Parameter(names = { "-zk", "--zookeeperServers" }, description = "ZooKeeper connection string", required = true)
Expand All @@ -110,7 +110,7 @@ static class Arguments {
public int maxConnections = 1;

@Parameter(names = { "-m",
"--num-messages" }, description = "Number of messages to publish in total. If 0, it will keep publishing")
"--num-messages" }, description = "Number of messages to publish in total. If <= 0, it will keep publishing")
public long numMessages = 0;

@Parameter(names = { "-e", "--ensemble-size" }, description = "Ledger ensemble size")
Expand All @@ -126,7 +126,7 @@ static class Arguments {
public DigestType digestType = DigestType.CRC32C;

@Parameter(names = { "-time",
"--test-duration" }, description = "Test duration in secs. If 0, it will keep publishing")
"--test-duration" }, description = "Test duration in secs. If <= 0, it will keep publishing")
public long testTime = 0;

}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.pulsar.testclient;

import com.beust.jcommander.IParameterValidator;
import com.beust.jcommander.ParameterException;

public class ParameterValidator implements IParameterValidator {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about naming this like PositiveNumberParameterValidator ?

Copy link
Contributor Author

@yuruguo yuruguo Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with it. Have renamed, PTAL.

@Override
public void validate(String name, String value) throws ParameterException {
if (Integer.parseInt(value) <= 0) {
throw new ParameterException("Parameter " + name + " should be > 0 (found " + value + ")");
}
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this is not needed

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static org.apache.commons.lang3.StringUtils.isBlank;
import static org.apache.commons.lang3.StringUtils.isNotBlank;

import com.beust.jcommander.IParameterValidator;
import com.beust.jcommander.JCommander;
import com.beust.jcommander.Parameter;
import com.beust.jcommander.ParameterException;
Expand Down Expand Up @@ -82,13 +83,13 @@ static class Arguments {
@Parameter(description = "persistent://prop/ns/my-topic", required = true)
public List<String> topic;

@Parameter(names = { "-t", "--num-topics" }, description = "Number of topics")
@Parameter(names = { "-t", "--num-topics" }, description = "Number of topics", validateWith = ParameterValidator.class)
public int numTopics = 1;

@Parameter(names = { "-n", "--num-consumers" }, description = "Number of consumers (per subscription), only one consumer is allowed when subscriptionType is Exclusive")
@Parameter(names = { "-n", "--num-consumers" }, description = "Number of consumers (per subscription), only one consumer is allowed when subscriptionType is Exclusive", validateWith = ParameterValidator.class)
public int numConsumers = 1;

@Parameter(names = { "-ns", "--num-subscriptions" }, description = "Number of subscriptions (per topic)")
@Parameter(names = { "-ns", "--num-subscriptions" }, description = "Number of subscriptions (per topic)", validateWith = ParameterValidator.class)
public int numSubscriptions = 1;

@Parameter(names = { "-s", "--subscriber-name" }, description = "Subscriber name prefix", hidden = true)
Expand Down Expand Up @@ -166,7 +167,7 @@ static class Arguments {
public String encKeyFile = null;

@Parameter(names = { "-time",
"--test-duration" }, description = "Test duration in secs. If 0, it will keep consuming")
"--test-duration" }, description = "Test duration in secs. If <= 0, it will keep consuming")
public long testTime = 0;

@Parameter(names = {"-ioThreads", "--num-io-threads"}, description = "Set the number of threads to be " +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static org.apache.commons.lang3.StringUtils.isBlank;
import static org.apache.commons.lang3.StringUtils.isNotBlank;

import com.beust.jcommander.IParameterValidator;
import com.beust.jcommander.JCommander;
import com.beust.jcommander.Parameter;
import com.beust.jcommander.ParameterException;
Expand Down Expand Up @@ -108,7 +109,7 @@ static class Arguments {
@Parameter(description = "persistent://prop/ns/my-topic", required = true)
public List<String> topics;

@Parameter(names = { "-threads", "--num-test-threads" }, description = "Number of test threads")
@Parameter(names = { "-threads", "--num-test-threads" }, description = "Number of test threads", validateWith = ParameterValidator.class)
public int numTestThreads = 1;

@Parameter(names = { "-r", "--rate" }, description = "Publish rate msg/s across topics")
Expand All @@ -117,10 +118,10 @@ static class Arguments {
@Parameter(names = { "-s", "--size" }, description = "Message size (bytes)")
public int msgSize = 1024;

@Parameter(names = { "-t", "--num-topic" }, description = "Number of topics")
@Parameter(names = { "-t", "--num-topic" }, description = "Number of topics", validateWith = ParameterValidator.class)
public int numTopics = 1;

@Parameter(names = { "-n", "--num-producers" }, description = "Number of producers (per topic)")
@Parameter(names = { "-n", "--num-producers" }, description = "Number of producers (per topic)", validateWith = ParameterValidator.class)
public int numProducers = 1;

@Parameter(names = {"--separator"}, description = "Separator between the topic and topic number")
Expand Down Expand Up @@ -169,7 +170,7 @@ static class Arguments {
public int maxConnections = 100;

@Parameter(names = { "-m",
"--num-messages" }, description = "Number of messages to publish in total. If 0, it will keep publishing")
"--num-messages" }, description = "Number of messages to publish in total. If <= 0, it will keep publishing")
public long numMessages = 0;

@Parameter(names = { "-i",
Expand Down Expand Up @@ -201,7 +202,7 @@ static class Arguments {
public int batchMaxBytes = 4 * 1024 * 1024;

@Parameter(names = { "-time",
"--test-duration" }, description = "Test duration in secs. If 0, it will keep publishing")
"--test-duration" }, description = "Test duration in secs. If <= 0, it will keep publishing")
public long testTime = 0;

@Parameter(names = "--warmup-time", description = "Warm-up time in seconds (Default: 1 sec)")
Expand Down Expand Up @@ -538,7 +539,7 @@ private static void runProducer(int producerId,
producerBuilder.producerName(producerName);
}

if (arguments.batchTimeMillis == 0.0 && arguments.batchMaxMessages == 0) {
if (arguments.batchTimeMillis <= 0.0 && arguments.batchMaxMessages <= 0) {
producerBuilder.enableBatching(false);
} else {
long batchTimeUsec = (long) (arguments.batchTimeMillis * 1000);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ static class Arguments {
@Parameter(description = "persistent://prop/ns/my-topic", required = true)
public List<String> topic;

@Parameter(names = { "-t", "--num-topics" }, description = "Number of topics")
@Parameter(names = { "-t", "--num-topics" }, description = "Number of topics", validateWith = ParameterValidator.class)
public int numTopics = 1;

@Parameter(names = { "-r", "--rate" }, description = "Simulate a slow message reader (rate in msg/s)")
Expand Down Expand Up @@ -122,7 +122,7 @@ static class Arguments {
public Boolean tlsAllowInsecureConnection = null;

@Parameter(names = { "-time",
"--test-duration" }, description = "Test duration in secs. If 0, it will keep consuming")
"--test-duration" }, description = "Test duration in secs. If <= 0, it will keep consuming")
public long testTime = 0;

@Parameter(names = {"-ioThreads", "--num-io-threads"}, description = "Set the number of threads to be " +
Expand Down