[KYUUBI #5315][FLINK] Propagate HIVE_CONF_DIR on launching Flink engine #5316
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hadoopkandy
changed the title
|
Also, cc @YesOrNo828 |
pan3793
changed the title
pan3793
reviewed
Sep 20, 2023
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala
Outdated
Show resolved
Hide resolved
pan3793
reviewed
Sep 20, 2023
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #5316 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 590 590
Lines 33429 33436 +7
Branches 4423 4423
======================================
- Misses 33429 33436 +7
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
hadoopkandy
force-pushed
the
KYUUBI-5315
branch
from
f09e492
to
2aec430
Compare
pan3793
approved these changes
Sep 21, 2023
YesOrNo828
reviewed
Sep 21, 2023
| @@ -104,13 +104,23 @@ class FlinkProcessBuilder( | |||
| val userJars = conf.get(ENGINE_FLINK_APPLICATION_JARS) | |||
| userJars.foreach(jars => flinkExtraJars ++= jars.split(",")) | |||
|
|
|||
| val hiveConfDirOpt = env.get("HIVE_CONF_DIR") | |||
| hiveConfDirOpt.foreach { hiveConfDir => | |||
| val hiveConf = s"${hiveConfDir}${File.separator}hive-site.xml" | |||
There was a problem hiding this comment.
Would it be better to check if the hive-site.xml file exists here and throw an exception if it doesn't? @hadoopkandy WDYT?
…ng Flink engine
hadoopkandy
force-pushed
the
KYUUBI-5315
branch
from
2aec430
to
3f12acb
Compare
pan3793
pushed a commit
that referenced
this pull request
Sep 21, 2023
### _Why are the changes needed?_ create a Hive catalog in Flink SQL Client as follows: ``` CREATE CATALOG myhive WITH ( 'type' = 'hive', 'hive-conf-dir' = '/opt/hive-conf' ) ``` we should propagate `hive-conf-dir` and pass `HIVE_CONF_DIR` as an env variable to the AppMaster ### _How was this patch tested?_ - [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [ ] [Run test](https://kyuubi.readthedocs.io/en/master/contributing/code/testing.html#running-tests) locally before make a pull request ### _Was this patch authored or co-authored using generative AI tooling?_ No. Closes #5316 from hadoopkandy/KYUUBI-5315. Closes #5315 3f12acb [kandy01.wang] [KYUUBI #5315] [Improvement] Propagate HIVE_CONF_DIR on launching Flink engine Authored-by: kandy01.wang <kandy01.wang@vipshop.com> Signed-off-by: Cheng Pan <chengpan@apache.org> (cherry picked from commit ffebc64) Signed-off-by: Cheng Pan <chengpan@apache.org>
|
Thanks, merged to master/1.8 |
|
@hadoopkandy would you mind linking your email kandy01.wang@vipshop.com to GitHub account? otherwise, your contribution won't be counted by GitHub |
link3280
reviewed
Sep 21, 2023
There was a problem hiding this comment.
@hadoopkandy I left some comments. PTAL. We could make follow-up PR if necessary.
| val builder = new FlinkProcessBuilder("paullam", applicationModeConf) { | ||
| override def env: Map[String, String] = envDefault | ||
| override def env: Map[String, String] = envWithAllHadoop |
There was a problem hiding this comment.
It'd be better to add a new test with hive conf env, leaving this test with default env as a negative case to ensure compatibility with the simplest default setup.
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala
Show resolved
Hide resolved
Never mind |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are the changes needed?
create a Hive catalog in Flink SQL Client as follows:
we should propagate
hive-conf-dirand passHIVE_CONF_DIRas an env variable to the AppMasterHow was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request
Was this patch authored or co-authored using generative AI tooling?
No.