[GitHub] activemq-artemis pull request #1999: ARTEMIS-1790 Improve Topology Member Fi...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/activemq-artemis/pull/1999


---

[GitHub] activemq-artemis pull request #1999: ARTEMIS-1790 Improve Topology Member Fi...

[gaohoward]

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

https://github.com/apache/activemq-artemis/pull/1999#discussion_r180614044
  
--- Diff: 
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnection.java
 ---
@@ -511,6 +511,45 @@ public final boolean isUsingProtocolHandling() {
   return true;
}
 
+   @Override
+   public boolean isSameTarget(TransportConfiguration... configs) {
+  boolean yes = false;
--- End diff --

alright. :)



---

[GitHub] activemq-artemis pull request #1999: ARTEMIS-1790 Improve Topology Member Fi...

[clebertsuconic]

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

https://github.com/apache/activemq-artemis/pull/1999#discussion_r180507233
  
--- Diff: 
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnection.java
 ---
@@ -511,6 +511,45 @@ public final boolean isUsingProtocolHandling() {
   return true;
}
 
+   @Override
+   public boolean isSameTarget(TransportConfiguration... configs) {
+  boolean yes = false;
--- End diff --

@gaohoward  can you rename it from yes? :)

call it result if you like :)


---

[GitHub] activemq-artemis pull request #1999: ARTEMIS-1790 Improve Topology Member Fi...

[stanlyDoge]

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

https://github.com/apache/activemq-artemis/pull/1999#discussion_r180413588
  
--- Diff: 
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnection.java
 ---
@@ -511,6 +511,45 @@ public final boolean isUsingProtocolHandling() {
   return true;
}
 
+   @Override
+   public boolean isSameTarget(TransportConfiguration... configs) {
+  boolean yes = false;
--- End diff --

hehehe


---

[GitHub] activemq-artemis pull request #1999: ARTEMIS-1790 Improve Topology Member Fi...

[gaohoward]

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

https://github.com/apache/activemq-artemis/pull/1999#discussion_r180281379
  
--- 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 --

@clebertsuconic Thanks!


---

[GitHub] activemq-artemis pull request #1999: ARTEMIS-1790 Improve Topology Member Fi...

[clebertsuconic]

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

https://github.com/apache/activemq-artemis/pull/1999#discussion_r180281260
  
--- 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 --

I ran the testsuite and it’s a lot stable now. 

So let me do some tests tomorrow. I think it’s a minor change now. 

Leave it with me. 


---

[GitHub] activemq-artemis pull request #1999: ARTEMIS-1790 Improve Topology Member Fi...

[gaohoward]

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

https://github.com/apache/activemq-artemis/pull/1999#discussion_r180279022
  
--- 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 --

ok, I'll fix the isMember() and put some context.


---

[GitHub] activemq-artemis pull request #1999: ARTEMIS-1790 Improve Topology Member Fi...

[clebertsuconic]

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

https://github.com/apache/activemq-artemis/pull/1999#discussion_r180162324
  
--- 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 --

@gaohoward  why not just fix isMember.

JBEAP is a RedHat specific issue. perhaps we should add context on an 
apache jira.


---

[GitHub] activemq-artemis pull request #1999: ARTEMIS-1790 Improve Topology Member Fi...

[gaohoward]

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

https://github.com/apache/activemq-artemis/pull/1999#discussion_r180155663
  
--- 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 --

The issue here: https://issues.jboss.org/browse/JBEAP-14165
the issue is that the broker thinks 127.0.0.1 and localhost are different 
hosts when checking isMemeber(), where it just compare the configuration 
parameter map. 


---

[GitHub] activemq-artemis pull request #1999: ARTEMIS-1790 Improve Topology Member Fi...

[clebertsuconic]

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. 


---

[GitHub] activemq-artemis pull request #1999: ARTEMIS-1790 Improve Topology Member Fi...

[gaohoward]

GitHub user gaohoward opened a pull request:

https://github.com/apache/activemq-artemis/pull/1999

ARTEMIS-1790 Improve Topology Member Finding

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 127.0.0.1 are same thing).

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

$ git pull https://github.com/gaohoward/activemq-artemis h_a1790

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

https://github.com/apache/activemq-artemis/pull/1999.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 #1999


commit f68d994f3124f5b533df53d70aa7495a83085b41
Author: Howard Gao 
Date:   2018-03-27T04:38:08Z

ARTEMIS-1790 Improve Topology Member Finding

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 127.0.0.1 are same thing).




---