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/04/08 15:25:15 UTC
[GitHub] [flink] slinkydeveloper opened a new pull request, #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
slinkydeveloper opened a new pull request, #19410:
URL: https://github.com/apache/flink/pull/19410
This PR solves the issue of allowing the user to provide its own classloader for UDFs and at other places where it's more appropriate than the current Thread context classloader.
The PR also contains a couple of hotfixes (mergeable separately):
* Deprecate `CatalogFunction#isGeneric`
* Remove `CodeGeneratorContext#apply` which makes the code unnecessarily more complicated
* Fix singleton config in `EnvironmentSettings`, which among various issues, it's also causing in this case to select the wrong user class loader
--
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] matriv commented on pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
matriv commented on PR #19410:
URL: https://github.com/apache/flink/pull/19410#issuecomment-1094694109
Thx!
--
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] matriv commented on pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
matriv commented on PR #19410:
URL: https://github.com/apache/flink/pull/19410#issuecomment-1123523164
I'm working on it: https://github.com/matriv/flink/tree/FLINK-15635-new already.
--
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 #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #19410:
URL: https://github.com/apache/flink/pull/19410#issuecomment-1120109452
ping @slinkydeveloper
--
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 #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #19410:
URL: https://github.com/apache/flink/pull/19410#issuecomment-1110755802
@slinkydeveloper Thanks for your contribution, can you help to rebase the master and push this pr merge to master, here some other work rely on you.
--
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] matriv commented on a diff in pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
matriv commented on code in PR #19410:
URL: https://github.com/apache/flink/pull/19410#discussion_r847037842
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/EncodingUtils.java:
##########
@@ -139,7 +139,11 @@ public static String encodeObjectToString(Serializable obj) {
}
}
- /** @see #loadClass(String, ClassLoader) */
+ /**
+ * @deprecated Use {@link #loadClass(String, ClassLoader)} instead, in order to explicitly
+ * provide the correct classloader.
+ * @see #loadClass(String, ClassLoader)
Review Comment:
maybe, remove now this line?
--
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] slinkydeveloper commented on pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
slinkydeveloper commented on PR #19410:
URL: https://github.com/apache/flink/pull/19410#issuecomment-1123325484
I don't think I'll be able to finish this PR, can someone take it over?
--
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] matriv commented on a diff in pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
matriv commented on code in PR #19410:
URL: https://github.com/apache/flink/pull/19410#discussion_r846285936
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/FunctionCatalog.java:
##########
@@ -621,7 +626,7 @@ public void registerTemporarySystemFunction(
}
@SuppressWarnings("unchecked")
- private void validateAndPrepareFunction(CatalogFunction function)
+ private CatalogFunction validateAndPrepareFunction(CatalogFunction function)
Review Comment:
Just a double check, have you checked all usages to make sure we use the returned value?
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/utils/PlannerMocks.java:
##########
@@ -96,6 +102,7 @@ private PlannerMocks(
true,
ExpressionResolver.resolverFor(
tableConfig,
+ Thread.currentThread().getContextClassLoader(),
Review Comment:
Why not use `PlannerMocks.class.getClassLoader()` here?
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/EncodingUtils.java:
##########
@@ -139,6 +139,8 @@ public static String encodeObjectToString(Serializable obj) {
}
}
+ /** @see #loadClass(String, ClassLoader) */
Review Comment:
please add `@deprecated`
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/CatalogFunction.java:
##########
@@ -59,6 +59,7 @@
*
* @return whether the function is a generic function
*/
+ @Deprecated
Review Comment:
Please add `@deprecated` section to the javadoc
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/rules/physical/stream/StreamPhysicalJoinRuleBase.scala:
##########
@@ -25,6 +25,11 @@ import org.apache.flink.table.planner.plan.nodes.{FlinkConventions, FlinkRelNode
import org.apache.flink.table.planner.plan.utils.IntervalJoinUtil
import org.apache.flink.table.planner.utils.ShortcutUtils.unwrapTableConfig
+import org.apache.flink.table.planner.plan.utils.{FlinkRelOptUtil, IntervalJoinUtil}
Review Comment:
leftover?
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/functions/UserDefinedFunctionHelper.java:
##########
@@ -244,17 +245,23 @@ public static UserDefinedFunction instantiateFunction(Class<?> functionClass) {
}
/** Prepares a {@link UserDefinedFunction} instance for usage in the API. */
- public static void prepareInstance(ReadableConfig config, UserDefinedFunction function) {
+ public static <T extends UserDefinedFunction> T prepareInstance(
Review Comment:
Maybe add to the javadoc, that the caller needs to use the returned value?
--
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] matriv commented on a diff in pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
matriv commented on code in PR #19410:
URL: https://github.com/apache/flink/pull/19410#discussion_r847035996
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/utils/PlannerMocks.java:
##########
@@ -96,6 +102,7 @@ private PlannerMocks(
true,
ExpressionResolver.resolverFor(
tableConfig,
+ Thread.currentThread().getContextClassLoader(),
Review Comment:
It's more for consistency, and avoid confusion, if someone checks the code and wonders why here is different.
--
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 closed pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
wuchong closed pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
URL: https://github.com/apache/flink/pull/19410
--
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 #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #19410:
URL: https://github.com/apache/flink/pull/19410#issuecomment-1136633593
> >
>
> ping @matriv Can you help to complete this work as soon as possible? [FLIP-214](https://cwiki.apache.org/confluence/display/FLINK/FLIP-214+Support+Advanced+Function+DDL) is blocked by this issue now, thanks.
Hi, @matriv , If you don't have time to complete this work, I will take over it, thanks.
--
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 #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #19410:
URL: https://github.com/apache/flink/pull/19410#issuecomment-1132420587
>
ping @matriv Can you help to complete this work as soon as possible? [FLIP-214](https://cwiki.apache.org/confluence/display/FLINK/FLIP-214+Support+Advanced+Function+DDL) is blocked by this issue now, thanks.
--
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] matriv commented on pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
matriv commented on PR #19410:
URL: https://github.com/apache/flink/pull/19410#issuecomment-1114831467
Working on rebasing and reshaping this PR
--
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 #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #19410:
URL: https://github.com/apache/flink/pull/19410#issuecomment-1142044729
Continue the review work in #19845
--
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 #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #19410:
URL: https://github.com/apache/flink/pull/19410#issuecomment-1140957508
Due to @matriv had not responsed for a long day, I will take over this ticket, thanks for all 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] slinkydeveloper commented on a diff in pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
slinkydeveloper commented on code in PR #19410:
URL: https://github.com/apache/flink/pull/19410#discussion_r847015525
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/utils/PlannerMocks.java:
##########
@@ -96,6 +102,7 @@ private PlannerMocks(
true,
ExpressionResolver.resolverFor(
tableConfig,
+ Thread.currentThread().getContextClassLoader(),
Review Comment:
In test classpath, this shouldn't make any difference. Anyway, I changed 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] matriv commented on pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
matriv commented on PR #19410:
URL: https://github.com/apache/flink/pull/19410#issuecomment-1094710854
If we don't choose to backport the functionality to 1.15, we need to backport the fix for `EnviromentSettings` with the static variables.
--
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] matriv commented on a diff in pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
matriv commented on code in PR #19410:
URL: https://github.com/apache/flink/pull/19410#discussion_r847037424
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/functions/UserDefinedFunctionHelper.java:
##########
@@ -244,7 +244,11 @@ public static UserDefinedFunction instantiateFunction(Class<?> functionClass) {
}
}
- /** Prepares a {@link UserDefinedFunction} instance for usage in the API. */
+ /**
+ * Prepares a {@link UserDefinedFunction} instance for usage in the API.
Review Comment:
Maybe also add that it's instantiated using the `userClassLoadrer`
--
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] slinkydeveloper commented on a diff in pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
slinkydeveloper commented on code in PR #19410:
URL: https://github.com/apache/flink/pull/19410#discussion_r847015104
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/FunctionCatalog.java:
##########
@@ -621,7 +626,7 @@ public void registerTemporarySystemFunction(
}
@SuppressWarnings("unchecked")
- private void validateAndPrepareFunction(CatalogFunction function)
+ private CatalogFunction validateAndPrepareFunction(CatalogFunction function)
Review Comment:
yep, all of them now use the returned value here
--
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 #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #19410:
URL: https://github.com/apache/flink/pull/19410#issuecomment-1093008517
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "5819f99906d857d550f5c2ce5ba6793076398fbe",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "5819f99906d857d550f5c2ce5ba6793076398fbe",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 5819f99906d857d550f5c2ce5ba6793076398fbe 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] lsyldliu commented on pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #19410:
URL: https://github.com/apache/flink/pull/19410#issuecomment-1127790501
>
@matriv Thanks for your contribution, can your complete this work as soon as possible?
--
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