-
Notifications
You must be signed in to change notification settings - Fork 13.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-28897] [TABLE-SQL] Fail to use udf in added jar when enabling checkpoint #25656
base: release-1.20
Are you sure you want to change the base?
Conversation
@@ -1029,6 +1029,8 @@ private TableResultInternal executeInternal( | |||
defaultJobName, | |||
jobStatusHookList); | |||
try { | |||
ClassLoader userClassLoader = Thread.currentThread().getContextClassLoader(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please could you change the variable name to be something like originalContextClassLoader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the variable name to contextClassLoader
.
@@ -1029,6 +1029,8 @@ private TableResultInternal executeInternal( | |||
defaultJobName, | |||
jobStatusHookList); | |||
try { | |||
ClassLoader userClassLoader = Thread.currentThread().getContextClassLoader(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add coments and refer to the v2 implementation in the comments and that the v2 refactor is not going to be backported to 1.20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comments on the need for this change. The latest FLIP implementation to introduce a stream graph based job submission moved the StreamGraph module to flink runtime and changed the job submission logic by directly submitting a StreamGraph to the job manager.
Ref FLIP: https://cwiki.apache.org/confluence/display/FLINK/FLIP-468%3A+Introducing+StreamGraph-Based+Job+Submission
Related JIRA: https://issues.apache.org/jira/browse/FLINK-36065
As a result of these changes this issue seems to be fixed for flink v2 as I was not able to reproduce it with the latest main. Hence limiting this particular change to 1.20 versions.
@@ -1069,8 +1072,11 @@ private TableResultInternal executeQueryOperation( | |||
|
|||
Pipeline pipeline = generatePipelineFromQueryOperation(operation, transformations); | |||
try { | |||
ClassLoader userClassLoader = Thread.currentThread().getContextClassLoader(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please change the variable name , add a comment and add unit tests (or provide a reason why unit tests cannot be added) .
Reviewed by Chi on 21/11/24. Asked submitter questions. |
What is the purpose of the change
This pull request fixes the class loading issues when using udf in add jar and enabling checkpointing.
Brief change log
Verifying this change
Please make sure both new and modified tests in this PR follow the conventions for tests defined in our code quality guide.
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: ( no)Documentation