sklochkov2 commented on PR #19754: URL: https://github.com/apache/druid/pull/19754#issuecomment-5192355421
> > @clintropolis what's your opinion on these two migration implementations? > > Heya, sorry have been busy with a bunch of other stuff and haven't had much time to look at this. First off, rad that you've been running this patch in production, that feels like useful information regardless of which direction we go in here so thanks for making this PR, mine has not really been run in production, i've just done some smoke testing on my laptop and got all the embedded tests to pass. That codex assessment dug in way deeper than I did when i first saw this shortly after it was opened, at the time to me the main differences (besides netty 4.1 vs 4.2) were that this branch has the custom timer stuff, but missing the explicit read after handleResponse thing to deal with split chunked responses (and at the time i first looked at it, was missing a bunch of the fixes that codex had also spotted on my PR). > > I do think that we'll want to upgrade netty to 4.2 eventually, but I'm also not particularly attached to my PR, so @sklochkov if you would like to take a stab at unifying our two branches I would be happy to review (maybe a fresh PR would be good?), else I'll probably get back to my branch sometime in the next week or 2. The suggested consolidation by codex seems pretty reasonable, i don't have a good handle on the byte[] vs ref-counting thing so not sure what would be best to do with that. I agree that getting to Netty 4.2 is the eventual goal; however, I only ran the netty 4.1 version in production, and it is still a major improvement over the long-discontinued 3.x branch. One thing I did *not* perform is comprehensive load testing though - in our case, the performance just needs to be acceptable, and it stayed so after patching. If you have any kind of repeatable load tests (e. g. as part of the Imply internal tooling), it would be awesome if you could run it against both patchsets to provide an idea of the relative performance impact. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
