ASF GitHub Bot commented on ARTEMIS-1790:

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

    --- Diff: 
    @@ -105,12 +105,16 @@ public void setUniqueEventID(final long 
uniqueEventID) {
           return connector;
    +   /**
    +    * We only need to check if the connection point to the same node,
    +    * don't need to compare the whole params map.
    +    * @param connection The connection to the target node
    +    * @return true if the connection point to the same node
    +    * as this member represents.
    +    */
        public boolean isMember(RemotingConnection connection) {
    -      TransportConfiguration connectorConfig = 
connection.getTransportConnection() != null ? 
connection.getTransportConnection().getConnectorConfig() : null;
    -      return isMember(connectorConfig);
    +      return connection.isSameTarget(getConnector().getA(), 
    --- End diff --
    First what was the issue that you fixed? isn't this what the isMember was 
checking before?
    and now you are testing if both A and B are the same as the isMember... 
that seems a dangerous change to me that could break Failover and a lot of 
other things...
    This is Hot Code.. we need to be careful here... I need some more 
information on what you tried to achieve here.. some tests showing what was 
broken against the current codebase.
    The tests you added are validating only new bits.. so I don't know how to 
apply this patch safely. 

> Improve Topology Member Finding
> -------------------------------
>                 Key: ARTEMIS-1790
>                 URL: https://issues.apache.org/jira/browse/ARTEMIS-1790
>             Project: ActiveMQ Artemis
>          Issue Type: Improvement
>          Components: Broker
>    Affects Versions: 2.5.0
>            Reporter: Howard Gao
>            Assignee: Howard Gao
>            Priority: Major
>             Fix For: 2.5.1
> When finding out if a connector belong to a target node it compares the whole 
> parameter map which is not necessary. Also in understanding the connector the 
> best place is to delegate it to the corresponding remoting connection who 
> understands it. (e.g. INVMConnection knows whether the connector belongs to a 
> target node by checking it's serverID only. The netty ones only need to match 
> host and port, and understanding that localhost and are same thing).

This message was sent by Atlassian JIRA

Reply via email to