[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313765154 ## File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/TemporaryDestinationTest.java ## @@ -266,6 +266,24 @@ public void testForTempQueueTargetInfosSizeLimit() throws Exception { } } + @Test + public void testForKnownAddressSizeLimit() throws Exception { + try { + conn = createConnection(); + Session s = conn.createSession(false, Session.AUTO_ACKNOWLEDGE); + for (int i = 0; i < 200; i++) { +TemporaryQueue temporaryQueue = s.createTemporaryQueue(); +MessageProducer producer = s.createProducer(temporaryQueue); +producer.send(s.createMessage()); + } + assertTrue(((ActiveMQConnection)conn).getKnownDestinations().size() <= 100); Review comment: ive very quickly tried to code up what im trying to explain here as an alternative approach: https://github.com/apache/activemq-artemis/pull/2796 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313765154 ## File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/TemporaryDestinationTest.java ## @@ -266,6 +266,24 @@ public void testForTempQueueTargetInfosSizeLimit() throws Exception { } } + @Test + public void testForKnownAddressSizeLimit() throws Exception { + try { + conn = createConnection(); + Session s = conn.createSession(false, Session.AUTO_ACKNOWLEDGE); + for (int i = 0; i < 200; i++) { +TemporaryQueue temporaryQueue = s.createTemporaryQueue(); +MessageProducer producer = s.createProducer(temporaryQueue); +producer.send(s.createMessage()); + } + assertTrue(((ActiveMQConnection)conn).getKnownDestinations().size() <= 100); Review comment: ive very quickly tried to code up what im trying to explain here: https://github.com/apache/activemq-artemis/pull/2796 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313740128 ## File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/TemporaryDestinationTest.java ## @@ -266,6 +266,24 @@ public void testForTempQueueTargetInfosSizeLimit() throws Exception { } } + @Test + public void testForKnownAddressSizeLimit() throws Exception { + try { + conn = createConnection(); + Session s = conn.createSession(false, Session.AUTO_ACKNOWLEDGE); + for (int i = 0; i < 200; i++) { +TemporaryQueue temporaryQueue = s.createTemporaryQueue(); +MessageProducer producer = s.createProducer(temporaryQueue); +producer.send(s.createMessage()); + } + assertTrue(((ActiveMQConnection)conn).getKnownDestinations().size() <= 100); Review comment: similar could be done for method void checkDestination(ActiveMQDestination destination) throws JMSException just check if the connection was the one that created the destination object, and then inside the destination object hold a flag if its been created broker side or not (which is what the cache was doing) 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313740128 ## File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/TemporaryDestinationTest.java ## @@ -266,6 +266,24 @@ public void testForTempQueueTargetInfosSizeLimit() throws Exception { } } + @Test + public void testForKnownAddressSizeLimit() throws Exception { + try { + conn = createConnection(); + Session s = conn.createSession(false, Session.AUTO_ACKNOWLEDGE); + for (int i = 0; i < 200; i++) { +TemporaryQueue temporaryQueue = s.createTemporaryQueue(); +MessageProducer producer = s.createProducer(temporaryQueue); +producer.send(s.createMessage()); + } + assertTrue(((ActiveMQConnection)conn).getKnownDestinations().size() <= 100); Review comment: similar could be done for method void checkDestination(ActiveMQDestination destination) throws JMSException just check if the connection was the one that created the destination object, and thus remove need for cache 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313739151 ## File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/TemporaryDestinationTest.java ## @@ -266,6 +266,24 @@ public void testForTempQueueTargetInfosSizeLimit() throws Exception { } } + @Test + public void testForKnownAddressSizeLimit() throws Exception { + try { + conn = createConnection(); + Session s = conn.createSession(false, Session.AUTO_ACKNOWLEDGE); + for (int i = 0; i < 200; i++) { +TemporaryQueue temporaryQueue = s.createTemporaryQueue(); +MessageProducer producer = s.createProducer(temporaryQueue); +producer.send(s.createMessage()); + } + assertTrue(((ActiveMQConnection)conn).getKnownDestinations().size() <= 100); Review comment: it seems we already have session inside destination as such this would be relatively easy. expose the session on ActiveMQDestination e.g. ``` public ActiveMQSession getSession() { return session; } ``` And then in this check: ``` if (jbdest.isTemporary() && (jbdest.getSession() == null || connection != jbdest.getSession().getConnection())) { ``` lastly just clean up bits that we adding and removing temp address into the cache, as redundent. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313739151 ## File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/TemporaryDestinationTest.java ## @@ -266,6 +266,24 @@ public void testForTempQueueTargetInfosSizeLimit() throws Exception { } } + @Test + public void testForKnownAddressSizeLimit() throws Exception { + try { + conn = createConnection(); + Session s = conn.createSession(false, Session.AUTO_ACKNOWLEDGE); + for (int i = 0; i < 200; i++) { +TemporaryQueue temporaryQueue = s.createTemporaryQueue(); +MessageProducer producer = s.createProducer(temporaryQueue); +producer.send(s.createMessage()); + } + assertTrue(((ActiveMQConnection)conn).getKnownDestinations().size() <= 100); Review comment: it seems we already have session inside destination as such this would be relatively easy. expose the session on ActiveMQDestination e.g. ``` public ActiveMQSession session() { return session; } ``` And then in this check: ``` if (jbdest.isTemporary() && (jbdest.session() == null || connection != jbdest.session().getConnection())) { ``` lastly just clean up bits that we adding and removing temp address into the cache, as redundent. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313734672 ## File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/TemporaryDestinationTest.java ## @@ -266,6 +266,24 @@ public void testForTempQueueTargetInfosSizeLimit() throws Exception { } } + @Test + public void testForKnownAddressSizeLimit() throws Exception { + try { + conn = createConnection(); + Session s = conn.createSession(false, Session.AUTO_ACKNOWLEDGE); + for (int i = 0; i < 200; i++) { +TemporaryQueue temporaryQueue = s.createTemporaryQueue(); +MessageProducer producer = s.createProducer(temporaryQueue); +producer.send(s.createMessage()); + } + assertTrue(((ActiveMQConnection)conn).getKnownDestinations().size() <= 100); Review comment: An alternative solution to this problem is remove knownDestination entirely, and have a reference to the session & connection in the Destination itself, and then it will no longer be needed, as we can do the check directly on the destination, e.g. something like ` jbdest.getConnection == connection` Removing the need for the set / cache for this check 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313734672 ## File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/TemporaryDestinationTest.java ## @@ -266,6 +266,24 @@ public void testForTempQueueTargetInfosSizeLimit() throws Exception { } } + @Test + public void testForKnownAddressSizeLimit() throws Exception { + try { + conn = createConnection(); + Session s = conn.createSession(false, Session.AUTO_ACKNOWLEDGE); + for (int i = 0; i < 200; i++) { +TemporaryQueue temporaryQueue = s.createTemporaryQueue(); +MessageProducer producer = s.createProducer(temporaryQueue); +producer.send(s.createMessage()); + } + assertTrue(((ActiveMQConnection)conn).getKnownDestinations().size() <= 100); Review comment: An alternative solution to this problem is remove knownDestination entirely, and have a reference to the session & connection in the Destination itself, and then it will no longer be needed, as we can do the check directly on the destination, e.g. something like ` jbdest.getConnection == connection` Removing the need for the set 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313731994 ## File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/TemporaryDestinationTest.java ## @@ -266,6 +266,24 @@ public void testForTempQueueTargetInfosSizeLimit() throws Exception { } } + @Test + public void testForKnownAddressSizeLimit() throws Exception { + try { + conn = createConnection(); + Session s = conn.createSession(false, Session.AUTO_ACKNOWLEDGE); + for (int i = 0; i < 200; i++) { +TemporaryQueue temporaryQueue = s.createTemporaryQueue(); +MessageProducer producer = s.createProducer(temporaryQueue); +producer.send(s.createMessage()); + } + assertTrue(((ActiveMQConnection)conn).getKnownDestinations().size() <= 100); Review comment: e.g. where you make a temp dest for a reply from for other services, that is set in the replyto on the produced message, and then setup the consumer to listen to, currently if > 100 other services it would need to co-ordinate with, it will now break, due to the below check, in session which will break the ability to make a consumer for temp destination (when more than100) if (jbdest.isTemporary() && !connection.containsTemporaryQueue(jbdest.getSimpleAddress())) { throw new JMSException("Can not create consumer for temporary destination " + destination + " from another JMS connection"); } 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313731994 ## File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/TemporaryDestinationTest.java ## @@ -266,6 +266,24 @@ public void testForTempQueueTargetInfosSizeLimit() throws Exception { } } + @Test + public void testForKnownAddressSizeLimit() throws Exception { + try { + conn = createConnection(); + Session s = conn.createSession(false, Session.AUTO_ACKNOWLEDGE); + for (int i = 0; i < 200; i++) { +TemporaryQueue temporaryQueue = s.createTemporaryQueue(); +MessageProducer producer = s.createProducer(temporaryQueue); +producer.send(s.createMessage()); + } + assertTrue(((ActiveMQConnection)conn).getKnownDestinations().size() <= 100); Review comment: e.g. where you make a temp dest for a reply from for other services, that is set in the replyto on the produced message, and then setup the consumer to listen to, currently if > 100 other services it would need to co-ordinate with, it will now break, due to the below check, in session which will break the ability to make a consumer for temp destination (when more the 100) if (jbdest.isTemporary() && !connection.containsTemporaryQueue(jbdest.getSimpleAddress())) { throw new JMSException("Can not create consumer for temporary destination " + destination + " from another JMS connection"); } 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313731994 ## File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/TemporaryDestinationTest.java ## @@ -266,6 +266,24 @@ public void testForTempQueueTargetInfosSizeLimit() throws Exception { } } + @Test + public void testForKnownAddressSizeLimit() throws Exception { + try { + conn = createConnection(); + Session s = conn.createSession(false, Session.AUTO_ACKNOWLEDGE); + for (int i = 0; i < 200; i++) { +TemporaryQueue temporaryQueue = s.createTemporaryQueue(); +MessageProducer producer = s.createProducer(temporaryQueue); +producer.send(s.createMessage()); + } + assertTrue(((ActiveMQConnection)conn).getKnownDestinations().size() <= 100); Review comment: e.g. where you make a temp dest for a reply from for other services, that is set in the replyto on the produced message, and then setup the consumer to listen to, currently if > 100 other services it would need to co-ordinate with, it will now break. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313731309 ## File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/TemporaryDestinationTest.java ## @@ -266,6 +266,24 @@ public void testForTempQueueTargetInfosSizeLimit() throws Exception { } } + @Test + public void testForKnownAddressSizeLimit() throws Exception { + try { + conn = createConnection(); + Session s = conn.createSession(false, Session.AUTO_ACKNOWLEDGE); + for (int i = 0; i < 200; i++) { +TemporaryQueue temporaryQueue = s.createTemporaryQueue(); +MessageProducer producer = s.createProducer(temporaryQueue); +producer.send(s.createMessage()); + } + assertTrue(((ActiveMQConnection)conn).getKnownDestinations().size() <= 100); Review comment: Need a test case where a temporary dest is created first and then later consumers are created on it, currently with this change the existing behaviour which allows this and will mean on create consumer for the temp dest will throw error as will be not a known dest. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313730591 ## File path: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQConnection.java ## @@ -97,7 +98,7 @@ private final Set tempQueues = new ConcurrentHashSet<>(); - private final Set knownDestinations = new ConcurrentHashSet<>(); + private final ConcurrentMaxSizeCache knownDestinations = new ConcurrentMaxSizeCache<>(100); Review comment: this will actually cause issues if someone is using temp destinations, and has say more than 100 legitimately. and creates the consumers later, as the consumer will check it its known, which now it wont and will throw an error. This will at least mean people can increase that setting if they need. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache
michaelandrepearce commented on a change in pull request #2794: ARTEMIS-2451 limit size of destination cache URL: https://github.com/apache/activemq-artemis/pull/2794#discussion_r313726708 ## File path: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQConnection.java ## @@ -97,7 +98,7 @@ private final Set tempQueues = new ConcurrentHashSet<>(); - private final Set knownDestinations = new ConcurrentHashSet<>(); + private final ConcurrentMaxSizeCache knownDestinations = new ConcurrentMaxSizeCache<>(100); Review comment: Can we make this configurable (e.g. a setting in the CF), 100 is a bit of a magic number. 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: us...@infra.apache.org With regards, Apache Git Services