-
Notifications
You must be signed in to change notification settings - Fork 28.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
[SPARK-3826][SQL]enable hive-thriftserver to support hive-0.13.1 #2685
Conversation
Can one of the admins verify this patch? |
QA tests have started for PR 2685 at commit
|
QA tests have finished for PR 2685 at commit
|
Can one of the admins verify this patch? |
@pwendell, i am resolving the conflicts, other TODO's here? |
6cfcec8
to
c6da3ce
Compare
<activeByDefault>false</activeByDefault> | ||
</activation> | ||
<modules> | ||
<module>sql/hive-thriftserver</module> |
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.
After hive-0.13.1 is committed, sql/hive-thirftserver can be put to top level instead of here.
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.
you mean move to <modules>
upper in this pom?
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.
Yes. either move to modules, or move to hive profile, since it is supported in both versions.
9e5e2fc
to
f48d3a5
Compare
Now that Hive 13 is merged in it would be great to get this in ASAP. I looked over this and it seems pretty good. My only high level comment is maybe we should keep all the Hive Shim code in a single project instead of having version specific code in both hive and hive-thrift server. That would simplify the build and consolidate the places where we have these hacks. It woudl also allow us to avoid duplicating things like @pwendell can you glance over the (limited) build changes. |
@@ -167,7 +167,7 @@ CURRENT_BLOCK=$BLOCK_SPARK_UNIT_TESTS | |||
# If the Spark SQL tests are enabled, run the tests with the Hive profiles enabled. | |||
# This must be a single argument, as it is. | |||
if [ -n "$_RUN_SQL_TESTS" ]; then | |||
SBT_MAVEN_PROFILES_ARGS="$SBT_MAVEN_PROFILES_ARGS -Phive" | |||
SBT_MAVEN_PROFILES_ARGS="$SBT_MAVEN_PROFILES_ARGS -Phive -Phive-0.12.0" |
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.
is this change ncessary? seems like it might be good to leave it how it is now
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.
Yeah agreed. If Hive13 is the default we should test Hive13.
Some minor comments on the build stuff. I think we're close, it's just small stuff. |
The thirift server is not available in the default (hive13) profile yet which is breaking all SQL only PRs. This turns off these test until #2685 is merged. Author: Michael Armbrust <[email protected]> Closes #2950 from marmbrus/fixTests and squashes the following commits: 1a6dfee [Michael Armbrust] [HOTFIX][SQL] Temporarily turn of hive-server tests.
@marmbrus, how about make a new sub project named hive-shim to keep all the Hive Shim code in it? |
Test PASSed. |
Yay! It passed! |
So will you re-publish the hive 0.13 jar @pwendell? or use 0.13.1a? |
it is not possible to mutate them after the fact. Let's stick with 0.13.1a - I will fully release it now. But don't remove the extra repository (we can remove it later) because it takes some time to propagate. |
echo -e "q\n" \ | ||
| sbt/sbt $BUILD_MVN_PROFILE_ARGS clean package assembly/assembly \ | ||
| sbt/sbt $BUILD_MVN_PROFILE_ARGS clean hive/compile hive-thriftserver/compile \ |
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 actually don't think this is compiling against Hive 0.12 right now... is it?
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's against 0.12 because BUILD_MVN_PROFILE_ARGS="$SBT_MAVEN_PROFILES_ARGS -Phive -Phive-0.12.0"
, right?
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.
It should be, BUILD_MVN_PROFILE_ARGS
is defined above with -Phive-0.12.0
:
BUILD_MVN_PROFILE_ARGS="$SBT_MAVEN_PROFILES_ARGS -Phive -Phive-0.12.0"
whew, finally. |
Test build #22602 has started for PR 2685 at commit
|
@zhzhan Had a glance at the Kryo issue you pointed out, it should be related to the POM inconsistency problem I mentioned above, but I'm not sure whether they are identical since this issue doesn't mention any specific Kryo version. I'll investigate this later. Thanks for pointing out this issue! Although Jenkins finally nods, I'm still puzzled with the root cause of the original build failure. We only know that introducing Kryo 2.22 prevents un-shaded Objenesis classes from being included in the assembly jar and thus breaks the core tests, but how and why? Also, is it 100% safe to downgrade Kryo 2.22 to Kryo 2.21 for Hive 0.13.1? |
Test build #22602 has finished for PR 2685 at commit
|
Test PASSed. |
Thanks guys for all your hard work on this! Merging to master. |
In /pom.xml
The 'hive-0.13.1' is overriding the hive.version back to the regular 'hive-0.13.1' version, instead of referencing 'hive-0.13.1a'. It seems However, I was building with |
@coderfi This is a good catch, would you mind to file a JIRA ticket for this? A PR would be even better :) |
Yes, here should be 0.13.1a. |
Yes - we just need to change that to 0.13.1a On Sun, Nov 2, 2014 at 8:05 PM, wangfei [email protected] wrote:
|
And note just to change |
@liancheng PR #3072 created (all of one line! :) ). |
instead of `hive.version=0.13.1`. e.g. mvn -Phive -Phive=0.13.1 Note: `hive.version=0.13.1a` is the default property value. However, when explicitly specifying the `hive-0.13.1` maven profile, the wrong one would be selected. References: PR #2685, which resolved a package incompatibility issue with Hive-0.13.1 by introducing a special version Hive-0.13.1a Author: fi <[email protected]> Closes #3072 from coderfi/master and squashes the following commits: 7ca4b1e [fi] Fixes the `hive-0.13.1` maven profile referencing `hive.version=0.13.1` instead of the Spark compatible `hive.version=0.13.1a` Note: `hive.version=0.13.1a` is the default version. However, when explicitly specifying the `hive-0.13.1` maven profile, the wrong one would be selected. e.g. mvn -Phive -Phive=0.13.1 See PR #2685 (cherry picked from commit df607da) Signed-off-by: Michael Armbrust <[email protected]>
instead of `hive.version=0.13.1`. e.g. mvn -Phive -Phive=0.13.1 Note: `hive.version=0.13.1a` is the default property value. However, when explicitly specifying the `hive-0.13.1` maven profile, the wrong one would be selected. References: PR #2685, which resolved a package incompatibility issue with Hive-0.13.1 by introducing a special version Hive-0.13.1a Author: fi <[email protected]> Closes #3072 from coderfi/master and squashes the following commits: 7ca4b1e [fi] Fixes the `hive-0.13.1` maven profile referencing `hive.version=0.13.1` instead of the Spark compatible `hive.version=0.13.1a` Note: `hive.version=0.13.1a` is the default version. However, when explicitly specifying the `hive-0.13.1` maven profile, the wrong one would be selected. e.g. mvn -Phive -Phive=0.13.1 See PR #2685
In #2241 hive-thriftserver is not enabled. This patch enable hive-thriftserver to support hive-0.13.1 by using a shim layer refer to #2241.
1 A light shim layer(code in sql/hive-thriftserver/hive-version) for each different hive version to handle api compatibility
2 New pom profiles "hive-default" and "hive-versions"(copy from #2241) to activate different hive version
3 SBT cmd for different version as follows:
hive-0.12.0 --- sbt/sbt -Phive,hadoop-2.3 -Phive-0.12.0 assembly
hive-0.13.1 --- sbt/sbt -Phive,hadoop-2.3 -Phive-0.13.1 assembly
4 Since hive-thriftserver depend on hive subproject, this patch should be merged with #2241 to enable hive-0.13.1 for hive-thriftserver