[GitHub] activemq-artemis pull request #1787: ARTEMIS-1577 Address-settings policies ...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] activemq-artemis pull request #1787: ARTEMIS-1577 Address-settings policies ...

[michaelandrepearce]

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

https://github.com/apache/activemq-artemis/pull/1787#discussion_r162367328
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
 ---
@@ -1284,7 +1284,7 @@ private boolean checkDuplicateID(final Message 
message,
  boolean isDuplicate = false;
 
  if (duplicateIDBytes != null) {
-cache = getDuplicateIDCache(message.getAddressSimpleString());
+cache = getDuplicateIDCache(context.getAddress(message));
--- End diff --

No worries I can do separate pr later


---

[GitHub] activemq-artemis pull request #1787: ARTEMIS-1577 Address-settings policies ...

[clebertsuconic]

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

https://github.com/apache/activemq-artemis/pull/1787#discussion_r162358705
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
 ---
@@ -1284,7 +1284,7 @@ private boolean checkDuplicateID(final Message 
message,
  boolean isDuplicate = false;
 
  if (duplicateIDBytes != null) {
-cache = getDuplicateIDCache(message.getAddressSimpleString());
+cache = getDuplicateIDCache(context.getAddress(message));
--- End diff --

@michaelandrepearce caching here goes beyond the scope here.

I would need to change a lot of signatures to pass an address as a 
parameter.. or cache within each method used during routing.


We can do that.. but it goes beyond the scope of this fix.


---

[GitHub] activemq-artemis pull request #1787: ARTEMIS-1577 Address-settings policies ...

[clebertsuconic]

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

https://github.com/apache/activemq-artemis/pull/1787#discussion_r162353710
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
 ---
@@ -736,11 +736,11 @@ public RoutingStatus route(final Message message,
  throw new IllegalStateException("Message cannot be routed more 
than once");
   }
 
-  setPagingStore(message);
+  setPagingStore(context.getAddress(message), message);
 
   AtomicBoolean startedTX = new AtomicBoolean(false);
 
-  final SimpleString address = message.getAddressSimpleString();
+  final SimpleString address = context.getAddress(message);
--- End diff --

I'm just going for the simple refactoring here.. every call to 
message.getAddressSimplString() to be calling context.getAddress()



---

[GitHub] activemq-artemis pull request #1787: ARTEMIS-1577 Address-settings policies ...

[clebertsuconic]

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

https://github.com/apache/activemq-artemis/pull/1787#discussion_r162353815
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
 ---
@@ -736,11 +736,11 @@ public RoutingStatus route(final Message message,
  throw new IllegalStateException("Message cannot be routed more 
than once");
   }
 
-  setPagingStore(message);
+  setPagingStore(context.getAddress(message), message);
 
   AtomicBoolean startedTX = new AtomicBoolean(false);
 
-  final SimpleString address = message.getAddressSimpleString();
+  final SimpleString address = context.getAddress(message);
--- End diff --

Your comment could apply to the previous version as well.. I didn't make 
any big changes here. nor did want to risk anything else.


---

[GitHub] activemq-artemis pull request #1787: ARTEMIS-1577 Address-settings policies ...

[michaelandrepearce]

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

https://github.com/apache/activemq-artemis/pull/1787#discussion_r162261691
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
 ---
@@ -1284,7 +1284,7 @@ private boolean checkDuplicateID(final Message 
message,
  boolean isDuplicate = false;
 
  if (duplicateIDBytes != null) {
-cache = getDuplicateIDCache(message.getAddressSimpleString());
+cache = getDuplicateIDCache(context.getAddress(message));
--- End diff --

Same as other comment, re making one call to context.getAddress(message) 
and re-using the result


---

[GitHub] activemq-artemis pull request #1787: ARTEMIS-1577 Address-settings policies ...

[michaelandrepearce]

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

https://github.com/apache/activemq-artemis/pull/1787#discussion_r162261502
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java
 ---
@@ -736,11 +736,11 @@ public RoutingStatus route(final Message message,
  throw new IllegalStateException("Message cannot be routed more 
than once");
   }
 
-  setPagingStore(message);
+  setPagingStore(context.getAddress(message), message);
 
   AtomicBoolean startedTX = new AtomicBoolean(false);
 
-  final SimpleString address = message.getAddressSimpleString();
+  final SimpleString address = context.getAddress(message);
--- End diff --

Can we not make the call to context.getAddress just the once, and re-use 
the result throughout the method call?


---

[GitHub] activemq-artemis pull request #1787: ARTEMIS-1577 Address-settings policies ...

[clebertsuconic]

GitHub user clebertsuconic opened a pull request:

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

ARTEMIS-1577 Address-settings policies not working with older clients

This closes #1744

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

$ git pull https://github.com/clebertsuconic/activemq-artemis compatibility

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

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


commit fcf5bfb3feb2708b0e3ddcae157efd5194fbb5f2
Author: Clebert Suconic 
Date:   2018-01-17T20:03:59Z

ARTEMIS-1577 Address-settings policies not working with older clients

This closes #1744




---