Skip to content
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-31398][sql-gateway] OperationExecutor is no longer set context classloader when executing statement. #22294

Merged
merged 1 commit into from Apr 19, 2023

Conversation

reswqa
Copy link
Member

@reswqa reswqa commented Mar 29, 2023

What is the purpose of the change

Currently, method OperationExecutor#executeStatement in sql client will wrap currently with sessionContext.getSessionState().resourceManager.getUserClassLoader(). Actually, it's not necessary.

It'll will cause the exception Trying to access closed classloader. Please check if you store xxx after quiting sql client.
The reason is in ShutdownHookManager, it will register a hook after jvm shutdown. In ShutdownHookManager, it will
create Configuration. It will then access Thread.currentThread().getContextClassLoader() which is FlinkUserClassLoader, the FlinkUserClassLoader has been closed before. So, it'll then cause Trying to access closed classloader exception.

Brief change log

  • OperationExecutor is no longer set context classloader when executing statement.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

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

@flinkbot
Copy link
Collaborator

flinkbot commented Mar 29, 2023

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

Copy link
Contributor

@luoyuxia luoyuxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reswqa Thanks for contribution. LGTM. Could you please rebase master to see if the ci still pass?

@@ -182,23 +181,17 @@ public ResultFetcher configureSession(OperationHandle handle, String statement)

public ResultFetcher executeStatement(OperationHandle handle, String statement) {
// Instantiate the TableEnvironment lazily
// TODO: remove the usage of the context classloader until {@link
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Althogh the comment said we need make HiveParserUtils#getFunctionInfo use ResourceManager explicitly.
But after do some investigation, I found it's not necessary.
Currently, there's two callers to call method HiveParserUtils#getFunctionInfo
1:

FunctionInfo fi = HiveParserUtils.getFunctionInfo(funcText);
 if (fi == null) {
 desc = convertSqlOperator(funcText, children, ctx);
}
....

The fi will be null after we remove the classloader wrapper as it can't load the function using Thread.currentThread().getContextClassLoader(). But it's fine since it will then try to find the function from the function catalog. Such case have been convered in flink-sql-client by set.q.

2: getFuncExprNodeDescWithUdfData
There're two places call method getFuncExprNodeDescWithUdfData.
a: convertGenericFunc(ExprNodeGenericFuncDesc func), but it will only be called when it's hive build-in function, so it can be found in Thread.currentThread().getContextClassLoader() as use must put flink-connector-hive jar which include hive built-in functions to FLINK_HOME/lib to use Hive Dialect.
b:

// type conversion for LHS
            if (TypeInfoUtils.isConversionRequiredForComparison(lhsType, commonType)) {
                lhsDesc =
                        HiveASTParseUtils.createConversionCast(
                                lhsDesc, (PrimitiveTypeInfo) commonType);
            }

Also, HiveParserUtils.getFunctionInfo(funcText) will only be called to when it's build-in function which can then be loaded.

AFAIC, it's not easy to make HiveParserUtils#getFunctionInfo use ResourceManager explicitly which will involves much modification. I think it's fine to remove the usage of the context classloader in here.
cc @fsk119

@reswqa reswqa force-pushed the FLINK-31398-fix-classloader branch from 171b25b to 01c9b98 Compare April 18, 2023 09:38
@reswqa
Copy link
Member Author

reswqa commented Apr 18, 2023

Thanks @luoyuxia for the review, I have rebased this pr.

@luoyuxia luoyuxia merged commit 921267b into apache:master Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants