pulsar-perf: add ability to create partitioned topics #9859

eolivelli · 2021-03-10T10:29:09Z

Motivation

When you are using pulsar-perf you have to pre-create the topic if you want it to be partitioned.
This is very awkward and error prone.

Modifications

This patch adds the ability to set a number of partitions.
Before starting the main procedure we are going to create the topics as partitioned topics.
I needed to add support to pass explicitly the http service url, because by default the serverURL is the binary protocol URL and it is not suitable for PulsarAdmin. The default value is http://localhost:8080.

I added support for not calling System.exit and execute a custom callback, otherwise it is not possible to execute reliably the tests without crashing the JVM.

Verifying this change

This change added tests and can be verified bu testing manually the new option.

Documentation

Does this pull request introduce a new feature? yes
If yes, how is the feature documented? internal help for pulsar-perf (just run "pulsar-perf produce" in order to see the help)

eolivelli · 2021-03-10T13:21:43Z

/pulsarbot rerun-failure-checks

codelipenghui · 2021-03-10T13:42:09Z

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

+                    try {
+                        client.topics().createPartitionedTopic(topic, arguments.createTopicPartitions);
+                    } catch (PulsarAdminException.ConflictException alreadyExists) {
+                        log.debug("Topic "+topic+" already exists: " + alreadyExists);


If the topic already exists, we should check the partitions is equals to the user settings, otherwise, we might get the incorrect test result. I think we should stop the test client in this situdation.

good idea, I will add this validation

@codelipenghui I added the validation, PTAL
thanks for your quick review

log.debug should be put in if (log.isDebugEnabled())

@sijie done

315157973

Nice work! It is better to add a unit test to cover the situation where Topic already exists

eolivelli · 2021-03-10T14:47:22Z

@315157973 I cannot do it easily/not in this PR.
because these command line tools call "System.exit" on failure and it will abort the JVM.

In order to make these tools "testable" we should run them in another way and add a way to not call "System.exit"
I have created this issue
#9865

I will be happy to follow up with an implementation for #9865

but I believe it is better to keep this PR focused on the task

eolivelli · 2021-03-10T15:44:37Z

/pulsarbot rerun-failure-checks

eolivelli · 2021-03-10T16:02:37Z

/pulsarbot rerun-failure-checks

eolivelli · 2021-03-11T11:25:55Z

/pulsarbot rerun-failure-checks

eolivelli · 2021-03-11T14:28:13Z

/pulsarbot rerun-failure-checks

eolivelli · 2021-03-12T12:52:57Z

/pulsarbot rerun-failure-checks

eolivelli · 2021-03-12T14:34:45Z

/pulsarbot rerun-failure-checks

sijie · 2021-03-12T23:56:52Z

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

@@ -140,6 +148,9 @@
        @Parameter(names = { "-p", "--max-outstanding-across-partitions" }, description = "Max number of outstanding messages across partitions")
        public int maxPendingMessagesAcrossPartitions = DEFAULT_MAX_PENDING_MESSAGES_ACROSS_PARTITIONS;

+        @Parameter(names = { "-np", "--partitions" }, description = "Create partitioned topics with the given number of partitions, set 0 to not try to create the topic")
+        public int createTopicPartitions = 0;


Suggested change

public int createTopicPartitions = 0;

public int partitions = 0;

sijie · 2021-03-12T23:57:45Z

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

@@ -140,6 +148,9 @@
        @Parameter(names = { "-p", "--max-outstanding-across-partitions" }, description = "Max number of outstanding messages across partitions")
        public int maxPendingMessagesAcrossPartitions = DEFAULT_MAX_PENDING_MESSAGES_ACROSS_PARTITIONS;

+        @Parameter(names = { "-np", "--partitions" }, description = "Create partitioned topics with the given number of partitions, set 0 to not try to create the topic")
+        public int createTopicPartitions = 0;


Suggested change

public int createTopicPartitions = 0;

public Integer createTopicPartitions = null;

Please use boxed integer to indicate if it is going to create a partitioned topic or not.

good idea. done

sijie · 2021-03-12T23:58:27Z

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

+                    try {
+                        client.topics().createPartitionedTopic(topic, arguments.createTopicPartitions);
+                    } catch (PulsarAdminException.ConflictException alreadyExists) {
+                        log.debug("Topic "+topic+" already exists: " + alreadyExists);


log.debug should be put in if (log.isDebugEnabled())

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

eolivelli

@sijie I have addressed all of your comments and answered to the System.exit question.
Thanks

eolivelli · 2021-03-15T08:21:53Z

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

+                    try {
+                        client.topics().createPartitionedTopic(topic, arguments.createTopicPartitions);
+                    } catch (PulsarAdminException.ConflictException alreadyExists) {
+                        log.debug("Topic "+topic+" already exists: " + alreadyExists);


@sijie done

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

eolivelli · 2021-03-15T08:22:48Z

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

@@ -140,6 +148,9 @@
        @Parameter(names = { "-p", "--max-outstanding-across-partitions" }, description = "Max number of outstanding messages across partitions")
        public int maxPendingMessagesAcrossPartitions = DEFAULT_MAX_PENDING_MESSAGES_ACROSS_PARTITIONS;

+        @Parameter(names = { "-np", "--partitions" }, description = "Create partitioned topics with the given number of partitions, set 0 to not try to create the topic")
+        public int createTopicPartitions = 0;


good idea. done

eolivelli · 2021-03-15T08:22:55Z

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

@@ -140,6 +148,9 @@
        @Parameter(names = { "-p", "--max-outstanding-across-partitions" }, description = "Max number of outstanding messages across partitions")
        public int maxPendingMessagesAcrossPartitions = DEFAULT_MAX_PENDING_MESSAGES_ACROSS_PARTITIONS;

+        @Parameter(names = { "-np", "--partitions" }, description = "Create partitioned topics with the given number of partitions, set 0 to not try to create the topic")
+        public int createTopicPartitions = 0;


eolivelli · 2021-03-15T10:55:19Z

/pulsarbot rerun-failure-checks

codelipenghui reviewed Mar 10, 2021

View reviewed changes

codelipenghui assigned eolivelli Mar 10, 2021

codelipenghui requested review from 315157973, hangc0276, jiazhai, sijie, zymap and merlimat March 10, 2021 13:42

codelipenghui added this to the 2.8.0 milestone Mar 10, 2021

codelipenghui approved these changes Mar 10, 2021

View reviewed changes

315157973 approved these changes Mar 10, 2021

View reviewed changes

sijie requested changes Mar 12, 2021

View reviewed changes

eolivelli force-pushed the fix/pulsar-perf-partitions branch from 354d89b to b68fb62 Compare March 15, 2021 08:14

eolivelli commented Mar 15, 2021

View reviewed changes

eolivelli force-pushed the fix/pulsar-perf-partitions branch from 3b2b4a2 to 6c53047 Compare March 18, 2021 08:53

eolivelli added 4 commits March 22, 2021 12:02

Support creating partitioned topics with pulsar-perf produce

b149efb

pulsar-perf: add ability to create partitioned topics

6a4d44b

add validation about the number of partitions

0c197f7

Address Sijie's comments

c932361

eolivelli force-pushed the fix/pulsar-perf-partitions branch from 6c53047 to c932361 Compare March 22, 2021 11:02

Add handler for System.exit

6215854

eolivelli requested a review from sijie March 22, 2021 13:57

sijie approved these changes Mar 23, 2021

View reviewed changes

sijie merged commit 647c2dc into apache:master Mar 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pulsar-perf: add ability to create partitioned topics #9859

pulsar-perf: add ability to create partitioned topics #9859

eolivelli commented Mar 10, 2021 •

edited

Loading

eolivelli commented Mar 10, 2021

codelipenghui Mar 10, 2021

eolivelli Mar 10, 2021

eolivelli Mar 10, 2021

sijie Mar 12, 2021

eolivelli Mar 15, 2021

315157973 left a comment

eolivelli commented Mar 10, 2021

eolivelli commented Mar 10, 2021

eolivelli commented Mar 10, 2021

eolivelli commented Mar 11, 2021

eolivelli commented Mar 11, 2021

eolivelli commented Mar 12, 2021

eolivelli commented Mar 12, 2021

sijie Mar 12, 2021

eolivelli Mar 15, 2021

sijie Mar 12, 2021

eolivelli Mar 15, 2021

sijie Mar 12, 2021

eolivelli left a comment

eolivelli Mar 15, 2021

eolivelli Mar 15, 2021

eolivelli Mar 15, 2021

eolivelli commented Mar 15, 2021

	public int createTopicPartitions = 0;
	public int partitions = 0;

	public int createTopicPartitions = 0;
	public Integer createTopicPartitions = null;

pulsar-perf: add ability to create partitioned topics #9859

pulsar-perf: add ability to create partitioned topics #9859

Conversation

eolivelli commented Mar 10, 2021 • edited Loading

Motivation

Modifications

Verifying this change

Documentation

eolivelli commented Mar 10, 2021

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

Choose a reason for hiding this comment

315157973 left a comment

Choose a reason for hiding this comment

eolivelli commented Mar 10, 2021

eolivelli commented Mar 10, 2021

eolivelli commented Mar 10, 2021

eolivelli commented Mar 11, 2021

eolivelli commented Mar 11, 2021

eolivelli commented Mar 12, 2021

eolivelli commented Mar 12, 2021

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

eolivelli commented Mar 15, 2021

eolivelli commented Mar 10, 2021 •

edited

Loading