gharris1727 commented on PR #12955: URL: https://github.com/apache/kafka/pull/12955#issuecomment-1339870223
@OmniaGM I believe that failure is flaky, as I cannot reproduce it locally. Those sorts of flaky failures are what got me looking at this code in the first place, coincidentally. @C0urante Thanks for your suggestion on making these sorts of leaks more difficult, given the complexity and number of resources being managed by the connector/task instances, that we should certainly look into it. I thought about the `Mirror*Config::close` solution that you proposed, and didn't like the idea of the config container being also responsible for managing the lifetime of the objects it creates. I didn't like it being responsible for creating the resources either, as you pointed out: it's too easy to leak the reference. I experimented with an alternative solution, to have a reusable (across task/connector classes) BackgroundResources class which handles all instantiation and cleanup of the AutoClosable resources used in an MM2 connector/task. It simplifies the `stop() methods, and doesn't appear to incur too much boilerplate in the `start()` methods, maybe with a shorter name it could be even better. I have two concerns here: 1. This change may be scope-creep for this PR, and maybe better handled as a lateral refactor after the release blocker bug is resolved. I am happy to continue iterating on the wider solution in this PR or a separate one, so let me know if you think the scope is still reasonable. 2. I wonder if the BackgroundResources class is in danger of becoming a "god object" which does everything. At the moment everything passes checkstyle, but if we add too many other pluggable interfaces or closeable resources, this class could be doing too much. -- 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]
