zentol commented on PR #20003: URL: https://github.com/apache/flink/pull/20003#issuecomment-1160865958
> Currently, the FlinkUserClassLoader and SafetyNetWrapperClassLoader already exposed addURL method Which was added 3 few weeks ago _by the same contributor_. What kind of justification is that? What you are doing here is poking a hole into some delicate assumptions about classloading that we could make so far. As of right now, _every_ classloader in Flink is deterministic. If you could load a class once you can do it again, and if 2 classes bundle the same class then there is a well-defined order in which they are loaded from. This PR changes that. A class _may not_ be loaded again a second time because some jar added in the mean time may have introduced a conflict. A class _may_ suddenly be loaded from a different jar than all the related classes before that. Which I might add will be a _nightmare_ to debug. > Why do we have to take a totally different and error-prone way to support the same thing? I'm sympathetic to the concern that having to call setters in all these components is error-prone, but there are different ways to go about this. You're only considering options where these components have direct access to the CL; but that's not necessarily required. A simple wrapper around a CL that provides a CL-like interface to the components, which can be mutated from the TableEnvironment (or wherever) would allow you to add additional jars by expanding the CL tree, _without_ breaking assumptions we make about how class-loading works, while _also_ limiting the ability to mutate the CL to the table API. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
