You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/06/17 09:18:32 UTC
[GitHub] [flink] lsyldliu opened a new pull request, #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
lsyldliu opened a new pull request, #20003:
URL: https://github.com/apache/flink/pull/20003
## What is the purpose of the change
In table module, we need an `URLClassLoader` which exposes the `addURL` method because we need to load jar dynamically in sql job. Although the SafetyNetWrapperClassLoader has exposed `addURL` method, but we can't ensure the classloader created by `FlinkUserCodeClassLoaders` is `SafetyNetWrapperClassLoader`, because the returned classloader might not be `SafetyNetWrapperClassLoader` if checkClassLoaderLeak is false. So we need introduce a `MutableURLClassLoader` that exposes the `addURL`, and the `SafetyNetWrapperClassLoader` and `FlinkUserCodeClassLoader` both extends it, we only need refer to this class in table module.
## Brief change log
- *Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader*
## Verifying this change
This change is already covered by existing tests, such as *FlinkUserCodeClassLoadersTest*.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: ( no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): ( no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
- The S3 file system connector: ( no)
## Documentation
- Does this pull request introduce a new feature? (no)
- If yes, how is the feature documented? (not documented)
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1158679157
cc @wuchong @zhuzhurk
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1160201207
Hi @zentol , we considered this option before. The problem is that the classloader is held by many components in table, e.g. `CatalogManger`, `FunctionCatalog`, `Planner`, and so on. It's hard to replace all the classloader references when adding a JAR URL. It's very easy to miss updating classloader in a certain component and cause problems.
That's why we prefer to introduce a `MutableURLClassLoader` which allows us to update the classloader in place.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1160459007
> That's quite a statement given that something external can just mutate the classloader under the hood without any these components being aware of it.
The classloader is managed and mutated by the `TableEnvironment` when receiving commands like `ADD JAR`, so the `TableEnvironment` is aware of it. Users don't have direct access to mutate the classloader.
> Since when is Spark SQL our measure for whether something is a good idea?
When Spark SQL is a bad idea? Why do we have to take a totally different and error-prone way to support the same thing?
Currently, the `FlinkUserClassLoader` and `SafetyNetWrapperClassLoader` already exposed `addURL` method. What this pull request wants to do is just provide a common parent class which have the access to `addURL`.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1161391220
> 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.
That's totally fine. If the classes are the same in different jars, then it should work. If the classes are different in jars, it will anyway throw class conflict (e.g. NoSuchMethod) exceptions during runtime. On the contrary, it is more friendly to fail fast before distributed execution.
Besides, there are no differences in this case when creating a new classloader to wrap the existing classloader. Creating a new FlinkUserClassloader with child-first mode will lead to the same class might be loaded from different jars. So it will have the same problem you mentioned. What's more, the same class will be loaded by different classloaders, there might be many class cast exceptions (X cannot be cast to X) **even if** the classes are in the same version.
Last but not least, it's important to keep the same classloader mechanism between local job compiling and distributed execution, e.g. Flink CLI, SQL CLI. Introducing a new classloader mechanism (each user jar in a new CL level) different from the existing one (all user jars are in the same CL level) will lead to inconsistent and unexpected behavior.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zentol commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1161787438
> What's more, the same class will be loaded by different classloaders, there might be many class cast exceptions (X cannot be cast to X) even if the classes are in the same version.
That depends on how the class-loading order is set up and how you actually use it.
If you load everything parent-first within the added sub-tree this problem will not occur.
I actually have to correct myself here; the addition of another jar should have no impact on the loading of a previously loaded class, because the URLClassLoader _should_ (because why not) access the jars in the order they were passed. Hence the most recently added jar is checked last. It should behave like parent-first within the sub-tree.
If we start removing URLs however this very much changes.
> Introducing a new classloader mechanism (each user jar in a new CL level) different from the existing one (all user jars are in the same CL level) will lead to inconsistent and unexpected behavior.
> it is more friendly to fail fast before distributed execution.
> it's important to keep the same classloader mechanism between local job compiling and distributed execution
These are good points, but doesn't that mean that the _full_ user CL should be built eagerly in the CLI before any user-code is called? (and forbid us from breaking that pattern)
Because that will be the actual behavior at runtime.
I.E., we collect all the jars we need, then build the user CL, and only then start using the jars.
Can you clarify on whether the jars are accessed in between `addUrl` calls?
Would it be technically feasible to first determine all the required jars before creating the first user CL?
Are there no plans to isolate the usage of a jar to a specific operator?
> Besides, there are no differences in this case when creating a new classloader to wrap the existing classloader.
It is indeed technically similar; but I already listed the benefits compared to the mutable CL in my previous comment.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1166415168
LGTM. Would be better @zhuzhurk or @zentol could have a double-check.
Btw, the E2E is still failed.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zentol commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1162750463
I'm pretty sure you are correct.
We never actually had the case where we had a tree of classloaders with different order settings between nodes.
It's always been either all child-first or all parent-first.
We'd have to change directions several times (from a call to the child CL, go up to default jars and try loading the class, _then_ drill down to other user jars and try loading the class, then jump up to the Flink CL and try loading the class).
That's just not supported. (And really we shouldn't because good look wrapping your head around that).
So let's discard _that_ idea.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zentol commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1164082044
+1
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zentol commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1158895050
You can just take whatever classloader you have (e.g., the Flink CL), and create a new FlinkUserCodeClassLoader with that CL as a parent and specify the jar via the urls parameter.
Conceptually, if you have a given CL, then adding a jar to it, or creating a new child CL with the jar should be functionally the same.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] MartijnVisser commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
MartijnVisser commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1170382823
If you now rebase, the CI should pass
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1158838709
> What prevents you from creating such a classloader as a child classloader?
The common parent class of `SafetyNetWrapperClassLoader ` and `FlinkUserCodeClassLoader ` is `URLClassLoader`, but `addURL` method of `URLClassLoader` is protected. If I implement a `MutableURLClassLoader` has the ability of `SafetyNetWrapperClassLoader` and `FlinkUserCodeClassLoader` simultaneously, which classLoader should `MutableURLClassLoader` extands? `SafetyNetWrapperClassLoader` or `FlinkUserCodeClassLoader` or `URLClassLoader`? Can you give me some idea? Or perhaps `MutableURLClassLoader` refer to `SafetyNetWrapperClassLoader` and `FlinkUserCodeClassLoader` as it member variable? I don't know how to implement this child classloader.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zentol commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1160143307
To further drive the point home, we don't need `addURL` or the `MutableURLClassLoader` at all,.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhuzhurk commented on a diff in pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zhuzhurk commented on code in PR #20003:
URL: https://github.com/apache/flink/pull/20003#discussion_r907262950
##########
flink-core/src/main/java/org/apache/flink/util/FlinkUserCodeClassLoaders.java:
##########
@@ -131,8 +130,8 @@ public static class ParentFirstClassLoader extends FlinkUserCodeClassLoader {
* delegate is nulled and can be garbage collected. Additional class resolution will be resolved
* solely through the bootstrap classloader and most likely result in ClassNotFound exceptions.
*/
- @Internal
- public static class SafetyNetWrapperClassLoader extends URLClassLoader implements Closeable {
+ private static class SafetyNetWrapperClassLoader extends MutableURLClassLoader
Review Comment:
No need to `implements Closeable` because `URLClassLoader` already `implements Closeable`.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1168679436
@zhuzhurk Thanks for your review, I've updated it.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1158696541
> Why do you need to modify the class loader instead of adding another child classloader?
Because we want the classlaoder has the ability of `SafetyNetWrapperClassLoader` and `FlinkUserCodeClassLoader ` simultaneously, user can decide wether checkClassLoaderLeak and child-first or parent first.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] MartijnVisser merged pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
MartijnVisser merged PR #20003:
URL: https://github.com/apache/flink/pull/20003
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1169449211
@flinkbot run azure
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zentol commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1160403004
> The required classes are dynamically changing when users interactively ADD JAR, so we have to replace the classloader or update classloader in place.
AKA the classloader provided earlier does not contain everything it needs _now_. If the requirements can change then the API of these components should be adjusted to support that.
> the classloader is clearly managed by the components.
That's quite a statement given that something external can just mutate the classloader under the hood without any these components being aware of it.
> Spark SQL also introduced MutableURLClassLoader to support ADD JAR
Since when is Spark SQL our measure for whether something is a good idea?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1162757295
Thank you @zentol .
So are you fine with the `MutableURLClassLoader` approach which is straightforward and (looks like so far) has no side effects?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zentol commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1162865164
> But we don't and won't support removing URLs/JARs.
Do you think this will likely stay the case?
Once a jar is added it is always distributed for every subsequent query independent of whether it actually requires it (as I understood it).
Are these jars small enough that users won't worry about that?
> Because this is an interactive process, the jars are added dynamically, and we don't know what jars will be added at what time point.
I assume the components that make use of the user CL all created when the session is started?
Is there any state kept across these statement that has a hard requirement for having access to the user CL?
For example, when I submit `CREATE TEMPORARY FUNCTION ...`, is the function eagerly loaded and put into some data-structure (a catalog I guess?)? Or do we store just some description and load it later when required?
> are you fine with the MutableURLClassLoader approach
yes-ish.
I think it would be still be nice if we would still create the user CL ahead of time before execution of a query (or more generally before creating any component that requires the user-jar). Conceptually that's certainly possible; as to whether that's possible (or reasonable) to implement right now w.r.t. the current architecture depends a lot of table api internals, how the parsing works and how the components are structured. I would defer that decision to you.
For a _very oversimplified_ view, let's consider these statements:
```
1) Flink SQL> ADD JAR 's3:///path/to/aaa.jar';
2) Flink SQL> CREATE TEMPORARY FUNCTION lower AS 'org.apache.flink.udf.Lower';
3) Flink SQL> SELECT id, lower(name) FROM T;
```
I assume that it is possible to _parse_ these statements without loading any user-code.
1) tells us we need a jar, so we add that to some list for later.
2) is just some function definition that we need later, so we store the parsed result _somewhere_
3) is a query (without side-effects) that requires execution -> build a new CL with 1), construct all the components required for execution (planner or something), pass statements 2+3 to them.
In a way this is a sort-of pre-processing step that filters for certain statement (parts) and maintains some state outside of components required for the execution.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1161906232
> That depends on how the class-loading order is set up and how you actually use it.
If you load everything parent-first within the added sub-tree this problem will not occur.
If we force to use parent-first mode, then the classloader behavior is inconsistent between local job compiling and distributed. Say the user wants to use `child-first` for distributed execution to resolve class conflict between user jar and flink core jar. However, the job can't be compiled because the client forces to use parent-first and ignores users' `classloader.resolve-order` configuration and causes NoSuchMethod exceptions.
> If we start removing URLs however this very much changes.
Yes. But we don't and won't support removing URLs/JARs.
> Can you clarify on whether the jars are accessed in between addUrl calls?
Would it be technically feasible to first determine all the required jars before creating the first user CL?
Ah, let me clarify the background of this pull request. The motivation is we would like to support `ADD JAR` and `CREATE FUNCTION ... USING JAR` ([FLIP-214](https://cwiki.apache.org/confluence/display/FLINK/FLIP-214+Support+Advanced+Function+DDL)) statements in the table ecosystem, especially in SQL CLI and SQL Gateway. I will explain a use case of SQL Gateway + ADD JAR. A SQL Gateway (FLIP-91) is a long-running service that many users can connect to it via REST/JDBC/Beeline... Each user has a separate environment (e.g. classloader) and can submit SQL statements interactively. For example:
```sql
-- start a SQL CLI and connects to the SQL Gateway which is serving at 10.10.11.2:8083
bin/sql-client.sh --endpoint 10.10.11.2:8083
-- A new session is opened for the current user, and a clean classloader is prepared for the user
-- query is executed without additional user jars
Flink SQL> SELECT * FROM T;
-- the user jar is added to the user classloader of the current session
Flink SQL> ADD JAR '/path/to/aaa.jar';
-- register a user-defined function (UDF) that is loaded from the previously added jaar
Flink SQL> CREATE TEMPORARY FUNCTION lower AS 'org.apache.flink.udf.Lower';
-- query is executed with the added jar and the UDF in the jar.
Flink SQL> SELECT id, lower(name) FROM T;
-- SQL Gateway downloads the jar from HDFS to local disk, and add the jar to the user classloader of the current session.
-- So the user classloader should contain both aaa.jar and bbb.jar
-- And register the UDF that is loaded from bbb.jar
Flink SQL> CREATE TEMPORARY FUNCTION upper AS 'me.wuchong.Upper' USING JAR 'hdfs:///path/to/bbb.jar';
-- query is executed with the added jars and the UDFs in the jar.
Flink SQL> SELECT id, lower(name), upper(name) FROM T;
-- session is closed, and the user classloader for this session is released in SQL Gateway
Flink SQL>exist;
```
So, yes, the jars are accessed in between addUrl calls and we can't determine all the required jars before creating the first user CL. Because this is an interactive process, the jars are added dynamically, and we don't know what jars will be added at what time point.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] MartijnVisser commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
MartijnVisser commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1169800206
> Do we can merge it before the test success?
We could. If we merge it and conclude after FLINK-28269 is fixed that this is a problem, it will have to be fixed at that moment (or this PR needs to be reverted). But I doubt that this PR would break the Kubernetes tests.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1164113298
> +1
Thanks for @zentol , can you help review this PR? https://github.com/apache/flink/pull/20001 is blocked by this.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zhuzhurk commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zhuzhurk commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1158708046
Is it possible to add the `FlinkUserCodeClassLoader` to the table module, and use it to wrap the existing classloader in the case that a classloader needs to be mutated, i.e. in CTAS code paths?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] MartijnVisser commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
MartijnVisser commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1169708503
The CI will continue to fail due to https://issues.apache.org/jira/browse/FLINK-28269
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zentol commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1162721597
> If we force to use parent-first mode, then the classloader behavior is inconsistent between local job compiling and distributed. Say the user wants to use child-first for distributed execution to resolve class conflict between user jar and flink core jar. However, the job can't be compiled because the client forces to use parent-first and ignores users' classloader.resolve-order configuration and causes NoSuchMethod exceptions.
Sorry if I had not made it clear, when I said child-first I only referred to the new child CLs.
Flink CL <order_set_by_user> User CL <parent-first> child user CL 1 <parent-first> child user CL 2
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1161703624
In product, one job may use multiple different udf jar, so in your way, here will be multiple CL existing at the same time. In [FLIP-91](https://cwiki.apache.org/confluence/display/FLINK/FLIP-91%3A+Support+SQL+Gateway) & [FLIP-223](https://cwiki.apache.org/confluence/display/FLINK/FLIP-223%3A+Support+HiveServer2+Endpoint), community will introduce gateway component which is a long running service, and responsible for compile and submit sql, the gateway also need to use this MutableURLClassLoader to add jar dynamically. If we create a new CL when add a jar every time, as the time go, there likely have a CL leak due to classloader is not guaranteed to be closed.
Spark & Hive both also `MutableURLClassLoader` which expose the `addURL` method to support Add Jar for hive server. In addition, Spark also use the `MutableURLClassLoader` in `Executor` to load dependent jar of the job during distribution execution phase.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zentol commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1160133431
> create a new CL in MutableURLClassLoader with the existing URL
No. There is no need to copy the URLs of the parent CL.
By adding the new CL as a child you will still be able to load classes located in the parent, because that's just how classloading works.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1162734430
This still seems inconsistent classloader behavior compared to the distributed execution. Please correct me if I'm wrong.
When resolve-order is child-first, the classloader order in client is:
`User CL` > `Flink CL` > `child user CL 1` > `child user CL 2`
which means the jar find path is:
default user jars > flink jars > user jar 1 > user jar 2
However, the classloader order (jar find path) in runtime is:
default user jars > user jar 1 > user jar 2 > flink jars
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1169634839
@flinkbot run azure
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] MartijnVisser commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
MartijnVisser commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1170827097
@flinkbot run azure
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zentol commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1158699288
What prevents you from creating such a classloader as a child classloader?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1158682088
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "738c2db31de828155396b1f89076459f6957607c",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "738c2db31de828155396b1f89076459f6957607c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 738c2db31de828155396b1f89076459f6957607c UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zentol commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1158691051
Why do you need to modify the class loader instead of adding another child classloader?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1160374181
> A classloader should not be passed to a component if at that time it does not have access to all required classes.
The classloader DOES have access to all required classes at the time when it is passed to a component. The required classes are dynamically changing when users interactively ADD JAR, so we have to replace the classloader or update classloader in place.
> If you're lacking clear ownership of the classloader then mutating said classloader is a dangerous operation anyway.
Sorry, I don't understand what's the "dangerous operation" you mean. There is no classloader leaking and the classloader is clearly managed by the components.
Btw, Spark SQL also introduced [`MutableURLClassLoader`](https://github.com/apache/spark/blob/master/core/src/main/java/org/apache/spark/util/MutableURLClassLoader.java) to support `ADD JAR`. I think this is a reasonable way to support this feature, not a hacking way.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1159453217
> You can just take whatever classloader you have (e.g., the Flink CL), and create a new FlinkUserCodeClassLoader with that CL as a parent and specify the jar via the urls parameter.
>
> Conceptually, if you have a given CL, then adding a jar to it, or creating a new child CL with the jar should be functionally the same.
I guess you mean that `MutableURLClassLoader ` refer to a given CL, when adding a jar, we create a new CL in `MutableURLClassLoader `with the existing URL and the new jar URL?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
zentol commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1160232881
Then we're just hacking around technical debt.
A classloader should not be passed to a component if at that time it does not have access to all required classes.
You should rework your code to either not eagerly supply all components with the classloader but pass it in at the time it is needed, or setup some supplier it can query later on.
If you're lacking clear ownership of the classloader then mutating said classloader is a dangerous operation anyway.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1163056508
> Do you think this will likely stay the case?
Are these jars small enough that users won't worry about that?
Yes. Spark/Databricks also doesn't support remove jar[1]. If a user wants to drop a jar, he can simply restart a session. Usually, the jars are small because only contain UDFs and limited dependencies.
> I assume the components that make use of the user CL all created when the session is started?
Yes. They are created when the session is started and released when the session is closed.
> Is there any state kept across these statement that has a hard requirement for having access to the user CL?
For example, when I submit CREATE TEMPORARY FUNCTION ..., is the function eagerly loaded and put into some data-structure (a catalog I guess?)? Or do we store just some description and load it later when required?
Yes. Currentlly, for example, `CREATE CATALOG` and `LOAD MODULE` will eagerly load the `Catalog` and `Module` instances and bookkeep them in `CatalogManager` and `ModuleManager`.
I think you raised a good idea that lazily creating the classloader when needed (e.g. validation, execution). Technical-wise, this is possible by holding everything as descriptions. However, this may slow down the SQL compiling speed because the classloaders need to be reloaded every time, and catalogs/modules/... need to be re-discovered by SPI every time.
Considering this needs a big refactor on the current architecture, the parsing mechanism, and there maybe some corner problems I don't notice so far. Therefore, I would prefer to go with `MutableURLClassloader` (mark as `@Internal`) approach first. We can revisit it sometime later whether it has any problems or can support all use cases, to see whether we need to refactor the architecture.
What do you think about this?
[1]: https://docs.gcp.databricks.com/spark/latest/spark-sql/language-manual/sql-ref-syntax-aux-resource-mgmt-add-jar.html#related-statements
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1169699353
@flinkbot run azure
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] lsyldliu commented on pull request #20003: [FLINK-28080][runtime] Introduce MutableURLClassLoader as parent class of FlinkUserClassLoader and SafetyNetWrapperClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #20003:
URL: https://github.com/apache/flink/pull/20003#issuecomment-1169784907
>
Do we can merge it before the test success?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org