[GitHub] flink pull request #3933: [FLINK-6606] Create checkpoint hook with user clas...
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3933 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request #3933: [FLINK-6606] Create checkpoint hook with user clas...
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/3933#discussion_r117427223
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/hooks/MasterHooks.java
---
@@ -267,6 +269,112 @@ else if (!allowUnmatchedState) {
}
//
+ // hook management
+ //
+
+ /**
+* Wraps a hook such that the user-code classloader is applied when the
hook is invoked.
+* @param hook the hook to wrap
+* @param userClassLoader the classloader to use
+*/
+ public static MasterTriggerRestoreHook
wrapHook(MasterTriggerRestoreHook hook, ClassLoader userClassLoader) {
+ return new WrappedMasterHook(hook, userClassLoader);
+ }
+
+ @VisibleForTesting
+ static class WrappedMasterHook implements
MasterTriggerRestoreHook {
+
+ private final MasterTriggerRestoreHook hook;
+ private final ClassLoader userClassLoader;
+
+ WrappedMasterHook(MasterTriggerRestoreHook hook, ClassLoader
userClassLoader) {
+ this.hook = hook;
+ this.userClassLoader = userClassLoader;
+ }
+
+ @Override
+ public String getIdentifier() {
+ Thread thread = Thread.currentThread();
+ ClassLoader originalClassLoader =
thread.getContextClassLoader();
+ thread.setContextClassLoader(userClassLoader);
--- End diff --
True.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3933: [FLINK-6606] Create checkpoint hook with user clas...
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/3933#discussion_r117421730
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/hooks/MasterHooks.java
---
@@ -267,6 +269,112 @@ else if (!allowUnmatchedState) {
}
//
+ // hook management
+ //
+
+ /**
+* Wraps a hook such that the user-code classloader is applied when the
hook is invoked.
+* @param hook the hook to wrap
+* @param userClassLoader the classloader to use
+*/
+ public static MasterTriggerRestoreHook
wrapHook(MasterTriggerRestoreHook hook, ClassLoader userClassLoader) {
+ return new WrappedMasterHook(hook, userClassLoader);
+ }
+
+ @VisibleForTesting
+ static class WrappedMasterHook implements
MasterTriggerRestoreHook {
+
+ private final MasterTriggerRestoreHook hook;
+ private final ClassLoader userClassLoader;
+
+ WrappedMasterHook(MasterTriggerRestoreHook hook, ClassLoader
userClassLoader) {
+ this.hook = hook;
+ this.userClassLoader = userClassLoader;
+ }
+
+ @Override
+ public String getIdentifier() {
+ Thread thread = Thread.currentThread();
+ ClassLoader originalClassLoader =
thread.getContextClassLoader();
+ thread.setContextClassLoader(userClassLoader);
--- End diff --
True, but I guess if `close` is idempotent, then it doesn't hurt.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3933: [FLINK-6606] Create checkpoint hook with user clas...
Github user EronWright commented on a diff in the pull request:
https://github.com/apache/flink/pull/3933#discussion_r117370390
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/hooks/MasterHooks.java
---
@@ -267,6 +269,112 @@ else if (!allowUnmatchedState) {
}
//
+ // hook management
+ //
+
+ /**
+* Wraps a hook such that the user-code classloader is applied when the
hook is invoked.
+* @param hook the hook to wrap
+* @param userClassLoader the classloader to use
+*/
+ public static MasterTriggerRestoreHook
wrapHook(MasterTriggerRestoreHook hook, ClassLoader userClassLoader) {
+ return new WrappedMasterHook(hook, userClassLoader);
+ }
+
+ @VisibleForTesting
+ static class WrappedMasterHook implements
MasterTriggerRestoreHook {
+
+ private final MasterTriggerRestoreHook hook;
+ private final ClassLoader userClassLoader;
+
+ WrappedMasterHook(MasterTriggerRestoreHook hook, ClassLoader
userClassLoader) {
+ this.hook = hook;
+ this.userClassLoader = userClassLoader;
+ }
+
+ @Override
+ public String getIdentifier() {
+ Thread thread = Thread.currentThread();
+ ClassLoader originalClassLoader =
thread.getContextClassLoader();
+ thread.setContextClassLoader(userClassLoader);
--- End diff --
I disagee with that approach due to lack of symmetry.The try..finally
idiom is typically written as:
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3933: [FLINK-6606] Create checkpoint hook with user clas...
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/3933#discussion_r117370134
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/hooks/MasterHooks.java
---
@@ -267,6 +269,112 @@ else if (!allowUnmatchedState) {
}
//
+ // hook management
+ //
+
+ /**
+* Wraps a hook such that the user-code classloader is applied when the
hook is invoked.
+* @param hook the hook to wrap
+* @param userClassLoader the classloader to use
+*/
+ public static MasterTriggerRestoreHook
wrapHook(MasterTriggerRestoreHook hook, ClassLoader userClassLoader) {
+ return new WrappedMasterHook(hook, userClassLoader);
+ }
+
+ @VisibleForTesting
+ static class WrappedMasterHook implements
MasterTriggerRestoreHook {
+
+ private final MasterTriggerRestoreHook hook;
+ private final ClassLoader userClassLoader;
+
+ WrappedMasterHook(MasterTriggerRestoreHook hook, ClassLoader
userClassLoader) {
+ this.hook = hook;
+ this.userClassLoader = userClassLoader;
+ }
+
+ @Override
+ public String getIdentifier() {
+ Thread thread = Thread.currentThread();
+ ClassLoader originalClassLoader =
thread.getContextClassLoader();
+ thread.setContextClassLoader(userClassLoader);
--- End diff --
Maybe we can move the `setContextClassLoader` into the `try` body. Just to
be on the safe side that whenever something happens we will reset the original
class loader.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3933: [FLINK-6606] Create checkpoint hook with user clas...
GitHub user EronWright opened a pull request: https://github.com/apache/flink/pull/3933 [FLINK-6606] Create checkpoint hook with user classloader - wrap calls to MasterTriggerRestoreHook (and its factory) such that the user classloader is applied You can merge this pull request into a Git repository by running: $ git pull https://github.com/EronWright/flink FLINK-6606 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3933.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 #3933 commit fbf904a60a1e252944e1cbc7ad60c5d95ae28ec2 Author: Wright, Eron Date: 2017-05-17T16:46:13Z FLINK-6606 - wrap calls to MasterTriggerRestoreHook (and its factory) such that the user classloader is applied --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
