cshannon commented on PR #928: URL: https://github.com/apache/activemq/pull/928#issuecomment-1304835694
@lucastetreault - The main issue here was the memory leak with durable subscriptions closing and coming back online with new consumers. This could have been fixed of course and your original fix probably would have been ok to keep. But besides the issue with the memory leak, I went with a different approach for 1 main reason which I thought was important and a secondary reason. 1. The primary reason I decided to go with the new approach is my implementation now avoids having to track duplicate metadata. Since the information is already tracked in the `ManagedRegionBroker` (since it extends `RegionBroker`) in the different `Regions` objects and well tested for a long time it made more sense to me to just use the existing maps so we don't have to worry about the extra logic of trying to track new maps/metadata and verifying things will work and are kept in sync since it's already there and tested. I saw zero benefit of attempting to track something again that we already track and can easily look up and only drawbacks in terms of now something else to keep in sync and update and things like memory leaks which happened. 2. A more minor secondary thing was you made the assumption that there is always a 1 to 1 mapping of ConsumerInfo to subscriptions. I think this is minor because this is usually the case (and I'd have to check but I think is always be the case now) even with composite destinations which is why the composite destination test works since it shares a subscription. However, in theory, this doesn't have to be which is why the original code was looking through all subscriptions for a matching ConsumerInfo and my approach preserves that by finding all subscriptions that match in the different Regions. The Regions implementation uses a 1 to 1 mapping of consumer id to subs but it also checks each region and if for some reason the code in the different regions ever associated more than one Consumer to subscription the new approach would still work. In regards to the tests passing, it's because the main issue was the memory leak and my test class of course doesn't try and check the map that had the memory leak (`consumerSubscriptionMap`) since it no longer exists in my PR. If I run my test against your original fix and update the `testDurableConsumerRemoval` test to check that `consumerSubscriptionMap ` is size zero at the end after all consumers close (which it should be), it's not and is still 1000 and the test fails. I assume of course it would be fine if you fixed that memory leak. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
