clintropolis commented on PR #19754: URL: https://github.com/apache/druid/pull/19754#issuecomment-5335178209
>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. Eh, my plans were about the same, deploy to some internal environments to monitor and try to bake it a bit before the release to make sure nothing funny happens and fix it if does, since I'm not aware of anything we have handy that would be good for measuring this part in particular. >missing the explicit read after handleResponse thing to deal with split chunked responses I dug into it a bit, and it turns out that this extra `ctx.read()` in my PR wasn't actually needed, rather claude likely stuck it in there fixing one of the other bugs which was causing a hang in the tests and it didn't hurt anything tests covered and so got counted as an important part of fixing things, which I guess I probably should have checked a bit closer before trusting things 😅 I should be able to get back to my PR sometime soon to try to fold in some of the changes here per the above recommendations. -- 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]
