You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/01/12 22:20:57 UTC
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
clebertsuconic opened a new pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406#discussion_r556335712
##########
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 {
+ final int MSG_SIZE = 1000;
+ final StringBuilder builder = new StringBuilder();
+ for (int i = 0; i < MSG_SIZE; i++) {
+ builder.append('0');
+ }
+ final String data = builder.toString();
+ final int MSG_COUNT = 3_000;
+
+ // sending size to explod max size
+ server.getPagingManager().addSize((int) ((PagingManagerImpl) server.getPagingManager()).getMaxSize());
+ server.getPagingManager().addSize(100_000);
+
+ server.getAddressSettingsRepository().addMatch("blockedQueue", new AddressSettings().setAddressFullMessagePolicy(AddressFullMessagePolicy.BLOCK));
+
+ ConnectionFactory factory = CFUtil.createConnectionFactory("AMQP", "tcp://localhost:5672");
Review comment:
Yes. Just to be sure.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406#discussion_r556227375
##########
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:
I'm looking for feedback on this error message here. any edit to make it more understandable would be appreciated.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406#discussion_r556236332
##########
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 {
+ final int MSG_SIZE = 1000;
+ final StringBuilder builder = new StringBuilder();
+ for (int i = 0; i < MSG_SIZE; i++) {
+ builder.append('0');
+ }
+ final String data = builder.toString();
+ final int MSG_COUNT = 3_000;
+
+ // sending size to explod max size
+ server.getPagingManager().addSize((int) ((PagingManagerImpl) server.getPagingManager()).getMaxSize());
+ server.getPagingManager().addSize(100_000);
+
+ server.getAddressSettingsRepository().addMatch("blockedQueue", new AddressSettings().setAddressFullMessagePolicy(AddressFullMessagePolicy.BLOCK));
+
+ ConnectionFactory factory = CFUtil.createConnectionFactory("AMQP", "tcp://localhost:5672");
Review comment:
this is a problem exclusive to AMQP. in AMQP the credit is per link, while in openwork and core it's per address..
Do you mean to run the same test with other protocols? is that what you asked?
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406#issuecomment-759712373
@gemmellr Thanks as usual!
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406#discussion_r556236332
##########
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 {
+ final int MSG_SIZE = 1000;
+ final StringBuilder builder = new StringBuilder();
+ for (int i = 0; i < MSG_SIZE; i++) {
+ builder.append('0');
+ }
+ final String data = builder.toString();
+ final int MSG_COUNT = 3_000;
+
+ // sending size to explod max size
+ server.getPagingManager().addSize((int) ((PagingManagerImpl) server.getPagingManager()).getMaxSize());
+ server.getPagingManager().addSize(100_000);
+
+ server.getAddressSettingsRepository().addMatch("blockedQueue", new AddressSettings().setAddressFullMessagePolicy(AddressFullMessagePolicy.BLOCK));
+
+ ConnectionFactory factory = CFUtil.createConnectionFactory("AMQP", "tcp://localhost:5672");
Review comment:
this is a problem exclusive to AMQP. in AMQP the credit is per link, while in openwire and core it's per address..
Do you mean to run the same test with other protocols? is that what you asked?
##########
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 {
+ final int MSG_SIZE = 1000;
+ final StringBuilder builder = new StringBuilder();
+ for (int i = 0; i < MSG_SIZE; i++) {
+ builder.append('0');
+ }
+ final String data = builder.toString();
+ final int MSG_COUNT = 3_000;
+
+ // sending size to explod max size
+ server.getPagingManager().addSize((int) ((PagingManagerImpl) server.getPagingManager()).getMaxSize());
+ server.getPagingManager().addSize(100_000);
+
+ server.getAddressSettingsRepository().addMatch("blockedQueue", new AddressSettings().setAddressFullMessagePolicy(AddressFullMessagePolicy.BLOCK));
+
+ ConnectionFactory factory = CFUtil.createConnectionFactory("AMQP", "tcp://localhost:5672");
Review comment:
ok, will do later today.. thanks for the review
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406#discussion_r556592747
##########
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:
I thought about eventually extending the check into some of the sizes.. that's why I kept PageStore originally.
As the idea evolved I agree I don't need the pageStore any more.. I will take it out.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406#discussion_r556799978
##########
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:
I'm storing the addresses so we can print them properly in case there's a clash. An architect would have a lot of trouble finding the proper producer without knowing the name where the issue occurred.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406#discussion_r556236455
##########
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:
@michaelandrepearce as I have your attention, can you review this error message? or is it fine as is?
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] asfgit closed pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
Posted by 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
[GitHub] [activemq-artemis] michaelandrepearce commented on pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406#issuecomment-759171401
I assume this either can occur in core and openwire also? Or is it already solved there? If already solved can you point us to how so can compare solutions in review. If it isnt shouldnt the solution be generic for all protocols? Not just amqp
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406#discussion_r556335902
##########
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 made sense to me when i read it.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3406: ARTEMIS-3065 AMQP Anonymous producer would eventually block
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3406:
URL: https://github.com/apache/activemq-artemis/pull/3406#discussion_r556232797
##########
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 {
+ final int MSG_SIZE = 1000;
+ final StringBuilder builder = new StringBuilder();
+ for (int i = 0; i < MSG_SIZE; i++) {
+ builder.append('0');
+ }
+ final String data = builder.toString();
+ final int MSG_COUNT = 3_000;
+
+ // sending size to explod max size
+ server.getPagingManager().addSize((int) ((PagingManagerImpl) server.getPagingManager()).getMaxSize());
+ server.getPagingManager().addSize(100_000);
+
+ server.getAddressSettingsRepository().addMatch("blockedQueue", new AddressSettings().setAddressFullMessagePolicy(AddressFullMessagePolicy.BLOCK));
+
+ ConnectionFactory factory = CFUtil.createConnectionFactory("AMQP", "tcp://localhost:5672");
Review comment:
Can we make this test more generic as its already using just jms api and test the other protocols switching the connection factories to test them for this problem as we do in some other tests.
----------------------------------------------------------------
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