[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2449 > It seems yours is a refactoring while mine had functionality involved.. hence I'm asking you to defer it Yep, got it :+1: > Adding your change in top of mine added failues (the one you sent). I see 2 reasons for that: - the context::clear that I have added is wrong - there are other cases where we need to clear the context that are not correct handled (due 2 my changes!) - ---
[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2449 @franz1981 the failures I mentioned are on my branch. I am asking you to defer your PR until after I merge mine on AMQP performance. This route change I'm making is a bit more functional than yours. It seems yours is a refactoring while mine had functionality involved.. hence I'm asking you to defer it Adding your change in top of mine added failues (the one you sent). ---
[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2449 @jbertram TBH I haven't had the chance to measure it as it deserves, but I have found that it produces much less garbage (several MB/sec). I hope to find some time to do a fair comparison with master ---
[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2449 @clebertsuconic I have tun the CI and it seems to work has expected (no additional failures), it's strange :O ---
[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2449 @franz1981 your changes (on my branch) are causing a few failures. I really like your optimizations here. but can you close this. .and resend once I'm done? ---
[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...
Github user jbertram commented on the issue: https://github.com/apache/activemq-artemis/pull/2449 @franz1981, I'd still love to see numbers whenever you get the chance to post them. :+1: ---
[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2449 @franz1981 close this please? it will cause me a lot of headache if someone merged it before my branch? I'm pulling your change with my PR. ---
[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2449 I have verified that the code isn't changing the current behaviour: the only failures I see are the same intermittent ones we have on CI (on master). ---
[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2449 @clebertsuconic No worries I can just wait to review the PR that you will send too, so we keep separate the changes. Or you are curious to see if it helps your part too? ---
[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2449 @franz1981 can you hold this? or better.. can you send these optimizations into my new-amqp branch? I have optimized routing to cache it.. and it would take me longer to refactor your changes in than yourself doing it. ---
[GitHub] activemq-artemis issue #2449: ARTEMIS-2191 Makes PostOfficeImpl::route commo...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2449 I will provide soon some number to justify these changes from the perf pov: from code quality pov it is a refactoring to make the code simpler to read in methods that are supposed to be called very often. The 2 commits are be left there just to simplify the code review: I will squash them into one if it will be approved. @michaelandrepearce @clebertsuconic Feel free to review it :+1: ---
