lucastetreault commented on PR #928: URL: https://github.com/apache/activemq/pull/928#issuecomment-1304744663
Hey @cshannon, I had a chance to look at this today. I ran the example app against a build including this change and the performance seems good 👍 I was trying to understand what I missed in my original changes. I ran the tests you added without your changes to ManagedRegionBroker (e.g.: https://github.com/apache/activemq/compare/main...lucastetreault:activemq:AMQ-9107-test) expecting failures but all the tests are passing even with the addition of checking that `consumerSubscriptionMap` is empty. Can you help me understand what I'm missing? As for the memory leak, it seems I missed one spot where I should have been removing from consumerSubscriptionMap in `public void unregisterSubscription(Subscription sub)` around [line 300](https://github.com/apache/activemq/compare/main...lucastetreault:activemq:AMQ-9107-test#diff-17798bd5f1fa8141e349c916704b7f1d7ff660f02bd3582ef696344a5bb3c723R300). I added a quick and dirty test [here](https://github.com/apache/activemq/compare/main...lucastetreault:activemq:AMQ-9107-test#diff-aa152194070293f1fb21ac1c45654d8041aa45cc3a6770aae3b3fd85961db08fR175) that passes now. -- 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]
