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-33358][sql] Fix Flink SQL Client fail to start in Flink on YARN #23629
Conversation
@flinkbot run azure |
@Samrat002 @1996fanrui Could you review this patch when you have time? Thanks. |
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.
Thank you Prabhu for the patch.
Changes looks good to me 🚀
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.
In my humble opinion, this change is fine. But I don't know much about the details of flink sql, and I'm not sure whether this change introduces additional risks. It would be better if there are flink sql experts to help and confirm.
Hi @lsyldliu , would you mind helping take a look this PR in your free time? thanks~
cc @fsk119 |
@fsk119 Please review this patch when you have time. Thanks. |
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.
I think it makes sense for the SQL client JAR to take precedence over the Hadoop CP, so in general any deps that is shaded into the SQL client JAR will be picked up instead whatever is on the Hadoop CP.
Although these kind of errors are likely to depend on the environment, how the Hadoop CP is setup.
@1996fanrui Could you help in committing this change. Thanks. |
I am hitting the same problem, with Flink 1.18.1. Setup
Check env
Run SQL Client
|
I can confirm that using the fix in this PR resolves the issue for me. |
Thanks @rmoff for the verification. @dannycranmer Could you please assist with reviewing this patch? Thank you. |
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.
LGTM
Thanks @fsk119 |
Sure enough it can be used |
What is the purpose of the change
Moves the flink sql client jar precedence before the Hadoop classpath to avoid conflicts with the older jline version from the Hadoop classpath.
Brief change log
Verifying this change
Launched the Flink SQL Session and verified with Flink sql examples.
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation