[FLINK-13841][hive] Extend Hive version support to all 1.2 and 2.3 ve… #9524

xuefuz · 2019-08-23T22:49:14Z

…rsions

What is the purpose of the change

Support all 1.2 and 2.3 minor Hive versions instead of currently 1.2.1 and 2.3.4 only.

Brief change log

Added new shims for each version
Made inheritance relationships between the shims
Shimmed methods to deal with API difference

Verifying this change

This change is already covered by existing tests, and manually ran the tests for all versions.

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, Yarn/Mesos, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

…rsions

xuefuz · 2019-08-23T22:51:53Z

cc @bowenli86 @lirui-apache @zjuwangg

flinkbot · 2019-08-23T22:52:19Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit a077b6c (Tue Aug 27 18:37:48 UTC 2019)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2019-08-23T22:58:54Z

CI report:

b14e1be : SUCCESS Build
a077b6c : SUCCESS Build

bowenli86

@xuefuz thanks for the PR!

Except a few minor formatting issues, what are the testing plan for all these Hive versions? We probably need to run nightly build for all versions but not necessary for every PR build.

bowenli86 · 2019-08-23T23:13:38Z

...nk-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/client/HiveShimV230.java

 	@Override
 	public SimpleGenericUDAFParameterInfo createUDAFParameterInfo(ObjectInspector[] params, boolean isWindowing, boolean distinct, boolean allColumns) {
 		try {
 			Constructor constructor = SimpleGenericUDAFParameterInfo.class.getConstructor(ObjectInspector[].class,
-					boolean.class, boolean.class, boolean.class);


bowenli86 · 2019-08-23T23:13:43Z

...nk-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/client/HiveShimV230.java

 			return (SimpleGenericUDAFParameterInfo) constructor.newInstance(params, isWindowing, distinct, allColumns);
 		} catch (NoSuchMethodException | IllegalAccessException | InstantiationException | InvocationTargetException e) {
 			throw new CatalogException("Failed to create SimpleGenericUDAFParameterInfo", e);
 		}
 	}
+


This is actually desirable as there was missing a new line at the end of file.

bowenli86 · 2019-08-23T23:13:54Z

...nk-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/client/HiveShimV230.java

@@ -86,7 +88,7 @@ public Function getFunction(IMetaStoreClient client, String dbName, String funct
 	public boolean moveToTrash(FileSystem fs, Path path, Configuration conf, boolean purge) throws IOException {
 		try {
 			Method method = FileUtils.class.getDeclaredMethod("moveToTrash", FileSystem.class, Path.class,
-					Configuration.class, boolean.class);


bowenli86 · 2019-08-23T23:14:32Z

...nk-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/client/HiveShimV230.java

@@ -99,14 +101,35 @@ public void alterTable(IMetaStoreClient client, String databaseName, String tabl
 		client.alter_table(databaseName, tableName, table);
 	}

+	@Override
+	public void alterPartition(IMetaStoreClient client, String databaseName, String tableName, Partition partition)
+		throws InvalidOperationException, MetaException, TException {


nit: needs an extra tab

xuefuz

Re: testing plan

The general idea is to run tests against those versions at the end of a release cycle, including unit tests and end to end tests. Daily test against one version of each major version can continue, but we cannot do so for each version we support.

We can discuss more about testing should be there any disagreement.

xuefuz · 2019-08-26T18:29:07Z

PR updated based on review feedback. @bowenli86 could you take another look? Thanks.

bowenli86 · 2019-08-26T22:35:11Z

LGTM. W.r.t testing, shall we at least add build profiles for these newly added Hive versions in flink-connector-hive's pom so they can currently be run manually?

E.g.

		<profile>
			<id>hive-1.2.0</id>
			<properties>
				<hive.version>1.2.0</hive.version>
				<hivemetastore.hadoop.version>2.6.5</hivemetastore.hadoop.version>
				<hiverunner.version>3.2.1</hiverunner.version>
			</properties>
		</profile>
               ...

I created FLINK-13866 to track the testing plan.

xuefuz · 2019-08-26T23:54:22Z

LGTM. W.r.t testing, shall we at least add build profiles for these newly added Hive versions in flink-connector-hive's pom so they can currently be run manually?

E.g.
		<profile>
			<id>hive-1.2.0</id>
			<properties>
				<hive.version>1.2.0</hive.version>
				<hivemetastore.hadoop.version>2.6.5</hivemetastore.hadoop.version>
				<hiverunner.version>3.2.1</hiverunner.version>
			</properties>
		</profile>
               ...
I created FLINK-13866 to track the testing plan.

Let's discuss about this in the JIRA you created. For short comment, I don't think we need to provide a profile for each version we support. There are just too many of them. However, this and how to test in general can be discussed.

bowenli86 · 2019-08-27T18:00:08Z

Let's discuss about this in the JIRA you created. For short comment, I don't think we need to provide a profile for each version we support. There are just too many of them. However, this and how to test in general can be discussed.

It is alright, as there's no difference between shims in the same major versions (1.2 and 2.3) and the current testing setup still covers the change. Support of other major versions may need the corresponding testing plan to come along.

Xuefu Zhang added 2 commits August 23, 2019 15:40

[FLINK-13841][hive] Extend Hive version support to all 1.2 and 2.3 ve…

50195b8

…rsions

Minor code refactoring

b14e1be

rmetzger added the review=description? label Aug 23, 2019

rmetzger added the component=Connectors/Hive label Aug 23, 2019

bowenli86 reviewed Aug 23, 2019

View reviewed changes

xuefuz commented Aug 26, 2019

View reviewed changes

Updated based on review feedback

a077b6c

xuefuz closed this Aug 26, 2019

xuefuz reopened this Aug 26, 2019

asfgit closed this in 0437ad2 Aug 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-13841][hive] Extend Hive version support to all 1.2 and 2.3 ve… #9524

[FLINK-13841][hive] Extend Hive version support to all 1.2 and 2.3 ve… #9524

xuefuz commented Aug 23, 2019 •

edited

xuefuz commented Aug 23, 2019

flinkbot commented Aug 23, 2019 •

edited

flinkbot commented Aug 23, 2019 •

edited

bowenli86 left a comment

bowenli86 Aug 23, 2019

bowenli86 Aug 23, 2019

xuefuz Aug 26, 2019

bowenli86 Aug 23, 2019

bowenli86 Aug 23, 2019

xuefuz left a comment

xuefuz commented Aug 26, 2019

bowenli86 commented Aug 26, 2019 •

edited

xuefuz commented Aug 26, 2019

bowenli86 commented Aug 27, 2019 •

edited

[FLINK-13841][hive] Extend Hive version support to all 1.2 and 2.3 ve… #9524

[FLINK-13841][hive] Extend Hive version support to all 1.2 and 2.3 ve… #9524

Conversation

xuefuz commented Aug 23, 2019 • edited

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

xuefuz commented Aug 23, 2019

flinkbot commented Aug 23, 2019 • edited

Automated Checks

Review Progress

flinkbot commented Aug 23, 2019 • edited

CI report:

bowenli86 left a comment

Choose a reason for hiding this comment

bowenli86 Aug 23, 2019

Choose a reason for hiding this comment

bowenli86 Aug 23, 2019

Choose a reason for hiding this comment

xuefuz Aug 26, 2019

Choose a reason for hiding this comment

bowenli86 Aug 23, 2019

Choose a reason for hiding this comment

bowenli86 Aug 23, 2019

Choose a reason for hiding this comment

xuefuz left a comment

Choose a reason for hiding this comment

xuefuz commented Aug 26, 2019

bowenli86 commented Aug 26, 2019 • edited

xuefuz commented Aug 26, 2019

bowenli86 commented Aug 27, 2019 • edited

xuefuz commented Aug 23, 2019 •

edited

flinkbot commented Aug 23, 2019 •

edited

flinkbot commented Aug 23, 2019 •

edited

bowenli86 commented Aug 26, 2019 •

edited

bowenli86 commented Aug 27, 2019 •

edited