You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/02/25 09:58:19 UTC
[GitHub] [pulsar] murong00 opened a new pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
murong00 opened a new pull request #9716:
URL: https://github.com/apache/pulsar/pull/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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-791889737
@zymap no problem.
Thanks.
I can send a patch on Monday.
I also have other improvements for pulsar-perf to contribute as well ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-791425429
@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:
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-792578482
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-792538577
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap edited a comment on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
zymap edited a comment on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-791849070
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap merged pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
zymap merged pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-791462981
@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
:-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 removed a comment on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
murong00 removed a comment on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-788541216
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-788541216
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-791444743
Currently, we don't have any tests that cover the test client, we'd better add tests to avoid the broken.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 removed a comment on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
murong00 removed a comment on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-786336550
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on a change in pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
murong00 commented on a change in pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#discussion_r585183597
##########
File path: pulsar-testclient/src/main/java/org/apache/pulsar/testclient/PerformanceConsumer.java
##########
@@ -72,8 +71,8 @@
@Parameter(names = { "--conf-file" }, description = "Configuration file")
public String confFile;
- @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;
Review comment:
Thanks for pointing this out, just keep the old format.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-791849070
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-792611028
@eolivelli Thanks for your tips, I will send a patch later to cover this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-787824677
@codelipenghui Please help to take a review about this when you are available.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on a change in pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
murong00 commented on a change in pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#discussion_r585183866
##########
File path: 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")
Review comment:
Done, use the previous version.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-786377335
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-788628022
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] murong00 commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
murong00 commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-786336550
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#discussion_r584601470
##########
File path: 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")
Review comment:
are we breaking compatibility with the previous version ?
##########
File path: pulsar-testclient/src/main/java/org/apache/pulsar/testclient/PerformanceConsumer.java
##########
@@ -72,8 +71,8 @@
@Parameter(names = { "--conf-file" }, description = "Configuration file")
public String confFile;
- @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;
Review comment:
are we breaking compatibility with the old format ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #9716: [Issue 8340] [pulsar-testclient] Fix to support to specify topics and subscriptions
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9716:
URL: https://github.com/apache/pulsar/pull/9716#issuecomment-792597210
@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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org