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/04/11 13:12:07 UTC

[GitHub] [rocketmq] sunxi92 opened a new pull request, #4152: [#4151]Add option p to the updateGlobalWhiteAddr command.

sunxi92 opened a new pull request, #4152:
URL: https://github.com/apache/rocketmq/pull/4152

   #4151


-- 
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] dugenkui03 commented on a diff in pull request #4152: [#4151]Add option p to the updateGlobalWhiteAddr command.

Posted by GitBox <gi...@apache.org>.
dugenkui03 commented on code in PR #4152:
URL: https://github.com/apache/rocketmq/pull/4152#discussion_r847512176


##########
acl/src/main/java/org/apache/rocketmq/acl/AccessValidator.java:
##########
@@ -71,7 +71,7 @@
      *
      * @return
      */
-    boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList);
+    boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList, String aclFileFullPath);
 

Review Comment:
   建议新增`default`接口以保持对旧代码的兼容性，避免该接口成为重大变更/breaking change。而且两个方法分别对默认路径和指定路径进行更新、语义更加明确。
   ```java
      default boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList, String aclFileFullPath) {
           return updateGlobalWhiteAddrsConfig(globalWhiteAddrsList);
       }
   ```



-- 
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] ni-ze commented on pull request #4152: [#4151]Add option p to the updateGlobalWhiteAddr command.

Posted by GitBox <gi...@apache.org>.
ni-ze commented on PR #4152:
URL: https://github.com/apache/rocketmq/pull/4152#issuecomment-1097470929

   @sunxi92 Travis CI have failed.


-- 
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] dugenkui03 commented on a diff in pull request #4152: [#4151]Add option p to the updateGlobalWhiteAddr command.

Posted by GitBox <gi...@apache.org>.
dugenkui03 commented on code in PR #4152:
URL: https://github.com/apache/rocketmq/pull/4152#discussion_r847512176


##########
acl/src/main/java/org/apache/rocketmq/acl/AccessValidator.java:
##########
@@ -71,7 +71,7 @@
      *
      * @return
      */
-    boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList);
+    boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList, String aclFileFullPath);
 

Review Comment:
   建议新增`default`接口以保持向前兼容，避免该修改成为重大变更/breaking change。而且两个方法分别对默认路径和指定路径进行更新、语义更加明确。
   ```java
      default boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList, String aclFileFullPath) {
           return updateGlobalWhiteAddrsConfig(globalWhiteAddrsList);
       }
   ```



-- 
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] dugenkui03 commented on a diff in pull request #4152: [#4151]Add option p to the updateGlobalWhiteAddr command.

Posted by GitBox <gi...@apache.org>.
dugenkui03 commented on code in PR #4152:
URL: https://github.com/apache/rocketmq/pull/4152#discussion_r847512176


##########
acl/src/main/java/org/apache/rocketmq/acl/AccessValidator.java:
##########
@@ -71,7 +71,7 @@
      *
      * @return
      */
-    boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList);
+    boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList, String aclFileFullPath);
 

Review Comment:
   建议新增`default`接口以保持向前兼容，避免该接口成为重大变更/breaking change。而且两个方法分别对默认路径和指定路径进行更新、语义更加明确。
   ```java
      default boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList, String aclFileFullPath) {
           return updateGlobalWhiteAddrsConfig(globalWhiteAddrsList);
       }
   ```



-- 
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] caigy merged pull request #4152: [#4151]Add option p to the updateGlobalWhiteAddr command.

Posted by GitBox <gi...@apache.org>.
caigy merged PR #4152:
URL: https://github.com/apache/rocketmq/pull/4152


-- 
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] sunxi92 commented on pull request #4152: [#4151]Add option p to the updateGlobalWhiteAddr command.

Posted by GitBox <gi...@apache.org>.
sunxi92 commented on PR #4152:
URL: https://github.com/apache/rocketmq/pull/4152#issuecomment-1098102472

   > @sunxi92 Travis CI have failed.
   
   missing license, already added.


-- 
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] caigy commented on a diff in pull request #4152: [#4151]Add option p to the updateGlobalWhiteAddr command.

Posted by GitBox <gi...@apache.org>.
caigy commented on code in PR #4152:
URL: https://github.com/apache/rocketmq/pull/4152#discussion_r886322028


##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainPermissionManager.java:
##########
@@ -471,6 +471,10 @@ public boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList) {
     }
 
     public boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList, String fileName) {
+        if (fileName == null || fileName == "") {

Review Comment:
   use `equals()` instead of `==` for comparison



-- 
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] caigy commented on a diff in pull request #4152: [#4151]Add option p to the updateGlobalWhiteAddr command.

Posted by GitBox <gi...@apache.org>.
caigy commented on code in PR #4152:
URL: https://github.com/apache/rocketmq/pull/4152#discussion_r877665039


##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainPermissionManager.java:
##########
@@ -466,11 +466,11 @@ public boolean deleteAccessConfig(String accesskey) {
         return false;
     }
 
-    public boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList) {
-        return this.updateGlobalWhiteAddrsConfig(globalWhiteAddrsList, this.defaultAclFile);
-    }
-

Review Comment:
   It may be better to keep original implementation for compatibility.



##########
tools/src/main/java/org/apache/rocketmq/tools/admin/MQAdminExt.java:
##########
@@ -77,7 +77,7 @@ void createAndUpdatePlainAccessConfig(final String addr, final PlainAccessConfig
     void deletePlainAccessConfig(final String addr, final String accessKey) throws RemotingException, MQBrokerException,
         InterruptedException, MQClientException;
 
-    void updateGlobalWhiteAddrConfig(final String addr, final String globalWhiteAddrs)throws RemotingException, MQBrokerException,
+    void updateGlobalWhiteAddrConfig(final String addr, final String globalWhiteAddrs, String aclFileFullPath)throws RemotingException, MQBrokerException,

Review Comment:
   It may be more appropriate to add a new method instead of changing its signature in an interface.



##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainPermissionManager.java:
##########
@@ -466,11 +466,11 @@ public boolean deleteAccessConfig(String accesskey) {
         return false;
     }
 
-    public boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList) {
-        return this.updateGlobalWhiteAddrsConfig(globalWhiteAddrsList, this.defaultAclFile);
-    }
-
     public boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList, String fileName) {
+        if (fileName == null) {

Review Comment:
   May `fileName` be empty string?



##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainPermissionManager.java:
##########
@@ -482,6 +482,16 @@ public boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList, S
             return false;
         }
 
+        if (!fileName.startsWith(fileHome)) {
+            log.error("Parameter value " + fileName + " is not in the directory rocketmq.home.dir");

Review Comment:
   Logging with the value of `rocketmq.home.dir` will be more clear.



##########
acl/src/main/java/org/apache/rocketmq/acl/AccessValidator.java:
##########
@@ -71,7 +71,7 @@
      *
      * @return
      */
-    boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList);
+    boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList, String aclFileFullPath);
 

Review Comment:
   +1. Compatibility should be kept.



-- 
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] coveralls commented on pull request #4152: [#4151]Add option p to the updateGlobalWhiteAddr command.

Posted by GitBox <gi...@apache.org>.
coveralls commented on PR #4152:
URL: https://github.com/apache/rocketmq/pull/4152#issuecomment-1098004788

   
   [![Coverage Status](https://coveralls.io/builds/48246194/badge)](https://coveralls.io/builds/48246194)
   
   Coverage decreased (-0.01%) to 51.954% when pulling **09a3d29d9e773a1f6b0643f9c170303672283ac4 on sunxi92:feature_update_global_white_addr** into **ead6274b3e8016ee2fa75cf0dc201b5581ee7a34 on apache:develop**.
   


-- 
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] coveralls commented on pull request #4152: [#4151]Add option p to the updateGlobalWhiteAddr command.

Posted by GitBox <gi...@apache.org>.
coveralls commented on PR #4152:
URL: https://github.com/apache/rocketmq/pull/4152#issuecomment-1098004786

   
   [![Coverage Status](https://coveralls.io/builds/48246194/badge)](https://coveralls.io/builds/48246194)
   
   Coverage decreased (-0.01%) to 51.954% when pulling **09a3d29d9e773a1f6b0643f9c170303672283ac4 on sunxi92:feature_update_global_white_addr** into **ead6274b3e8016ee2fa75cf0dc201b5581ee7a34 on apache:develop**.
   


-- 
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 #4152: [#4151]Add option p to the updateGlobalWhiteAddr command.

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #4152:
URL: https://github.com/apache/rocketmq/pull/4152#issuecomment-1098001251

   # [Codecov](https://codecov.io/gh/apache/rocketmq/pull/4152?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#4152](https://codecov.io/gh/apache/rocketmq/pull/4152?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (09a3d29) into [develop](https://codecov.io/gh/apache/rocketmq/commit/4574d59f4b8d28888576293db6c019a1d2c37adb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4574d59) will **increase** coverage by `0.00%`.
   > The diff coverage is `24.00%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##             develop    #4152   +/-   ##
   ==========================================
     Coverage      47.90%   47.90%           
   - Complexity      5001     5005    +4     
   ==========================================
     Files            633      635    +2     
     Lines          42523    42524    +1     
     Branches        5573     5571    -2     
   ==========================================
   + Hits           20371    20372    +1     
     Misses         19654    19654           
     Partials        2498     2498           
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/rocketmq/pull/4152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ocketmq/broker/processor/AdminBrokerProcessor.java](https://codecov.io/gh/apache/rocketmq/pull/4152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvcHJvY2Vzc29yL0FkbWluQnJva2VyUHJvY2Vzc29yLmphdmE=) | `34.31% <0.00%> (-0.04%)` | :arrow_down: |
   | [...g/apache/rocketmq/client/impl/MQClientAPIImpl.java](https://codecov.io/gh/apache/rocketmq/pull/4152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9NUUNsaWVudEFQSUltcGwuamF2YQ==) | `13.84% <0.00%> (-0.02%)` | :arrow_down: |
   | [...der/UpdateGlobalWhiteAddrsConfigRequestHeader.java](https://codecov.io/gh/apache/rocketmq/pull/4152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jb21tb24vcHJvdG9jb2wvaGVhZGVyL1VwZGF0ZUdsb2JhbFdoaXRlQWRkcnNDb25maWdSZXF1ZXN0SGVhZGVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...apache/rocketmq/tools/admin/DefaultMQAdminExt.java](https://codecov.io/gh/apache/rocketmq/pull/4152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dG9vbHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3Rvb2xzL2FkbWluL0RlZmF1bHRNUUFkbWluRXh0LmphdmE=) | `41.40% <0.00%> (ø)` | |
   | [...he/rocketmq/tools/admin/DefaultMQAdminExtImpl.java](https://codecov.io/gh/apache/rocketmq/pull/4152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dG9vbHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3Rvb2xzL2FkbWluL0RlZmF1bHRNUUFkbWluRXh0SW1wbC5qYXZh) | `39.83% <0.00%> (ø)` | |
   | [...che/rocketmq/acl/plain/PlainPermissionManager.java](https://codecov.io/gh/apache/rocketmq/pull/4152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YWNsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9hY2wvcGxhaW4vUGxhaW5QZXJtaXNzaW9uTWFuYWdlci5qYXZh) | `74.87% <25.00%> (-0.83%)` | :arrow_down: |
   | [...s/command/acl/UpdateGlobalWhiteAddrSubCommand.java](https://codecov.io/gh/apache/rocketmq/pull/4152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dG9vbHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3Rvb2xzL2NvbW1hbmQvYWNsL1VwZGF0ZUdsb2JhbFdoaXRlQWRkclN1YkNvbW1hbmQuamF2YQ==) | `36.36% <37.50%> (+2.15%)` | :arrow_up: |
   | [...pache/rocketmq/acl/plain/PlainAccessValidator.java](https://codecov.io/gh/apache/rocketmq/pull/4152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YWNsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9hY2wvcGxhaW4vUGxhaW5BY2Nlc3NWYWxpZGF0b3IuamF2YQ==) | `92.42% <100.00%> (ø)` | |
   | [...va/org/apache/rocketmq/store/FlushDiskWatcher.java](https://codecov.io/gh/apache/rocketmq/pull/4152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL0ZsdXNoRGlza1dhdGNoZXIuamF2YQ==) | `81.25% <0.00%> (-9.38%)` | :arrow_down: |
   | [...in/java/org/apache/rocketmq/test/util/MQAdmin.java](https://codecov.io/gh/apache/rocketmq/pull/4152/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dGVzdC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvdGVzdC91dGlsL01RQWRtaW4uamF2YQ==) | `38.88% <0.00%> (-5.56%)` | :arrow_down: |
   | ... and [38 more](https://codecov.io/gh/apache/rocketmq/pull/4152/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/rocketmq/pull/4152?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/rocketmq/pull/4152?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4574d59...09a3d29](https://codecov.io/gh/apache/rocketmq/pull/4152?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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