You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by aljoscha <gi...@git.apache.org> on 2017/08/18 13:17:13 UTC
[GitHub] flink pull request #4564: [FLINK-7442] Add option for using child-first clas...
GitHub user aljoscha opened a pull request:
https://github.com/apache/flink/pull/4564
[FLINK-7442] Add option for using child-first classloader for loading user code
This is an alternative to #4554 that does not make the client class loader configurable.
## What is the purpose of the change
This PR introduces a new core option (`classloader.resolve-order: child-first`) that allows using a child-first class loader for user code. The default is still to use a parent-first class loader.
This also does a minor refactoring in the way the blob manager retrieves the cleanup interval. It's now also read from the `Configuration`, since we already have the `Configuration` for the class loader settings.
## Brief change log
- Introduce new option
- Pass `Configuration` thought to all places where we previously created a user class loader
- Instantiate correct class loader based on config
## Verifying this change
This PR introduces new end-to-end tests that verify the new feature in a complete Flink workflow, including starting the program using `bin/flink run`.
## Does this pull request potentially affect one of the following parts:
This affects class loader, which is quite important to get right.
## Documentation
- the new flag is documented in the config documentation
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aljoscha/flink jira-7441-child-first-classloader-alternative
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4564.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4564
----
commit 33bd4cf5160ec64cbaace876b305922b804cc3a1
Author: Aljoscha Krettek <al...@gmail.com>
Date: 2017-08-14T12:53:14Z
[FLINK-7442] Add option for using a child-first classloader for loading user code
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4564: [FLINK-7442] Add option for using child-first classloader...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4564
Thanks, this looks better to me!
I would suggest to not pass the configuration into the library cache manager.
I think passing configuration objects into specialized / dedicated components is an anti-pattern. It makes testing complicated, signatures inexplicit, etc.
For the second task of the classloader, loading resources: Do we have a test that validates the resource resolution? I think this passes the tests of the previous pull request because it behaves for resources like a *parent-first classloader*, which seems inconsistent. Admittedly the existing implementation of the child-first classloader was for resources a child-only classloader, which was also not correct.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4564: [FLINK-7442] Add option for using child-first classloader...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/4564
@StephanEwen I finally ironed out the last kinks. I think this is now ready for a final review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4564: [FLINK-7442] Add option for using child-first clas...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha closed the pull request at:
https://github.com/apache/flink/pull/4564
---
[GitHub] flink issue #4564: [FLINK-7442] Add option for using child-first classloader...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/4564
@StephanEwen I updated the PR. Turns out you also have to override the resource-related methods to change the resolution order. I added checks for that to the end-to-end test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4564: [FLINK-7442] Add option for using child-first classloader...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4564
Did a side-by-side review with @aljoscha and directly fixed issues.
This is in very good shape from my side now.
+1 to merge this
---