[
https://issues.apache.org/jira/browse/ARTEMIS-3065?focusedWorklogId=535360&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-535360
]
ASF GitHub Bot logged work on ARTEMIS-3065:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 13/Jan/21 12:48
Start Date: 13/Jan/21 12:48
Worklog Time Spent: 10m
Work Description: 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:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 535360)
Time Spent: 1h 40m (was: 1.5h)
> AMQP Anonymous Producer will block on global max page when using only paged
> messages
> ------------------------------------------------------------------------------------
>
> Key: ARTEMIS-3065
> URL: https://issues.apache.org/jira/browse/ARTEMIS-3065
> Project: ActiveMQ Artemis
> Issue Type: Bug
> Affects Versions: 2.16.0
> Reporter: Clebert Suconic
> Assignee: Clebert Suconic
> Priority: Major
> Fix For: 2.17.0
>
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> This is in between an improvement and a bug fix...
>
> When sending message to a AMQP anonymous producer, and all the messages on
> the producer have full address as paged, will still block.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
