Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARTEMIS-2451 remove need for knownDestination Cache #2796

Closed
wants to merge 1 commit into from

Conversation

@michaelandrepearce
Copy link
Contributor

commented Aug 14, 2019

@jbertram this is partially what i was trying to explain as an alternative approach to simply remove the need for the cache #2794.

I assume you've hit some client side memory leak issue, that you're trying to resolve

@jbertram

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

At first glance this looks like a clever solution and better than the one I came up with. Let me review it in more detail.

@@ -227,45 +228,6 @@ public void testForTempQueueCleanerUpperLeak() throws Exception {
}
}

@Test

This comment has been minimized.

Copy link
@jbertram

jbertram Aug 14, 2019

Contributor

These tests are still valid and shouldn't be removed. They are testing the server-side accumulation of address info. The changes in this PR are for the client-side.

@@ -81,7 +82,7 @@ public void testTemporaryQueueLeak() throws Exception {

tempQueue.delete();

assertFalse(conn.containsKnownDestination(SimpleString.toSimpleString(tempQueue.getQueueName())));
assertFalse(((ActiveMQDestination) tempQueue).isCreated());

This comment has been minimized.

Copy link
@jbertram

jbertram Aug 14, 2019

Contributor

This test fails because ActiveMQDestination.delete() doesn't call setCreated(false).

import org.apache.activemq.artemis.jms.client.ActiveMQTemporaryTopic;
import org.apache.activemq.artemis.jms.client.ActiveMQTopic;
import org.apache.activemq.artemis.tests.util.Wait;
import org.apache.activemq.artemis.jms.client.*;

This comment has been minimized.

Copy link
@jbertram

jbertram Aug 14, 2019

Contributor

Star imports are a check-style violation.

import org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory;
import org.apache.activemq.artemis.jms.client.ActiveMQQueue;
import org.apache.activemq.artemis.jms.client.ActiveMQTopic;
import org.apache.activemq.artemis.jms.client.*;

This comment has been minimized.

Copy link
@jbertram

jbertram Aug 14, 2019

Contributor

Star imports are a check-style violation.

@jbertram

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

BTW, the issue I saw wasn't a leak, per se. It was simply unwanted accumulation based on the way it was designed.

@michaelandrepearce

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

@jbertram this was a quick scrap togeather of what i was trying to describe in review comment on your pr. Im happy to complete it fully for you, but it wont be till next week. Im more than happy if you need this done faster and wish to just take it and complete it (e.g. dont worry about keeping my authorship)

@michaelandrepearce

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

@jbertram

re:

BTW, the issue I saw wasn't a leak, per se. It was simply unwanted accumulation based on the way it was designed.

Won't we get similar issue on the tempoary queue set thats used to track temp queues in connection? I guess that could probably move into the session object, so theyre cleaned up as a session is closed, which is anyhow the lifetime of a temporary queue anyhow.

@jbertram

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

It's possible that tempQueues could grow like knownDestinations. However, entries are only added to tempQueues for temporary destinations that the client actually creates and entries from tempQueues get removed when the corresponding temporary destination is deleted from the broker. An entry is added to knownDestinations for every unique destination to which a client sends a message and those entries never get removed unless they are temporary destinations which the client itself deletes. In my mind these facts make the pathological growth much less likely.

Also, in JMS terms the lifetime of a temporary destination is actually the lifetime of the connection that created it, not the session. The temporary destinations need to be tracked somehow so that the client implementation can delete them implicitly when the connection is closed in the case that the application doesn't delete them explicitly.

@jbertram

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

I just sent #2800 which is based on your work here, @michaelandrepearce.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.