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

yuruguo · 2021-09-08T16:45:57Z

Fixes #11983

Motivation

When I execute pulsar-perf to simulate pub/sub test, I found that the task can be started normally after some options are set to unreasonable values, but there is no instance running, which leads to a waste of resources. In fact, when these unreasonable values appear, we should check to prevent the task from starting.
for example: ./bin/pulsar-perf consume --num-consumers -1 test

Modifications

Add ParameterValidator class
Correct parameter description
Use checkArgument method

Documentation

Need to update docs

eolivelli

Overall it looks good to me

I left one comment

eolivelli · 2021-09-09T19:47:43Z

pulsar-testclient/src/main/java/org/apache/pulsar/testclient/ParameterValidator.java

+        if (Integer.parseInt(value) <= 0) {
+            throw new ParameterException("Parameter " + name + " should be > 0 (found " + value + ")");
+        }
+        return;


Nit: this is not needed

eolivelli · 2021-09-09T19:48:17Z

pulsar-testclient/src/main/java/org/apache/pulsar/testclient/ParameterValidator.java

+import com.beust.jcommander.ParameterException;
+
+public class ParameterValidator implements IParameterValidator {
+


What about naming this like PositiveNumberParameterValidator ?

I agree with it. Have renamed, PTAL.

michaeljmarshall

Looks good. Just one minor edge case that needs to be addressed.

michaeljmarshall · 2021-09-10T15:36:04Z

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

@@ -164,6 +167,7 @@ public ClientBuilder listenerThreads(int numListenerThreads) {

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


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");

~~Done. PTAL again, THX!~~

I have reverted，see review

eolivelli · 2021-09-14T14:42:24Z

pulsar-broker/src/test/java/org/apache/pulsar/client/impl/MessageChecksumTest.java

@@ -69,7 +69,7 @@ protected void cleanup() throws Exception {
    @Override
    protected void customizeNewPulsarClientBuilder(ClientBuilder clientBuilder) {
        // disable connection pooling
-        clientBuilder.connectionsPerBroker(0);
+        // clientBuilder.connectionsPerBroker(0);


please revert

The value of connectionsPerBroker must be greater than 0 (doc in ClientBuilder L266), and this test will fail after the check is added, so a comment is made.

Related review：#11973 (comment)

so what do you suggest ?
modifying a test means that we modify the behaviour and this is not good (most of the times)

This test tells me that 0 is an allowed number, so you have to update your preconditions (and docs?) and let this test pass

I agree with you that 0 is a feasible value for this test. So I update the document and revert the test.

I took a look at how this setting is used, and 0 is a valid setting.

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

Lines 148 to 151 in 4147db8

if (maxConnectionsPerHosts == 0) {

// Disable pooling

return createConnection(logicalAddress, physicalAddress, -1);

}

eolivelli

LGTM

@michaeljmarshall PTAL

michaeljmarshall

I think we should improve the javadoc to better explain what 0 connections per broker means. Otherwise, looks good to me.

michaeljmarshall · 2021-09-15T15:25:56Z

pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ClientBuilder.java

@@ -263,7 +263,7 @@ ClientBuilder authentication(String authPluginClassName, Map<String, String> aut
     * Increasing this parameter may improve throughput when using many producers over a high latency connection.
     *
     * @param connectionsPerBroker
-     *            max number of connections per broker (needs to be greater than 0)
+     *            max number of connections per broker (needs to be not less than 0)


Given the name of this setting (connectionsPerBroker), it isn't immediately clear to me why 0 is a valid setting. I think we should add a note specifying that a value of 0 will disable connection pooling. I think it could also be valuable to update the client configuration file with the same note:

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java

Lines 122 to 126 in b557e24

@ApiModelProperty(

name = "connectionsPerBroker",

value = "Number of connections established between the client and each Broker."

)

private int connectionsPerBroker = 1;

Nit: my preference for this line is to write this as needs to be greater than or equal to 0. Your change is certainly correct, so I'm not sure how much my preference matters, but based on a quick search through the project, we often use greater than or equal to in this situation.

Good suggestion, done. PTAL

315157973

@Anonymitaet Please help review the docs

Anonymitaet · 2021-09-22T03:51:16Z

site2/docs/reference-cli-tools.md

@@ -448,6 +448,7 @@ Options
 |`-ss`, `--subscriptions`|A list of subscriptions to consume on (e.g. sub1,sub2)|sub|
 |`-st`, `--subscription-type`|Subscriber type. Possible values are Exclusive, Shared, Failover, Key_Shared.|Exclusive|
 |`-sp`, `--subscription-position`|Subscriber position. Possible values are Latest, Earliest.|Latest|
+|`-time`, `--test-duration`|Test duration in secs. If <= 0, it will keep consuming|0|


Thanks for your contribution. Does this affect only master or other versioned docs?
If latter, could you please help update all affected versions? Thanks

The document has been checked and updated, PTAL again @Anonymitaet , thx!

This affects only master docs.

[testclient] Improve parameter checking in perf

f001b28

Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Sep 9, 2021

Update doc

55d67e9

eolivelli requested changes Sep 9, 2021

View reviewed changes

yuruguo requested a review from eolivelli September 10, 2021 01:07

michaeljmarshall requested changes Sep 10, 2021

View reviewed changes

eolivelli requested changes Sep 14, 2021

View reviewed changes

yuruguo requested a review from eolivelli September 15, 2021 08:37

eolivelli approved these changes Sep 15, 2021

View reviewed changes

michaeljmarshall requested changes Sep 15, 2021

View reviewed changes

315157973 approved these changes Sep 22, 2021

View reviewed changes

Anonymitaet reviewed Sep 22, 2021

View reviewed changes

Anonymitaet added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-not-needed Your PR changes do not impact docs labels Sep 22, 2021

[testclient] Improve parameter checking in pulsar-perf

6d4aced

yuruguo requested a review from Anonymitaet September 22, 2021 08:12

congbobo184 merged commit 9516e5d into apache:master Sep 23, 2021

Anonymitaet added this to the 2.9.0 milestone Oct 18, 2021

zymap assigned yuruguo Oct 19, 2021

bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022

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

1775009

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yuruguo commented Sep 8, 2021 •

edited

Loading

eolivelli left a comment

eolivelli Sep 9, 2021

eolivelli Sep 9, 2021

yuruguo Sep 10, 2021 •

edited

Loading

michaeljmarshall left a comment

michaeljmarshall Sep 10, 2021

yuruguo Sep 10, 2021 •

edited

Loading

eolivelli Sep 14, 2021

yuruguo Sep 14, 2021 •

edited

Loading

eolivelli Sep 15, 2021

yuruguo Sep 15, 2021

michaeljmarshall Sep 15, 2021

eolivelli left a comment

michaeljmarshall left a comment

michaeljmarshall Sep 15, 2021

yuruguo Sep 18, 2021

315157973 left a comment

Anonymitaet Sep 22, 2021

Anonymitaet Sep 22, 2021

yuruguo Sep 22, 2021 •

edited

Loading

		import com.beust.jcommander.ParameterException;

		public class ParameterValidator implements IParameterValidator {

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

	if (maxConnectionsPerHosts == 0) {
	// Disable pooling
	return createConnection(logicalAddress, physicalAddress, -1);
	}

	@ApiModelProperty(
	name = "connectionsPerBroker",
	value = "Number of connections established between the client and each Broker."
	)
	private int connectionsPerBroker = 1;

	\|`-time`, `--test-duration`\|Test duration in secs. If <= 0, it will keep consuming\|0\|
	\|`-time`, `--test-duration`\|Test duration (in seconds). If this value is less than or equal to 0, it keeps consuming messages.\|0\|

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

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

Conversation

yuruguo commented Sep 8, 2021 • edited Loading

Motivation

Modifications

Documentation

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuruguo Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuruguo Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuruguo Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

315157973 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuruguo Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

yuruguo commented Sep 8, 2021 •

edited

Loading

yuruguo Sep 10, 2021 •

edited

Loading

yuruguo Sep 10, 2021 •

edited

Loading

yuruguo Sep 14, 2021 •

edited

Loading

yuruguo Sep 22, 2021 •

edited

Loading