You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by GitBox <gi...@apache.org> on 2022/10/08 01:53:31 UTC
[GitHub] [rocketmq] zhiliatom opened a new pull request, #5244: [ISSUE #5243] Add acl support to TimerProducer/TimerConsumer
zhiliatom opened a new pull request, #5244:
URL: https://github.com/apache/rocketmq/pull/5244
**Make sure set the target branch to `develop`**
## What is the purpose of the change
close #5243
## Brief changelog
1. fix wrong option name of namesrv
2. add acl support to TimerProducer/Consumer benchmark
3. add new 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.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on a diff in pull request #5244: [ISSUE #5243] Add acl support to TimerProducer/TimerConsumer
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on code in PR #5244:
URL: https://github.com/apache/rocketmq/pull/5244#discussion_r990756918
##########
example/src/main/java/org/apache/rocketmq/example/benchmark/timer/TimerConsumer.java:
##########
@@ -151,6 +166,30 @@ private Options buildCommandlineOptions(Options options) {
opt.setRequired(false);
options.addOption(opt);
+ opt = new Option("w", "threadCount", true, "Thread count, Default: 20");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("m", "msgTraceEnable", true, "Message Trace Enable, Default: false");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("g", "group", true, "Consumer group name, Default: benchmark_consumer");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("a", "aclEnable", true, "Acl Enable, Default: false");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("ak", "accessKey", true, "Acl access key, Default: rocketmq2");
Review Comment:
Having defaults for access key/secret might not be a good idea.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on a diff in pull request #5244: [ISSUE #5243] Add acl support to TimerProducer/TimerConsumer
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on code in PR #5244:
URL: https://github.com/apache/rocketmq/pull/5244#discussion_r990756918
##########
example/src/main/java/org/apache/rocketmq/example/benchmark/timer/TimerConsumer.java:
##########
@@ -151,6 +166,30 @@ private Options buildCommandlineOptions(Options options) {
opt.setRequired(false);
options.addOption(opt);
+ opt = new Option("w", "threadCount", true, "Thread count, Default: 20");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("m", "msgTraceEnable", true, "Message Trace Enable, Default: false");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("g", "group", true, "Consumer group name, Default: benchmark_consumer");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("a", "aclEnable", true, "Acl Enable, Default: false");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("ak", "accessKey", true, "Acl access key, Default: rocketmq2");
Review Comment:
Having defaults for access key/secret might not be a good idea. If access control is enabled, best to check required credentials are properly provided and warn neatly if something is missing or wrong, instead of using default ones.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] ltamber commented on a diff in pull request #5244: [ISSUE #5243] Add acl support to TimerProducer/TimerConsumer
Posted by GitBox <gi...@apache.org>.
ltamber commented on code in PR #5244:
URL: https://github.com/apache/rocketmq/pull/5244#discussion_r990717752
##########
example/src/main/java/org/apache/rocketmq/example/benchmark/timer/TimerConsumer.java:
##########
@@ -151,6 +166,30 @@ private Options buildCommandlineOptions(Options options) {
opt.setRequired(false);
options.addOption(opt);
+ opt = new Option("w", "threadCount", true, "Thread count, Default: 20");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("m", "msgTraceEnable", true, "Message Trace Enable, Default: false");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("g", "group", true, "Consumer group name, Default: benchmark_consumer");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("a", "aclEnable", true, "Acl Enable, Default: false");
Review Comment:
how about use a no argument option ?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhiliatom commented on a diff in pull request #5244: [ISSUE #5243] Add acl support to TimerProducer/TimerConsumer
Posted by GitBox <gi...@apache.org>.
zhiliatom commented on code in PR #5244:
URL: https://github.com/apache/rocketmq/pull/5244#discussion_r990793993
##########
example/src/main/java/org/apache/rocketmq/example/benchmark/timer/TimerConsumer.java:
##########
@@ -151,6 +166,30 @@ private Options buildCommandlineOptions(Options options) {
opt.setRequired(false);
options.addOption(opt);
+ opt = new Option("w", "threadCount", true, "Thread count, Default: 20");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("m", "msgTraceEnable", true, "Message Trace Enable, Default: false");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("g", "group", true, "Consumer group name, Default: benchmark_consumer");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("a", "aclEnable", true, "Acl Enable, Default: false");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("ak", "accessKey", true, "Acl access key, Default: rocketmq2");
Review Comment:
Thanks for your review, I’ll exchange their default value like existed code.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [GitHub] [rocketmq] ltamber commented on a diff in pull request #5244: [ISSUE #5243] Add acl support to TimerProducer/TimerConsumer
Posted by "zhiliatox@163.com" <zh...@163.com>.
Good suggestion, but to keep consistent wist other code, I don't do any change here
zhiliatox@163.com
From: GitBox
Date: 2022-10-09 09:42
To: dev
Subject: [GitHub] [rocketmq] ltamber commented on a diff in pull request #5244: [ISSUE #5243] Add acl support to TimerProducer/TimerConsumer
ltamber commented on code in PR #5244:
URL: https://github.com/apache/rocketmq/pull/5244#discussion_r990717698
##########
example/src/main/java/org/apache/rocketmq/example/benchmark/timer/TimerConsumer.java:
##########
@@ -151,6 +166,30 @@ private Options buildCommandlineOptions(Options options) {
opt.setRequired(false);
options.addOption(opt);
+ opt = new Option("w", "threadCount", true, "Thread count, Default: 20");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("m", "msgTraceEnable", true, "Message Trace Enable, Default: false");
Review Comment:
how about use a no argument option ?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] ltamber commented on a diff in pull request #5244: [ISSUE #5243] Add acl support to TimerProducer/TimerConsumer
Posted by GitBox <gi...@apache.org>.
ltamber commented on code in PR #5244:
URL: https://github.com/apache/rocketmq/pull/5244#discussion_r990717698
##########
example/src/main/java/org/apache/rocketmq/example/benchmark/timer/TimerConsumer.java:
##########
@@ -151,6 +166,30 @@ private Options buildCommandlineOptions(Options options) {
opt.setRequired(false);
options.addOption(opt);
+ opt = new Option("w", "threadCount", true, "Thread count, Default: 20");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("m", "msgTraceEnable", true, "Message Trace Enable, Default: false");
Review Comment:
how about use a no argument option ?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhiliatom commented on a diff in pull request #5244: [ISSUE #5243] Add acl support to TimerProducer/TimerConsumer
Posted by GitBox <gi...@apache.org>.
zhiliatom commented on code in PR #5244:
URL: https://github.com/apache/rocketmq/pull/5244#discussion_r990725960
##########
example/src/main/java/org/apache/rocketmq/example/benchmark/timer/TimerConsumer.java:
##########
@@ -151,6 +166,30 @@ private Options buildCommandlineOptions(Options options) {
opt.setRequired(false);
options.addOption(opt);
+ opt = new Option("w", "threadCount", true, "Thread count, Default: 20");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("m", "msgTraceEnable", true, "Message Trace Enable, Default: false");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("g", "group", true, "Consumer group name, Default: benchmark_consumer");
+ opt.setRequired(false);
+ options.addOption(opt);
+
+ opt = new Option("a", "aclEnable", true, "Acl Enable, Default: false");
Review Comment:
Good suggestion, but to keep consistent wist other code, I don't do any change here.
If It really need to change, I'd like to make a new patch to change all .sh .
--
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.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] codecov-commenter commented on pull request #5244: [ISSUE #5243] Add acl support to TimerProducer/TimerConsumer
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5244:
URL: https://github.com/apache/rocketmq/pull/5244#issuecomment-1720756115
## [Codecov](https://app.codecov.io/gh/apache/rocketmq/pull/5244?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#5244](https://app.codecov.io/gh/apache/rocketmq/pull/5244?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e5e71bd) into [develop](https://app.codecov.io/gh/apache/rocketmq/commit/4a8e0d5b851d1f9573cda79b7d2e42ee498809da?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (4a8e0d5) will **decrease** coverage by `0.11%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## develop #5244 +/- ##
=============================================
- Coverage 42.68% 42.58% -0.11%
+ Complexity 9482 9457 -25
=============================================
Files 1150 1150
Lines 82729 82729
Branches 10777 10777
=============================================
- Hits 35311 35228 -83
- Misses 43012 43087 +75
- Partials 4406 4414 +8
```
[see 42 files with indirect coverage changes](https://app.codecov.io/gh/apache/rocketmq/pull/5244/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org