Uploaded image for project: 'Apache RocketMQ'
  1. Apache RocketMQ
  2. ROCKETMQ-138

Add AuthenticationException class to remove hard coded Aliyun authentication class

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.1.0-incubating
    • Component/s: rocketmq-remoting
    • Labels:
      None

      Description

      in NettyRemotingAbstract.java

      a hard coded aliyun class is used

      catch (Throwable e) {
                              if (!"com.aliyun.openservices.ons.api.impl.authority.exception.AuthenticationException"
                                  .equals(e.getClass().getCanonicalName())) {
                                  PLOG.error("process request exception", e);
                                  PLOG.error(cmd.toString());
                              }
      

      A common AuthenticationException should be added to identify Authentication failure. Developers can throw this exception so that remoting component can ignore it

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user Jaskey opened a pull request:

          https://github.com/apache/incubator-rocketmq/pull/75

          Add AuthenticationException class to remove hard coded Aliyun authentication code

          JIRA:https://issues.apache.org/jira/browse/ROCKETMQ-138

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/Jaskey/incubator-rocketmq ROCKETMQ-138-AuthenticationException

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/incubator-rocketmq/pull/75.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #75


          commit 79bb95e5fa7c8ddbf224e22868b13451b7c21f8b
          Author: Jaskey <linjunjie1103@gmail.com>
          Date: 2017-03-10T08:17:05Z

          Add AuthenticationException class to remove hard coded Aliyun authentication class


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user Jaskey opened a pull request: https://github.com/apache/incubator-rocketmq/pull/75 Add AuthenticationException class to remove hard coded Aliyun authentication code JIRA: https://issues.apache.org/jira/browse/ROCKETMQ-138 You can merge this pull request into a Git repository by running: $ git pull https://github.com/Jaskey/incubator-rocketmq ROCKETMQ-138 -AuthenticationException Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-rocketmq/pull/75.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #75 commit 79bb95e5fa7c8ddbf224e22868b13451b7c21f8b Author: Jaskey <linjunjie1103@gmail.com> Date: 2017-03-10T08:17:05Z Add AuthenticationException class to remove hard coded Aliyun authentication class
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          [![Coverage Status](https://coveralls.io/builds/10542075/badge)](https://coveralls.io/builds/10542075)

          Coverage decreased (-0.006%) to 31.023% when pulling *79bb95e5fa7c8ddbf224e22868b13451b7c21f8b on Jaskey:ROCKETMQ-138-AuthenticationException* into *a146646b27af75540b7691e6dd9b1227d6aaf59b on apache:develop*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 [! [Coverage Status] ( https://coveralls.io/builds/10542075/badge)](https://coveralls.io/builds/10542075 ) Coverage decreased (-0.006%) to 31.023% when pulling * 79bb95e5fa7c8ddbf224e22868b13451b7c21f8b on Jaskey: ROCKETMQ-138 -AuthenticationException * into * a146646b27af75540b7691e6dd9b1227d6aaf59b on apache:develop *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          [![Coverage Status](https://coveralls.io/builds/10542075/badge)](https://coveralls.io/builds/10542075)

          Coverage decreased (-0.006%) to 31.023% when pulling *79bb95e5fa7c8ddbf224e22868b13451b7c21f8b on Jaskey:ROCKETMQ-138-AuthenticationException* into *a146646b27af75540b7691e6dd9b1227d6aaf59b on apache:develop*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 [! [Coverage Status] ( https://coveralls.io/builds/10542075/badge)](https://coveralls.io/builds/10542075 ) Coverage decreased (-0.006%) to 31.023% when pulling * 79bb95e5fa7c8ddbf224e22868b13451b7c21f8b on Jaskey: ROCKETMQ-138 -AuthenticationException * into * a146646b27af75540b7691e6dd9b1227d6aaf59b on apache:develop *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          @vongosling @shroman
          please review the updated pr

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 @vongosling @shroman please review the updated pr
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          [![Coverage Status](https://coveralls.io/builds/10557748/badge)](https://coveralls.io/builds/10557748)

          Coverage decreased (-0.04%) to 30.971% when pulling *e8104c0b89fb7b3527d24d99d2de358a82e91e36 on Jaskey:ROCKETMQ-138-AuthenticationException* into *d7decc84abc32dab63ee423d4d904f28d18cb1d7 on apache:develop*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 [! [Coverage Status] ( https://coveralls.io/builds/10557748/badge)](https://coveralls.io/builds/10557748 ) Coverage decreased (-0.04%) to 30.971% when pulling * e8104c0b89fb7b3527d24d99d2de358a82e91e36 on Jaskey: ROCKETMQ-138 -AuthenticationException * into * d7decc84abc32dab63ee423d4d904f28d18cb1d7 on apache:develop *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          [![Coverage Status](https://coveralls.io/builds/10557748/badge)](https://coveralls.io/builds/10557748)

          Coverage decreased (-0.04%) to 30.971% when pulling *e8104c0b89fb7b3527d24d99d2de358a82e91e36 on Jaskey:ROCKETMQ-138-AuthenticationException* into *d7decc84abc32dab63ee423d4d904f28d18cb1d7 on apache:develop*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 [! [Coverage Status] ( https://coveralls.io/builds/10557748/badge)](https://coveralls.io/builds/10557748 ) Coverage decreased (-0.04%) to 30.971% when pulling * e8104c0b89fb7b3527d24d99d2de358a82e91e36 on Jaskey: ROCKETMQ-138 -AuthenticationException * into * d7decc84abc32dab63ee423d4d904f28d18cb1d7 on apache:develop *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zhouxinyu commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          @lollipopjin Please help review this PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zhouxinyu commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 @lollipopjin Please help review this PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vongosling commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          LGTM

          Show
          githubbot ASF GitHub Bot added a comment - Github user vongosling commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 LGTM
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lollipopjin commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          @Jaskey In my opinion, it' better to just remove the

          > if (!"com.aliyun.openservices.ons.api.impl.authority.exception.AuthenticationException" .equals(e.getClass().getCanonicalName()))

          and record the log in any case throwable.

          Show
          githubbot ASF GitHub Bot added a comment - Github user lollipopjin commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 @Jaskey In my opinion, it' better to just remove the > if (!"com.aliyun.openservices.ons.api.impl.authority.exception.AuthenticationException" .equals(e.getClass().getCanonicalName())) and record the log in any case throwable.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vongosling commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          Yep agreed. We can remove the instanceof check and just log it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user vongosling commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 Yep agreed. We can remove the instanceof check and just log it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          Do you guys really consider that is good enough?? @vongosling @lollipopjin

          IMO, the origin design is OK, since the AuthenticationException is thrown from the hook.

          The devs will log their own Authentication fail log already (may be print periodically or somehow ), but rocketmq remoting should only care about the real error.

          If we remove it, all authentication fail will have one log, and this could be possibly very often and fill up with the remoting log, which I do not think it is good enough.

          User should care about their custom authentication log, and remoting component should care about real error thrown from the processor.

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 Do you guys really consider that is good enough?? @vongosling @lollipopjin IMO, the origin design is OK, since the AuthenticationException is thrown from the hook. The devs will log their own Authentication fail log already (may be print periodically or somehow ), but rocketmq remoting should only care about the real error. If we remove it, all authentication fail will have one log, and this could be possibly very often and fill up with the remoting log, which I do not think it is good enough. User should care about their custom authentication log, and remoting component should care about real error thrown from the processor.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vongosling commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          If we remove it, all authentication fail will have one log, and this could be possibly very often and fill up with the remoting log, which I do not think it is good enough.

          We can just keep it as the previous. We will introduce a new remoting module in the 4.1 with seriously consideration about exception mechanism~

          Show
          githubbot ASF GitHub Bot added a comment - Github user vongosling commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 If we remove it, all authentication fail will have one log, and this could be possibly very often and fill up with the remoting log, which I do not think it is good enough. We can just keep it as the previous. We will introduce a new remoting module in the 4.1 with seriously consideration about exception mechanism~
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          @vongosling OK, look forward to that!

          So when will this pr be merged?

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 @vongosling OK, look forward to that! So when will this pr be merged?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vongosling commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          Could you remove the code snippet “ (!(e instanceof AuthenticationException)) {” and class AuthenticationException

          Show
          githubbot ASF GitHub Bot added a comment - Github user vongosling commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 Could you remove the code snippet “ (!(e instanceof AuthenticationException)) {” and class AuthenticationException
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          So do you mean we just remove the hard code snippet in the pr and just make it log for that? Better solution will be introduced in 4.1.x in another patches?

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 So do you mean we just remove the hard code snippet in the pr and just make it log for that? Better solution will be introduced in 4.1.x in another patches?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user shroman commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          If no special consideration for authentication errors is needed (which was obviously the intention in the original code), +1 for just removing the hard-coded snippet. No `AuthenticationException` is needed then.
          I see no problem logging on every `Throwable`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 If no special consideration for authentication errors is needed (which was obviously the intention in the original code), +1 for just removing the hard-coded snippet. No `AuthenticationException` is needed then. I see no problem logging on every `Throwable`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          [![Coverage Status](https://coveralls.io/builds/10801304/badge)](https://coveralls.io/builds/10801304)

          Coverage increased (+0.08%) to 31.701% when pulling *ce164ebb4dd624f7debcd3d83e9eaec246137b18 on Jaskey:ROCKETMQ-138-AuthenticationException* into *203cb30a906a77f41b0e5ba09fc351434862d408 on apache:develop*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 [! [Coverage Status] ( https://coveralls.io/builds/10801304/badge)](https://coveralls.io/builds/10801304 ) Coverage increased (+0.08%) to 31.701% when pulling * ce164ebb4dd624f7debcd3d83e9eaec246137b18 on Jaskey: ROCKETMQ-138 -AuthenticationException * into * 203cb30a906a77f41b0e5ba09fc351434862d408 on apache:develop *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          [![Coverage Status](https://coveralls.io/builds/10801304/badge)](https://coveralls.io/builds/10801304)

          Coverage increased (+0.08%) to 31.701% when pulling *ce164ebb4dd624f7debcd3d83e9eaec246137b18 on Jaskey:ROCKETMQ-138-AuthenticationException* into *203cb30a906a77f41b0e5ba09fc351434862d408 on apache:develop*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 [! [Coverage Status] ( https://coveralls.io/builds/10801304/badge)](https://coveralls.io/builds/10801304 ) Coverage increased (+0.08%) to 31.701% when pulling * ce164ebb4dd624f7debcd3d83e9eaec246137b18 on Jaskey: ROCKETMQ-138 -AuthenticationException * into * 203cb30a906a77f41b0e5ba09fc351434862d408 on apache:develop *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vongosling commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          @Jaskey Thanks for your elaborative consideration about exception. Let it go as you have changed. we will merge this PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user vongosling commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 @Jaskey Thanks for your elaborative consideration about exception. Let it go as you have changed. we will merge this PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          @vongosling

          Now nothing special has been done unless remove the hard corded aliyun code snippet.

          I will close this pr after merge.

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 @vongosling Now nothing special has been done unless remove the hard corded aliyun code snippet. I will close this pr after merge.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey closed the pull request at:

          https://github.com/apache/incubator-rocketmq/pull/75

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey closed the pull request at: https://github.com/apache/incubator-rocketmq/pull/75
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          It seems this pr is not merged yet, so I reopen it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 It seems this pr is not merged yet, so I reopen it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user Jaskey reopened a pull request:

          https://github.com/apache/incubator-rocketmq/pull/75

          ROCKETMQ-138Remove hard coded Aliyun authentication code

          JIRA:https://issues.apache.org/jira/browse/ROCKETMQ-138

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/Jaskey/incubator-rocketmq ROCKETMQ-138-AuthenticationException

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/incubator-rocketmq/pull/75.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #75


          commit ce164ebb4dd624f7debcd3d83e9eaec246137b18
          Author: Jaskey <linjunjie1103@gmail.com>
          Date: 2017-03-10T08:17:05Z

          ROCKETMQ-138remove hard coded Aliyun authentication class


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user Jaskey reopened a pull request: https://github.com/apache/incubator-rocketmq/pull/75 ROCKETMQ-138 Remove hard coded Aliyun authentication code JIRA: https://issues.apache.org/jira/browse/ROCKETMQ-138 You can merge this pull request into a Git repository by running: $ git pull https://github.com/Jaskey/incubator-rocketmq ROCKETMQ-138 -AuthenticationException Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-rocketmq/pull/75.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #75 commit ce164ebb4dd624f7debcd3d83e9eaec246137b18 Author: Jaskey <linjunjie1103@gmail.com> Date: 2017-03-10T08:17:05Z ROCKETMQ-138 remove hard coded Aliyun authentication class
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          [![Coverage Status](https://coveralls.io/builds/11020784/badge)](https://coveralls.io/builds/11020784)

          Coverage increased (+0.3%) to 31.868% when pulling *ce164ebb4dd624f7debcd3d83e9eaec246137b18 on Jaskey:ROCKETMQ-138-AuthenticationException* into *203cb30a906a77f41b0e5ba09fc351434862d408 on apache:develop*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 [! [Coverage Status] ( https://coveralls.io/builds/11020784/badge)](https://coveralls.io/builds/11020784 ) Coverage increased (+0.3%) to 31.868% when pulling * ce164ebb4dd624f7debcd3d83e9eaec246137b18 on Jaskey: ROCKETMQ-138 -AuthenticationException * into * 203cb30a906a77f41b0e5ba09fc351434862d408 on apache:develop *.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit a8fa05e8c0b0e57b6e2278747a5f6a8111447a8e in incubator-rocketmq's branch refs/heads/develop from Jaskey Lam
          [ https://git-wip-us.apache.org/repos/asf?p=incubator-rocketmq.git;h=a8fa05e ]

          ROCKETMQ-138 Remove hard coded Aliyun authentication code, closes apache/incubator-rocketmq#75

          Show
          jira-bot ASF subversion and git services added a comment - Commit a8fa05e8c0b0e57b6e2278747a5f6a8111447a8e in incubator-rocketmq's branch refs/heads/develop from Jaskey Lam [ https://git-wip-us.apache.org/repos/asf?p=incubator-rocketmq.git;h=a8fa05e ] ROCKETMQ-138 Remove hard coded Aliyun authentication code, closes apache/incubator-rocketmq#75
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zhouxinyu commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/75

          Done, please help close this PR. @Jaskey

          Show
          githubbot ASF GitHub Bot added a comment - Github user zhouxinyu commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 Done, please help close this PR. @Jaskey
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey closed the pull request at:

          https://github.com/apache/incubator-rocketmq/pull/75

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey closed the pull request at: https://github.com/apache/incubator-rocketmq/pull/75
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lyy4j commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/138

          I have look at the class which has a good abstract of Consistent Hash ,but I think the class SelectMessageQueueByConsistentHash is more simple and have the unified style with the original code

          Show
          githubbot ASF GitHub Bot added a comment - Github user lyy4j commented on the issue: https://github.com/apache/incubator-rocketmq/pull/138 I have look at the class which has a good abstract of Consistent Hash ,but I think the class SelectMessageQueueByConsistentHash is more simple and have the unified style with the original code
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/138

          @lyy4j

          1. I will still suggest resuing ConsistentHashRouter since it has a good enough abstraction which will make your selector much cleanner and easier as well as extensible(say specified virtual node nums, hash funtions)

          2. I will suggest we expose a more friendly interface for user to use, for example , just let user to implement a shardingKey(MessageQueue q) method, the `select` method should be hidden since it is not friendly enough in this case IMO

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/138 @lyy4j 1. I will still suggest resuing ConsistentHashRouter since it has a good enough abstraction which will make your selector much cleanner and easier as well as extensible(say specified virtual node nums, hash funtions) 2. I will suggest we expose a more friendly interface for user to use, for example , just let user to implement a shardingKey(MessageQueue q) method, the `select` method should be hidden since it is not friendly enough in this case IMO
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lyy4j commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/138

          I agree with you that ConsistentHashRouter has a good enough abstraction, making the code more cleanner and extensible. But I think SelectMessageQueueByConsistentHash will not increase the difficult of the using and more easier to understand and friendly to read comparing with using ConsistentHashRouter to implement

          Show
          githubbot ASF GitHub Bot added a comment - Github user lyy4j commented on the issue: https://github.com/apache/incubator-rocketmq/pull/138 I agree with you that ConsistentHashRouter has a good enough abstraction, making the code more cleanner and extensible. But I think SelectMessageQueueByConsistentHash will not increase the difficult of the using and more easier to understand and friendly to read comparing with using ConsistentHashRouter to implement
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dongeforever commented on a diff in the pull request:

          https://github.com/apache/incubator-rocketmq/pull/138#discussion_r140401564

          — Diff: client/src/main/java/org/apache/rocketmq/client/producer/selector/SelectMessageQueueByConsistentHash.java —
          @@ -0,0 +1,132 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.rocketmq.client.producer.selector;
          +
          +import org.apache.rocketmq.client.producer.MessageQueueSelector;
          +import org.apache.rocketmq.common.message.Message;
          +import org.apache.rocketmq.common.message.MessageQueue;
          +
          +import java.util.HashMap;
          +import java.util.List;
          +import java.util.SortedMap;
          +import java.util.TreeMap;
          +
          +/**
          +* wanting to send a message with the same key combine with orderly consumer, user can
          +* use the implements of the MessageQueueSelector, which has one optimization compared with SelectMessageQueueByHash
          +* when a broker crash , it can reduce the message Arriving out of the order
          +*/
          +public class SelectMessageQueueByConsistentHash implements MessageQueueSelector {
          +
          + private volatile SortedMap<Integer, String> virtualNodes =
          — End diff –

          how about just using <Integer, MessageQueue>?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dongeforever commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/138#discussion_r140401564 — Diff: client/src/main/java/org/apache/rocketmq/client/producer/selector/SelectMessageQueueByConsistentHash.java — @@ -0,0 +1,132 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.rocketmq.client.producer.selector; + +import org.apache.rocketmq.client.producer.MessageQueueSelector; +import org.apache.rocketmq.common.message.Message; +import org.apache.rocketmq.common.message.MessageQueue; + +import java.util.HashMap; +import java.util.List; +import java.util.SortedMap; +import java.util.TreeMap; + +/** +* wanting to send a message with the same key combine with orderly consumer, user can +* use the implements of the MessageQueueSelector, which has one optimization compared with SelectMessageQueueByHash +* when a broker crash , it can reduce the message Arriving out of the order +*/ +public class SelectMessageQueueByConsistentHash implements MessageQueueSelector { + + private volatile SortedMap<Integer, String> virtualNodes = — End diff – how about just using <Integer, MessageQueue>?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dongeforever commented on a diff in the pull request:

          https://github.com/apache/incubator-rocketmq/pull/138#discussion_r140401292

          — Diff: client/src/main/java/org/apache/rocketmq/client/producer/selector/SelectMessageQueueByConsistentHash.java —
          @@ -0,0 +1,132 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.rocketmq.client.producer.selector;
          +
          +import org.apache.rocketmq.client.producer.MessageQueueSelector;
          +import org.apache.rocketmq.common.message.Message;
          +import org.apache.rocketmq.common.message.MessageQueue;
          +
          +import java.util.HashMap;
          +import java.util.List;
          +import java.util.SortedMap;
          +import java.util.TreeMap;
          +
          +/**
          +* wanting to send a message with the same key combine with orderly consumer, user can
          +* use the implements of the MessageQueueSelector, which has one optimization compared with SelectMessageQueueByHash
          +* when a broker crash , it can reduce the message Arriving out of the order
          +*/
          +public class SelectMessageQueueByConsistentHash implements MessageQueueSelector {
          +
          + private volatile SortedMap<Integer, String> virtualNodes =
          + new TreeMap<Integer, String>();
          +
          + private static final int DEFAULT_VIRTUAL_NODES = 100;
          +
          + private final int virtualNodeNum;
          +
          + private volatile HashMap<String, MessageQueue> idToQueueMap = new HashMap<String, MessageQueue>();
          +
          + public SelectMessageQueueByConsistentHash()

          { + this.virtualNodeNum = DEFAULT_VIRTUAL_NODES; + }

          +
          + public SelectMessageQueueByConsistentHash(int virtualNodeNum)

          { + this.virtualNodeNum = virtualNodeNum; + }

          +
          + @Override
          + public MessageQueue select(List<MessageQueue> mqs, Message msg, Object arg) {
          + synchronized (this) {
          + if (queueChange(mqs))

          { + reloadConsistentHash(mqs); + }

          +
          + String uniqueQueueId = getMsgQueueIdBy(arg.toString());
          + MessageQueue messageQueue = idToQueueMap.get(uniqueQueueId);
          + return messageQueue;
          + }
          + }
          +
          + private boolean queueChange(List<MessageQueue> mqs) {
          + if (mqs.size() != this.idToQueueMap.size())

          { + return true; + }

          +
          + for (MessageQueue queue : mqs) {
          + String id = queue.getTopic() + "" + queue.getBrokerName() + "" + queue.getQueueId();
          + if (!this.idToQueueMap.containsKey(id))

          { + return true; + }

          + }
          +
          + return false;
          + }
          +
          + private String getMsgQueueIdBy(String arg) {
          + int hash = getHash(arg);
          + SortedMap<Integer, String> subMap = virtualNodes.tailMap(hash);
          +
          + Integer i;
          + String virtualNode;
          +
          + if (subMap.size() == 0)

          { + i = virtualNodes.firstKey(); + virtualNode = virtualNodes.get(i); + }

          else

          { + i = subMap.firstKey(); + virtualNode = subMap.get(i); + }

          +
          + String result = virtualNode.substring(0, virtualNode.indexOf("&&"));
          + return result;
          + }
          +
          + private int getHash(String str) {
          — End diff –

          Why not just use str.hashCode?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dongeforever commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/138#discussion_r140401292 — Diff: client/src/main/java/org/apache/rocketmq/client/producer/selector/SelectMessageQueueByConsistentHash.java — @@ -0,0 +1,132 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.rocketmq.client.producer.selector; + +import org.apache.rocketmq.client.producer.MessageQueueSelector; +import org.apache.rocketmq.common.message.Message; +import org.apache.rocketmq.common.message.MessageQueue; + +import java.util.HashMap; +import java.util.List; +import java.util.SortedMap; +import java.util.TreeMap; + +/** +* wanting to send a message with the same key combine with orderly consumer, user can +* use the implements of the MessageQueueSelector, which has one optimization compared with SelectMessageQueueByHash +* when a broker crash , it can reduce the message Arriving out of the order +*/ +public class SelectMessageQueueByConsistentHash implements MessageQueueSelector { + + private volatile SortedMap<Integer, String> virtualNodes = + new TreeMap<Integer, String>(); + + private static final int DEFAULT_VIRTUAL_NODES = 100; + + private final int virtualNodeNum; + + private volatile HashMap<String, MessageQueue> idToQueueMap = new HashMap<String, MessageQueue>(); + + public SelectMessageQueueByConsistentHash() { + this.virtualNodeNum = DEFAULT_VIRTUAL_NODES; + } + + public SelectMessageQueueByConsistentHash(int virtualNodeNum) { + this.virtualNodeNum = virtualNodeNum; + } + + @Override + public MessageQueue select(List<MessageQueue> mqs, Message msg, Object arg) { + synchronized (this) { + if (queueChange(mqs)) { + reloadConsistentHash(mqs); + } + + String uniqueQueueId = getMsgQueueIdBy(arg.toString()); + MessageQueue messageQueue = idToQueueMap.get(uniqueQueueId); + return messageQueue; + } + } + + private boolean queueChange(List<MessageQueue> mqs) { + if (mqs.size() != this.idToQueueMap.size()) { + return true; + } + + for (MessageQueue queue : mqs) { + String id = queue.getTopic() + " " + queue.getBrokerName() + " " + queue.getQueueId(); + if (!this.idToQueueMap.containsKey(id)) { + return true; + } + } + + return false; + } + + private String getMsgQueueIdBy(String arg) { + int hash = getHash(arg); + SortedMap<Integer, String> subMap = virtualNodes.tailMap(hash); + + Integer i; + String virtualNode; + + if (subMap.size() == 0) { + i = virtualNodes.firstKey(); + virtualNode = virtualNodes.get(i); + } else { + i = subMap.firstKey(); + virtualNode = subMap.get(i); + } + + String result = virtualNode.substring(0, virtualNode.indexOf("&&")); + return result; + } + + private int getHash(String str) { — End diff – Why not just use str.hashCode?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dongeforever commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/138

          @lyy4j It is important to put the code of same logic in one place. For easy maintenance, it is suggested to reuse ConsistentHashRouter. If you think that ConsistentHashRouter is not clear and extensible enough, please feel free to polish it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dongeforever commented on the issue: https://github.com/apache/incubator-rocketmq/pull/138 @lyy4j It is important to put the code of same logic in one place. For easy maintenance, it is suggested to reuse ConsistentHashRouter. If you think that ConsistentHashRouter is not clear and extensible enough, please feel free to polish it.

            People

            • Assignee:
              Jaskey Jaskey Lam
              Reporter:
              Jaskey Jaskey Lam
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development