[Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions #9716

murong00 · 2021-02-25T09:58:07Z

Motivation

Fixes #8340

Modifications

Fix option main parameter for CLI pulsar-perf produce/consume/read to specify multi topics.
Add option --subscriptions for CLI pulsar-perf consume to specify multi subscriptions.
Deprecate and hide the option --subscriber-name.
Modify the corresponding doc.

murong00 · 2021-02-26T03:00:51Z

/pulsarbot run-failure-checks

murong00 · 2021-03-01T10:05:22Z

@codelipenghui Please help to take a review about this when you are available.

eolivelli

I appreciate this work, happy to see that

Please do not break compatibility with the previous signature.
We have scripts that are used to work with many different versions of pulsar (just by changing the docker image name).

IMHO in general it is fine to add new feature, but we should not change the behaviour if it is not strictly nececessary

eolivelli · 2021-03-01T10:30:35Z

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

-        @Parameter(description = "persistent://prop/ns/my-topic", required = true)
-        public List<String> topic;
+        @Parameter(description = "A list of topics to consume from (e.g. persistent://prop/ns/topic1 persistent://prop/ns/topic2)", required = true)
+        public List<String> topics;


are we breaking compatibility with the old format ?

Thanks for pointing this out, just keep the old format.

eolivelli · 2021-03-01T10:31:17Z

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

@@ -109,7 +108,7 @@
        @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-topics" }, description = "Number of topics")


are we breaking compatibility with the previous version ?

Done, use the previous version.

murong00 · 2021-03-02T05:39:55Z

/pulsarbot run-failure-checks

zymap

Please avoid changing the document and the code with the same PR. Pulsar's document does not need to cherry-pick to the release branch and this may introduce some unnecessary work when we cherry-pick this to the target branch.

… subscriptions (#9716) ### Motivation Fixes #8340 ### Modifications - Fix option `main parameter` for CLI `pulsar-perf produce/consume/read` to specify multi topics. - Add option `--subscriptions` for CLI `pulsar-perf` consume to specify multi subscriptions. - Deprecate and hide the option `--subscriber-name`. - Modify the corresponding doc. (cherry picked from commit e137f26)

… subscriptions (apache#9716) ### Motivation Fixes apache#8340 ### Modifications - Fix option `main parameter` for CLI `pulsar-perf produce/consume/read` to specify multi topics. - Add option `--subscriptions` for CLI `pulsar-perf` consume to specify multi subscriptions. - Deprecate and hide the option `--subscriber-name`. - Modify the corresponding doc. (cherry picked from commit e137f26)

… subscriptions (apache#9716) ### Motivation Fixes apache#8340 ### Modifications - Fix option `main parameter` for CLI `pulsar-perf produce/consume/read` to specify multi topics. - Add option `--subscriptions` for CLI `pulsar-perf` consume to specify multi subscriptions. - Deprecate and hide the option `--subscriber-name`. - Modify the corresponding doc.

eolivelli · 2021-03-05T13:40:25Z

@zymap
This patch broke compatibility with the previous version.
I have tools that used the useful feature, that is to auto create many topics on demand and automatically

I left comments and you committed the patch anyway.

This is not the right way to behave IMHO.

Can you please revert this patch ?

Otherwise I will be happy to send a patch that at least restores the auto-topic creation behaviour

bin/pulsar-perf produce -t 10 test

should create 10 topics, now it does not work anymore

The size of topics list should be equal to --num-topic
Usage: pulsar-perf produce [options] persistent://prop/ns/my-topic
  Options:

codelipenghui · 2021-03-05T14:14:07Z

Currently, we don't have any tests that cover the test client, we'd better add tests to avoid the broken.

eolivelli · 2021-03-05T14:44:24Z

@codelipenghui I agree.
But I added a comment and asked explicitly to not change the behaviour.

IMHO This is not about tests, it is about taking care of comments of the users in the community.

In my experience in OSS projects, and especially in the ASF,
if anyone "request changes" or anyway has a perplexity regarding a change,
it is necessary to understand the problem deeper before moving forward with committing the patch.

Think about any other user/contributor that tries to comment and express his opinion on a change, but the owners of the project ignore his comments and go forward.......
how can the newcomer feel ?
This is not a good behaviour indeed.

Please take this into consideration for the next time.

The is no hurry in committing a patch in Apache projects,
but we must ensure that everybody feels well in the community
:-)

zymap · 2021-03-06T02:55:28Z

@eolivelli I am sorry for my mistake and the inconvenience.
I saw your all suggested changes already addressed by him and the command arguments look good for compatibility.
I will revert the change from branch 2.7 and we can fix the issue at master.

Also, it's a good idea to add some test case for this so we can keep the compatibility when creating PRs.

…pics and subscriptions (#9716)" This reverts commit f4a3f4a.

eolivelli · 2021-03-06T07:33:44Z

@zymap no problem.
Thanks.

I can send a patch on Monday.

I also have other improvements for pulsar-perf to contribute as well ;)

codelipenghui · 2021-03-08T07:30:23Z

@eolivelli

But I added a comment and asked explicitly to not change the behaviour.
IMHO This is not about tests, it is about taking care of comments of the users in the community.

You have mentioned 2 points and the author has fixed them. I'm not sure why to make you feel we are not taking care of your comments. If some other places break the behavior, we'd better point out? Otherwise, other reviewers may not understand where the break is.

murong00 · 2021-03-08T08:36:08Z

@eolivelli I'm so sorry that I misunderstood your comment about the breaking compatibility. Actually, I have discussed this with penghui on the issue page:

Both you and penghui are right about the behaviour. I think we can just fix this by adding an option to cover the compatibility. I can send a patch if you needed @eolivelli, sorry again about the mistake.

eolivelli · 2021-03-08T09:02:44Z

@murong00
don't worry at all!
let's fix this together.

I appreciate that you are volunteering to follow up. Thank you very much.

I really would like to see that same behaviour as before, otherwise the behaviour of the tool is different from one version to another and you cannot know which version of the tool you are using.
I am maintaining a suite of tests that are to be run against several different versions of Pulsar.
It is fine to ADD new features, but NOT to change the behaviour.

So out-of-the-box it would be great that the behaviour does not change.
So adding a new "compatibility mode" flag won't work for me, because I cannot pass that flag to old versions.

I was going to start writing a patch in order to fallback to 2.7 mode in case you pass only one topic to the command.
pulsar-perf produce -n 10 test
here we see that the user is selecting only one topic and asking for 10...so we can expand the list of topics to
test-1, test-2, test-3....

this way we are 100% compatible with previous behaviour and your new behaviour works as well

The same is for pulsar-perf consume please

I will be happy to test your patch

murong00 · 2021-03-08T09:21:13Z

@eolivelli Thanks for your tips, I will send a patch later to cover this.

…pics and subscriptions (apache#9716)" This reverts commit f4a3f4a. (cherry picked from commit 8ea4a39)

### Motivation Fixes #9821 ### Modifications Add the logic to cover the default behaviour of previous versions. More details can be found in #9716.

Fix to support to specify topics and subscriptions

85b8323

murong00 requested review from merlimat, sijie and codelipenghui February 25, 2021 09:59

murong00 self-assigned this Feb 25, 2021

eolivelli requested changes Mar 1, 2021

View reviewed changes

keep compatibility with the old format

30587c4

murong00 added this to the 2.8.0 milestone Mar 2, 2021

murong00 added the release/2.7.1 label Mar 2, 2021

codelipenghui approved these changes Mar 3, 2021

View reviewed changes

codelipenghui added the type/performance label Mar 3, 2021

zymap approved these changes Mar 3, 2021

View reviewed changes

zymap merged commit e137f26 into apache:master Mar 3, 2021

zymap added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Mar 3, 2021

eolivelli mentioned this pull request Mar 5, 2021

pulsar-perf produce: "-t" option is broken #9821

Closed

sijie mentioned this pull request Mar 5, 2021

ISSUE-9821: pulsar-perf produce: "-t" option is broken streamnative/pulsar-archived#2253

Closed

zymap added a commit that referenced this pull request Mar 6, 2021

Revert "[Issue 8340] [pulsar-testclient] Fix to support to specify to…

8ea4a39

…pics and subscriptions (#9716)" This reverts commit f4a3f4a.

zymap removed cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.1 labels Mar 6, 2021

zymap added a commit to streamnative/pulsar-archived that referenced this pull request Mar 8, 2021

Revert "[Issue 8340] [pulsar-testclient] Fix to support to specify to…

3ec65c2

…pics and subscriptions (apache#9716)" This reverts commit f4a3f4a. (cherry picked from commit 8ea4a39)

murong00 mentioned this pull request Mar 8, 2021

[Issue 9821] [pulsar-testclient] Fix perf tools to keep compatibility with previous version #9838

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions #9716

[Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions #9716

murong00 commented Feb 25, 2021

murong00 commented Feb 26, 2021

murong00 commented Mar 1, 2021

eolivelli left a comment

eolivelli Mar 1, 2021

murong00 Mar 2, 2021

eolivelli Mar 1, 2021

murong00 Mar 2, 2021

murong00 commented Mar 2, 2021

zymap left a comment

eolivelli commented Mar 5, 2021

codelipenghui commented Mar 5, 2021

eolivelli commented Mar 5, 2021

zymap commented Mar 6, 2021 •

edited

eolivelli commented Mar 6, 2021

codelipenghui commented Mar 8, 2021

murong00 commented Mar 8, 2021

eolivelli commented Mar 8, 2021

murong00 commented Mar 8, 2021

[Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions #9716

[Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions #9716

Conversation

murong00 commented Feb 25, 2021

Motivation

Modifications

murong00 commented Feb 26, 2021

murong00 commented Mar 1, 2021

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli Mar 1, 2021

Choose a reason for hiding this comment

murong00 Mar 2, 2021

Choose a reason for hiding this comment

eolivelli Mar 1, 2021

Choose a reason for hiding this comment

murong00 Mar 2, 2021

Choose a reason for hiding this comment

murong00 commented Mar 2, 2021

zymap left a comment

Choose a reason for hiding this comment

eolivelli commented Mar 5, 2021

codelipenghui commented Mar 5, 2021

eolivelli commented Mar 5, 2021

zymap commented Mar 6, 2021 • edited

eolivelli commented Mar 6, 2021

codelipenghui commented Mar 8, 2021

murong00 commented Mar 8, 2021

eolivelli commented Mar 8, 2021

murong00 commented Mar 8, 2021

zymap commented Mar 6, 2021 •

edited