[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block

[GitBox <gi...@apache.org>]

gemmellr commented on a change in pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406#discussion_r556398482



##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/paging/AmqpPagingTest.java
##########
@@ -118,4 +128,49 @@ public void testPaging() throws Exception {
       connection.close();
    }
 
+
+   @Test(timeout = 60000)
+   public void testNotBlockOnGlobalMaxSize() throws Exception {

Review comment:
       Perhaps "testNotBlockOnGlobalMaxSizeWithAnonymousProducer"? Gives the name more clarity for later, since the anonymous producer usage is critical to the changes/behaviours under test.

##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/logger/ActiveMQAMQPProtocolLogger.java
##########
@@ -66,4 +66,9 @@
       "\nSuccess on Server AMQP Connection {0} on {1} after {2} retries" +
       "\n*******************************************************************************************************************************\n", format = Message.Format.MESSAGE_FORMAT)
    void successReconnect(String name, String hostAndPort, int currentRetry);
+
+   @LogMessage(level = Logger.Level.WARN)
+   @Message(id = 111004, value = "An anonymous producer is sending messages on destination {0} and {1} while their AddressFullPolicies are clashing with each other. This could lead to inconsistencies between blocking and paging at your client sender. Notice you could have other occurencies of this scenario but this message is printed only once per anonymous client sender.",
+      format = Message.Format.MESSAGE_FORMAT)
+   void incompatibleAddressSettingsOnAnonymousProducer(String oldAddress, String newAddress);

Review comment:
       The method name mentions  'incompatibleAddressSettings' but its specific to AddressFullPolicy so perhaps it should refer to only AddressFullPolicy rather than Settings.
   
   Also its called for any difference, but some differences aren't really incompatible/problematic, really the main issue is between BLOCK and anything else. Maybe it shouldnt always be called as it is now, or should be named 'different..' rather than 'incompatible..'.

##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java
##########
@@ -184,6 +193,19 @@ protected void actualDelivery(AMQPMessage message, Delivery delivery, Receiver r
       }
    }
 
+   private void validateAddressOnAnymousLink(AMQPMessage message) throws Exception {
+      SimpleString newAddress = message.getAddressSimpleString();
+      if (newAddress != null && !newAddress.equals(lastAddress)) {
+         PagingStore newPageStore = sessionSPI.getProtocolManager().getServer().getPagingManager().getPageStore(newAddress);
+         if (!addressAlreadyClashed && lastPageStore != null && lastPageStore.getAddressFullMessagePolicy() != newPageStore.getAddressFullMessagePolicy()) {

Review comment:
       Would it be nicer to only retain reference to the AddressFullMessagePolicy return value, rather than whole PageStore (and thus everything it refers to in turn), since thats all that seems to be of interest?

##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java
##########
@@ -184,6 +193,19 @@ protected void actualDelivery(AMQPMessage message, Delivery delivery, Receiver r
       }
    }
 
+   private void validateAddressOnAnymousLink(AMQPMessage message) throws Exception {
+      SimpleString newAddress = message.getAddressSimpleString();
+      if (newAddress != null && !newAddress.equals(lastAddress)) {
+         PagingStore newPageStore = sessionSPI.getProtocolManager().getServer().getPagingManager().getPageStore(newAddress);
+         if (!addressAlreadyClashed && lastPageStore != null && lastPageStore.getAddressFullMessagePolicy() != newPageStore.getAddressFullMessagePolicy()) {
+            addressAlreadyClashed = true; // print the warning only once

Review comment:
       Nitpicking, but its the policies differing that was important rather than the address not being the same, so maybe addressFullPolicyAlreadyDiffered?

##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java
##########
@@ -174,6 +179,10 @@ private RoutingType getRoutingType(Symbol[] symbols, SimpleString address) {
    protected void actualDelivery(AMQPMessage message, Delivery delivery, Receiver receiver, Transaction tx) {
       try {
          if (sessionSPI != null) {
+            // message could be null on unit tests (Mocking from ProtonServerReceiverContextTest).
+            if (address == null && message != null) {
+               validateAddressOnAnymousLink(message);

Review comment:
       Typo in method name, Anonymous.

##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/logger/ActiveMQAMQPProtocolLogger.java
##########
@@ -66,4 +66,9 @@
       "\nSuccess on Server AMQP Connection {0} on {1} after {2} retries" +
       "\n*******************************************************************************************************************************\n", format = Message.Format.MESSAGE_FORMAT)
    void successReconnect(String name, String hostAndPort, int currentRetry);
+
+   @LogMessage(level = Logger.Level.WARN)
+   @Message(id = 111004, value = "An anonymous producer is sending messages on destination {0} and {1} while their AddressFullPolicies are clashing with each other. This could lead to inconsistencies between blocking and paging at your client sender. Notice you could have other occurencies of this scenario but this message is printed only once per anonymous client sender.",

Review comment:
       It reads fine, though it somewhat suggests the difference involves paging and blocking, which it may not. Perhaps the precise policy values should be logged also for clarity?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org