Skip to content
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-13090][hive] Test Hive connector with hive runner #8987

Closed
wants to merge 5 commits into from

Conversation

lirui-apache
Copy link
Contributor

What is the purpose of the change

To use hive runner in our hive connector tests.

Brief change log

  • Added a JUnit runner FlinkStandaloneHiveRunner which launches a standalone HMS and provides an embedded Hive shell. The standalone HMS is closer to real world deployment, and the Hive shell makes it easier for us to write test cases.
  • Changed HiveTableSinkTest to use FlinkStandaloneHiveRunner, so that we can use the Hive shell to verify data instead of reading from files ourselves. This will give us more confidence that the written data is hive-compatible.

Verifying this change

Adapted existing test cases.

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

@lirui-apache
Copy link
Contributor Author

cc @xuefuz @bowenli86 @zjuwangg
Please note although the PR comes with a shim for hive runner, I haven't verified it against Hive-1.2.1 since the build is currently broken. The shim is simply ported from our internal Blink.

@flinkbot
Copy link
Collaborator

flinkbot commented Jul 4, 2019

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 c0939e0 (Wed Aug 07 08:14:55 UTC 2019)

Warnings:

  • 1 pom.xml files were touched: Check for build and licensing issues.
  • 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

Copy link
Contributor

@xuefuz xuefuz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. PR looks good to me. One thing that I'm not sure of is about the license. Do we need to update the license file as we are introducing some external libraries (though for test only). Please verify.

@zjuwangg Since you're mostly familiar to HiveRunner, please also take a look. Thanks.

@lirui-apache
Copy link
Contributor Author

@xuefuz I don't think we need to worry about licenses because all the newly introduced deps are in test scope and only intended to be used during testing.

@xuefuz
Copy link
Contributor

xuefuz commented Jul 5, 2019

Sounds good then. +1 for the PR to be merged.

@zentol
Copy link
Contributor

zentol commented Jul 5, 2019

CI report for commit c9a63b5: SUCCESS Travis

Copy link
Member

@bowenli86 bowenli86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. I only have time to briefly look at the PR, and most part LGTM. @zjuwangg can you help to review this in detail?

@flinkbot
Copy link
Collaborator

flinkbot commented Jul 8, 2019

CI report for commit b3e5599: FAILURE Build

@bowenli86
Copy link
Member

@flinkbot attention @zjuwangg

Copy link
Member

@bowenli86 bowenli86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent time and took a closer look at this PR. LGTM

Can you rebase to master? @lirui-apache

@lirui-apache
Copy link
Contributor Author

Rebased and addressed comments. @bowenli86 please take another look, thanks.

@flinkbot
Copy link
Collaborator

flinkbot commented Jul 9, 2019

CI report for commit c0939e0: SUCCESS Build

@bowenli86
Copy link
Member

@lirui-apache LGTM, thanks for your contribution.

Merging

@asfgit asfgit closed this in 5d1e64e Jul 9, 2019
asfgit pushed a commit that referenced this pull request Jul 9, 2019
Resolve bad merge between #8965 and #8987 though they passed CI separately.
@lirui-apache lirui-apache deleted the FLINK-13090 branch July 10, 2019 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants