attilapiros commented on pull request #31942: URL: https://github.com/apache/spark/pull/31942#issuecomment-809322175
Although this little fix itself looks good for me **but** I think this is a great opportunity to prove its validity with some unit tests. Moreover I agree with @srowen regarding the similar cases: those should be taken care of too. ### How would I test this? Behind this `ManagedBuffer` there is a `NettyManagedBuffer` which is the only managed buffer where `retain` and `release` *could have any* real effect (as `NioManagedBuffer` and `FileSegmentManagedBuffer` are garbage collected). On Netty's documentation there is complete page about reference counting with a section about [troubleshooting / detection of leaks](https://netty.io/wiki/reference-counted-objects.html#troubleshooting-buffer-leaks). So I suggest to write some unit tests (extend `TransportResponseHandlerSuite`) to check those cases along with **io.netty.leakDetection.level=PARANOID** without any of the fixes we should see the leaks in the logs. I know it is not a real assert but latter we might find a way to make the test fail either by extra code or by analyzing the logs with some scripts after test runs... (I would say it is out of scope now). Right now it is enough if the logs are shared with us (both before and after the fix). This way we could be 100% sure the fixes are valid. -- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
