[GitHub] activemq-artemis pull request #1984: ARTEMIS-1779 ClusterConnectionBridge ma...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] activemq-artemis pull request #1984: ARTEMIS-1779 ClusterConnectionBridge ma...

[gaohoward]

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

https://github.com/apache/activemq-artemis/pull/1984#discussion_r178819137
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/ClusterConnectionBridge.java
 ---
@@ -127,9 +126,6 @@ public ClusterConnectionBridge(final ClusterConnection 
clusterConnection,
   this.managementNotificationAddress = managementNotificationAddress;
   this.flowRecord = flowRecord;
 
-  // we need to disable DLQ check on the clustered bridges
-  queue.setInternalQueue(true);
--- End diff --

Thanks!


---

[GitHub] activemq-artemis pull request #1984: ARTEMIS-1779 ClusterConnectionBridge ma...

[clebertsuconic]

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

https://github.com/apache/activemq-artemis/pull/1984#discussion_r178814438
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/ClusterConnectionBridge.java
 ---
@@ -127,9 +126,6 @@ public ClusterConnectionBridge(final ClusterConnection 
clusterConnection,
   this.managementNotificationAddress = managementNotificationAddress;
   this.flowRecord = flowRecord;
 
-  // we need to disable DLQ check on the clustered bridges
-  queue.setInternalQueue(true);
--- End diff --

Ok.. I see..

I have a small refactoring on this.. instead of trusting queue.isInternal I 
think we should move nodeUp to Bridge, and @override on the ClusterBridge.

I'm running some tests before I merge it.


---

[GitHub] activemq-artemis pull request #1984: ARTEMIS-1779 ClusterConnectionBridge ma...

[gaohoward]

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

https://github.com/apache/activemq-artemis/pull/1984#discussion_r178696366
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/ClusterConnectionBridge.java
 ---
@@ -127,9 +126,6 @@ public ClusterConnectionBridge(final ClusterConnection 
clusterConnection,
   this.managementNotificationAddress = managementNotificationAddress;
   this.flowRecord = flowRecord;
 
-  // we need to disable DLQ check on the clustered bridges
-  queue.setInternalQueue(true);
--- End diff --

@clebertsuconic no it doesn't. But it's redundant because this has already 
been set during the creation of cluster connection bridge. See

org.apache.activemq.artemis.core.server.cluster.impl.ClusterConnectionImpl#nodeUP()


---

[GitHub] activemq-artemis pull request #1984: ARTEMIS-1779 ClusterConnectionBridge ma...

[clebertsuconic]

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

https://github.com/apache/activemq-artemis/pull/1984#discussion_r178669327
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/ClusterConnectionBridge.java
 ---
@@ -127,9 +126,6 @@ public ClusterConnectionBridge(final ClusterConnection 
clusterConnection,
   this.managementNotificationAddress = managementNotificationAddress;
   this.flowRecord = flowRecord;
 
-  // we need to disable DLQ check on the clustered bridges
-  queue.setInternalQueue(true);
--- End diff --

Why did you change this?

It seems it doesn't have any connection to your change.

Beyond that.. we don't want any DLQ processing on internal queues from 
clustered bridge. This is a dangerous change.


---

[GitHub] activemq-artemis pull request #1984: ARTEMIS-1779 ClusterConnectionBridge ma...

[gaohoward]

GitHub user gaohoward opened a pull request:

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

ARTEMIS-1779 ClusterConnectionBridge may connect to other nodes than its 
target


The cluster connection bridge has a TopologyListener and connects to a new 
node
each time it receives a nodeUp() event. It needs to put a check here to make
sure that the cluster bridge only connects to its target node and it's 
backups.

This issue shows up when you run 
LiveToLiveFailoverTest.testConsumerTransacted
test.

Also in this commit improvement of BackupSyncJournalTest so that it runs 
more
stable.

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

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

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

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


commit 5ea8eba8ed7445f010e996ea528d5f7f66e2fd0d
Author: Howard Gao 
Date:   2018-04-02T11:16:30Z

ARTEMIS-1779 ClusterConnectionBridge may connect to other nodes than its 
target

The cluster connection bridge has a TopologyListener and connects to a new 
node
each time it receives a nodeUp() event. It needs to put a check here to make
sure that the cluster bridge only connects to its target node and it's 
backups.

This issue shows up when you run 
LiveToLiveFailoverTest.testConsumerTransacted
test.

Also in this commit improvement of BackupSyncJournalTest so that it runs 
more
stable.




---