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

Allow setting hive conf which doesn't have a hivevar #39

Merged
merged 3 commits into from
May 12, 2020

Conversation

amalakar
Copy link
Contributor

We use this library to test metastore audit logger plugin we use. The plugin uses configurations like http url of ingestion service etc. As of now there is no ability to set HiveConf.ConfVars in BeejuCore. This adds the ability to set custom configuration keys by string in hive conf object.

@amalakar amalakar requested a review from a team May 12, 2020 16:15
@coveralls
Copy link

coveralls commented May 12, 2020

Coverage Status

Coverage decreased (-0.5%) to 68.1% when pulling 0631a7c on amalakar:add_hive_conf into 52fc718 on HotelsDotCom:master.

Copy link
Contributor

@massdosage massdosage left a comment

Choose a reason for hiding this comment

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

If you could please add a test and also one line to the CHANGELOG.md describing your change that would be great. Thanks for the contribution!

@@ -101,6 +101,10 @@ void setHiveVar(HiveConf.ConfVars variable, String value) {
conf.setVar(variable, value);
}

void setHiveConf(String variable, String value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so this allows one to set arbitrary String values in the HiveConf as opposed to only the ones that Hive itself exposes via HiveConf.ConfVars. It looks like a reasonable addition to me and I can see how this could be useful. Could you please add a simple unit test for this along the lines of https://github.com/HotelsDotCom/beeju/blob/master/src/test/java/com/hotels/beeju/core/BeejuCoreTest.java#L75?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I made the changes.

Copy link
Contributor

@patduin patduin left a comment

Choose a reason for hiding this comment

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

Looks good thanks for the contribution! If @massdosage approves as well, we'll sort out a release.

@massdosage massdosage merged commit 6dc0e83 into ExpediaGroup:master May 12, 2020
@massdosage
Copy link
Contributor

Thanks for your contribution, we'll try get a release out in the next day or two.

@amalakar amalakar deleted the add_hive_conf branch May 12, 2020 20:59
@amalakar
Copy link
Contributor Author

Thank you @massdosage and @patduin for the quick turn around on the PR. I love this library, has been very handy in testing our internal metastore plugins.

@patduin
Copy link
Contributor

patduin commented May 13, 2020

@amalakar FYI just release BeeJu 3.1.0 should be propagating to maven central in a few moments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants