[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
