activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From clebertsuconic <...@git.apache.org>
Subject [GitHub] activemq-artemis pull request #1999: ARTEMIS-1790 Improve Topology Member Fi...
Date Mon, 09 Apr 2018 15:29:48 GMT
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1999#discussion_r180134239
  
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/TopologyMemberImpl.java
---
    @@ -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.
    +    */
        @Override
        public boolean isMember(RemotingConnection connection) {
    -      TransportConfiguration connectorConfig = connection.getTransportConnection() !=
null ? connection.getTransportConnection().getConnectorConfig() : null;
    -
    -      return isMember(connectorConfig);
    -
    +      return connection.isSameTarget(getConnector().getA(), getConnector().getB());
    --- 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. 


---

Mime
View raw message