bobpaulin commented on PR #10691: URL: https://github.com/apache/nifi/pull/10691#issuecomment-3702926937
Hi @exceptionfactory you are correct that I could replace ReflectionUtils with MethodUtils to invoke annotated methods without caching in strategic places that do not benefit from caching. I also agree that making that change in a few strategic locations could reduce the risk given how widely used this class is. I'm open to going in that direction in the short term. That said I'd like to lean in a bit to on these failures. There is a fundamental issue with using a WeakHashMap when the values (specifically the methods) contain references to the keys (the class). It effectively makes the WeakHashMap contain strong references which prevents it from serving it's intended purpose here. I've just rebased the code on the latest main and will also see if I can reproduce these failures locally to determine if there is in fact a logical problem or if perhaps there's something else going on. > Thanks for proposing this change @bobpaulin. The adjustment appears to be logical, but some sporadic test failures raise questions about unintended consequences. This might be an opportunity to consider switching some references to Apache Commons Lang3 `MethodUtils`, depending on whether certain uses should participate in the caching of Class and Method references. The framework uses this class in a number of places, so if there are only a few places where Method references should not be cached, it may be cleaner to switch to `MethodUtils`, instead of making this more fundamental change. -- 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]
