[FLINK-12715][hive] Hive-1.2.1 build is broken

[lirui-apache]

What is the purpose of the change

To fix hive-1.2.1 build. The build was broken because some util method used in FLINK-12568 is not compatible in Hive-2.3.4 and Hive-1.2.1.

Brief change log

• Added shim for FileUtils.moveToTrash.
• Added test to cover it.

Verifying this change

Added new test and tested for both 2.3.4 and 1.2.1 (manually).

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? NA

[flinkbot]

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.

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.

Details

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

[lirui-apache]

cc @zjuwangg @bowenli86 @xuefuz

[xuefuz]

LGTM

[bowenli86]

I wonder that, sincemoveToTrash() is mainly a util, should we move it to a separate util shim class like HiveUtilShim? The current HiveShim class, as its javadoc says, is mainly for Hive Metastore. We probably should rename it to HiveClientShim?

LGTM otherwise.

[xuefuz]

I wonder that, sincemoveToTrash() is mainly a util, should we move it to a separate util shim class like HiveUtilShim? The current HiveShim class, as its javadoc says, is mainly for Hive Metastore. We probably should rename it to HiveClientShim?

LGTM otherwise.

HiveShim is quite generic in naming. It's a shim, so in theory anything can be put there. Maybe we just need to update the javadoc to reflect the new addition.

Having another shim for utils might be an overkill for now. However, we can keep an eye on this and see how we organize better when this layer gets too heavy.

[lirui-apache]

I also prefer to keep HiveShim generic. Actually APIs in HiveShim like getViews and getFunction take a IMetaStoreClient as parameter. So in that sense it's generic in nature.
@bowenli86 what do you think?

[bowenli86]

Make sense, let's keep it in HiveShim for now.

[bowenli86]

Thanks @lirui-apache for the PR and review from @xuefuz .

LGTM. Merging