-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HADOOP-19305: Fix ProcessEnvironment ClassCastException in Shell.java #7106
base: trunk
Are you sure you want to change the base?
Conversation
@zhangbutao I will review this PR later. I hope that after resolving this issue, we can update Hadoop 3.4.0 in Hive. |
Thanks @slfan1989 . Please check my comment. Change Shell.java can fix the issue, and imrpove hive's UTs code can also resolve this issue. Changing Shell.java is more simple but i am not sure when hadoop release the new version. Imrpoving hive's UTs may need some time to explore&test the env related codes. |
@zhangbutao Thank you for your contribution! I will respond later today. |
while this change seems valid, why aren't you just using |
Also, can you try doing this PR against branch-3.4.1, as this is the one we want to build an RC off this week. get in in there and the next hadoop release will work |
💔 -1 overall
This message was automatically generated. |
@steveloughran Thanks your suggestion. Not sure if I understand what you mean correctly. BTW, the root cause is not from |
@steveloughran #7107 Fix in branch-3.4.1 |
@zhangbutao afraid RC3 is already out, and as this is a test only failure I don't think it is a blocker. Why doesn't the setEnvironment method work for your tests? |
@steveloughran No worry, i can also fix this at Hive side. See commit apache/hive@fa7797b in apache/hive#5500 . In fact, This related code snippet is useless & incorrect, but it triggered this issue. I just remove the code snippet. I think i won't take much time to fix the useless test code.
Sorry i didn't get you about the setEnvironment. Do you mean the change like this:
I tested this |
Thank you for your continued attention to this issue! The change seems feasible, but we may need to find a workaround.
|
I have found the solution in Hive side. So free feel to merge it in Hadoop 3.4.2. BTW, if Hadoop 3.4.1 is released out, i think we can update hadoop version to 3.4.1 in Hive. :) |
hadoop-3.4.1 may take some time to be released; I personally feel it could be at least 1-2 weeks, or even longer. Would it be possible for us to upgrade the supported Hadoop version in Hive to 3.4.0 first? This way, when Hadoop 3.4.1 is released, we can submit a PR for the upgrade version. |
@zhangbutao I can approve this PR, but we still need Steve's consent, as he is very knowledgeable and insightful in this area. |
@@ -977,7 +977,9 @@ private void runCommand() throws IOException { | |||
builder.environment().clear(); | |||
} | |||
|
|||
builder.environment().putAll(this.environment); | |||
if (!environment.isEmpty()) { |
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.
@zhangbutao Personally, I think adding a check is reasonable, but can we modify it further based on Steve's suggestions?
cc: @steveloughran
We may also need Apache Tez to upgrade Hadoop version to 3.4.0 at the same time. So i am not sure if we can quickly upgrade Hadoop version in Hive. |
@zhangbutao Hi, what is different to #7107 , if they are the same one, please choose one to close, thanks. |
@Hexiaoqiao it is a backport to branch3.4.1. If 3.4.1 is released out, please feel free to close it. thanks. |
Description of PR
See HADOOP-19305 and HIVE-28191
How was this patch tested?
Tested with the Apache Hive Tests.
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?