[KYUUBI #5987] Enhance KYUUBI_HOME detection in shell script to handle softlink cases #5988

Emor-nj · 2024-01-17T08:17:01Z

🔍 Description

Issue References 🔗

This pull request fixes #5987
Modify the logic in the beeline script that specifies the KYUUBI_HOME environment variable.

Describe Your Solution 🔧

If the environment has already declared KYUUBI_HOME, it should use the pre-declared KYUUBI_HOME instead of forcefully specifying the parent directory of the current one.

Types of changes 🔖

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

set soft link:
ln -s /opt/soft/kyuubi/kyuubi/bin/beeline /usr/bin/kyuubi-beeline
set env like:
export KYUUBI_HOME=/opt/soft/kyuubi/kyuubi
run kyuubi-beeline:
kyuubi-beeline -u "jdbc:hive2://xx.xx.xx.xx:2181,xx.xx.xx.xx:2181,xx.xx.xx.xx:2181/tmp;serviceDiscoveryMode=zooKeeper;zooKeeperNamespace=kyuubi;principal=hive/_HOST@xxx"
command will fail:
/bin/kyuubi-beeline: line 27: //bin/load-kyuubi-env.sh: No such file or directory Error: JAVA_HOME IS NOT SET! CANNOT PROCEED.

Behavior With This Pull Request 🎉

set soft link:
ln -s /opt/soft/kyuubi/kyuubi/bin/beeline /usr/bin/kyuubi-beeline
set env like:
export KYUUBI_HOME=/opt/soft/kyuubi/kyuubi
run kyuubi-beeline:
kyuubi-beeline -u "jdbc:hive2://xx.xx.xx.xx:2181,xx.xx.xx.xx:2181,xx.xx.xx.xx:2181/tmp;serviceDiscoveryMode=zooKeeper;zooKeeperNamespace=kyuubi;principal=hive/_HOST@xxx"
command will success

Related Unit Tests

Checklist 📝

This patch was not authored or co-authored using Generative Tooling

Be nice. Be informative.

… environment variable.

Emor-nj · 2024-01-17T08:19:48Z

cc @pan3793

pan3793 · 2024-01-17T08:21:58Z

The change makes sense to me, mind supplying poistive/negative test cases in the PR description?

pan3793 · 2024-01-17T08:26:15Z

seems we have the same issue on the other scripts.

actually, what we want to do here is to follow the soft link to find the real file, then locale the load-kyuubi-env.sh via the relative path. if we can do that, we don't even declare the evn KYUUBI_HOME before executing the soft link of those scripts

pan3793 · 2024-01-17T08:38:23Z

I checked Spark's scripts, let's simply use this pattern

if [ -z "${KYUUBI_HOME}" ]; then
  KYUUBI_HOME="$(cd "`dirname "$0"`"/..; pwd)"
fi

Emor-nj · 2024-01-17T08:42:28Z

seems we have the same issue on the other scripts.

actually, what we want to do here is to follow the soft link to find the real file, then locale the load-kyuubi-env.sh via the relative path. if we can do that, we don't even declare the evn KYUUBI_HOME before executing the soft link of those scripts

Good idea! Here's the modified line:
export KYUUBI_HOME="$(cd "$(dirname "$(realpath "$0")")"/..; pwd)"
If there are no issues, I will proceed to uniformly modify all related scripts.

Emor-nj · 2024-01-17T08:44:57Z

I checked Spark's scripts, let's simply use this pattern
if [ -z "${KYUUBI_HOME}" ]; then
  KYUUBI_HOME="$(cd "`dirname "$0"`"/..; pwd)"
fi

Is the intention to maintain consistency with Spark by prioritizing the use of environment variables rather than directly obtaining the absolute path?

pan3793 · 2024-01-17T08:54:51Z

is realpath available on all UNIX-like OS? I saw Spark handles it exceptionally

Is the intention to maintain consistency with Spark ...

I haven't seen users report similar issues to Spark, thus I suppose the approach Spark used is reliable, it always is my first fallback candidate if no better options.

Emor-nj · 2024-01-17T08:59:21Z

is realpath available on all UNIX-like OS? I saw Spark handles it exceptionally

Is the intention to maintain consistency with Spark ...

I haven't seen users report similar issues to Spark, thus I suppose the approach Spark used is reliable, it always is my first fallback candidate if no better options.

Get it, let's align with Spark. I will uniformly modify all related scripts.

pan3793 · 2024-01-17T09:37:19Z

bin/beeline

@@ -19,7 +19,9 @@
 ## Kyuubi BeeLine Entrance
 CLASS="org.apache.hive.beeline.KyuubiBeeLine"

-export KYUUBI_HOME="${KYUUBI_HOME:-"$(cd "$(dirname "$0")"/.. || exit; pwd)"}"
+if [ -z "${KYUUBI_HOME}" ]; then
+  export KYUUBI_HOME="$(cd "`dirname "$0"`"/..; pwd)"


export is not required

pan3793

LGTM

pan3793 · 2024-01-17T09:52:52Z

bin/load-kyuubi-env.sh

@@ -18,7 +18,7 @@


 if [ -z "${KYUUBI_HOME}" ]; then
-  export ${KYUUBI_HOME:-"$(cd "$(dirname "$0")"/.. || exit; pwd)"}
+  export KYUUBI_HOME="${KYUUBI_HOME:-"$(cd "$(dirname "$0")"/.. || exit; pwd)"}"


KYUUBI_HOME is always empty

Suggested change

export KYUUBI_HOME="${KYUUBI_HOME:-"$(cd "$(dirname "$0")"/.. || exit; pwd)"}"

export KYUUBI_HOME="$(cd "$(dirname "$0")"/.. || exit; pwd)"

get it, thanks

…e softlink cases # 🔍 Description ## Issue References 🔗 This pull request fixes #5987 Modify the logic in the beeline script that specifies the KYUUBI_HOME environment variable. ## Describe Your Solution 🔧 If the environment has already declared KYUUBI_HOME, it should use the pre-declared KYUUBI_HOME instead of forcefully specifying the parent directory of the current one. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ 1. set soft link: `ln -s /opt/soft/kyuubi/kyuubi/bin/beeline /usr/bin/kyuubi-beeline` 2. set env like: `export KYUUBI_HOME=/opt/soft/kyuubi/kyuubi` 3. run kyuubi-beeline: `kyuubi-beeline -u "jdbc:hive2://xx.xx.xx.xx:2181,xx.xx.xx.xx:2181,xx.xx.xx.xx:2181/tmp;serviceDiscoveryMode=zooKeeper;zooKeeperNamespace=kyuubi;principal=hive/_HOSTxxx"` 4. command will fail: `/bin/kyuubi-beeline: line 27: //bin/load-kyuubi-env.sh: No such file or directory Error: JAVA_HOME IS NOT SET! CANNOT PROCEED.` #### Behavior With This Pull Request 🎉 1. set soft link: `ln -s /opt/soft/kyuubi/kyuubi/bin/beeline /usr/bin/kyuubi-beeline` 2. set env like: `export KYUUBI_HOME=/opt/soft/kyuubi/kyuubi` 3. run kyuubi-beeline: `kyuubi-beeline -u "jdbc:hive2://xx.xx.xx.xx:2181,xx.xx.xx.xx:2181,xx.xx.xx.xx:2181/tmp;serviceDiscoveryMode=zooKeeper;zooKeeperNamespace=kyuubi;principal=hive/_HOSTxxx"` 4. command will success #### Related Unit Tests --- # Checklist 📝 - [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) **Be nice. Be informative.** Closes #5988 from Emor-nj/kyuubi-beeline. Closes #5987 3a988e0 [Emor-nj] fix KYUUBI_HOME is always empty in load-kyuubi-env.sh cc70387 [Emor-nj] fix load-kyuubi-env.sh a101428 [Emor-nj] Remove unnecessary exports. f48db3f [Emor-nj] uniformly modify all related scripts. 35215b9 [Emor-nj] Modify the logic in the beeline script that specifies the KYUUBI_HOME environment variable. Authored-by: Emor-nj <[email protected]> Signed-off-by: Cheng Pan <[email protected]> (cherry picked from commit 7b38889) Signed-off-by: Cheng Pan <[email protected]>

pan3793 · 2024-01-17T10:03:07Z

Thanks, merged to master/1.8

… handle softlink cases # 🔍 Description ## Issue References 🔗 This pull request fixes apache#5987 Modify the logic in the beeline script that specifies the KYUUBI_HOME environment variable. ## Describe Your Solution 🔧 If the environment has already declared KYUUBI_HOME, it should use the pre-declared KYUUBI_HOME instead of forcefully specifying the parent directory of the current one. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ 1. set soft link: `ln -s /opt/soft/kyuubi/kyuubi/bin/beeline /usr/bin/kyuubi-beeline` 2. set env like: `export KYUUBI_HOME=/opt/soft/kyuubi/kyuubi` 3. run kyuubi-beeline: `kyuubi-beeline -u "jdbc:hive2://xx.xx.xx.xx:2181,xx.xx.xx.xx:2181,xx.xx.xx.xx:2181/tmp;serviceDiscoveryMode=zooKeeper;zooKeeperNamespace=kyuubi;principal=hive/_HOSTxxx"` 4. command will fail: `/bin/kyuubi-beeline: line 27: //bin/load-kyuubi-env.sh: No such file or directory Error: JAVA_HOME IS NOT SET! CANNOT PROCEED.` #### Behavior With This Pull Request 🎉 1. set soft link: `ln -s /opt/soft/kyuubi/kyuubi/bin/beeline /usr/bin/kyuubi-beeline` 2. set env like: `export KYUUBI_HOME=/opt/soft/kyuubi/kyuubi` 3. run kyuubi-beeline: `kyuubi-beeline -u "jdbc:hive2://xx.xx.xx.xx:2181,xx.xx.xx.xx:2181,xx.xx.xx.xx:2181/tmp;serviceDiscoveryMode=zooKeeper;zooKeeperNamespace=kyuubi;principal=hive/_HOSTxxx"` 4. command will success #### Related Unit Tests --- # Checklist 📝 - [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) **Be nice. Be informative.** Closes apache#5988 from Emor-nj/kyuubi-beeline. Closes apache#5987 3a988e0 [Emor-nj] fix KYUUBI_HOME is always empty in load-kyuubi-env.sh cc70387 [Emor-nj] fix load-kyuubi-env.sh a101428 [Emor-nj] Remove unnecessary exports. f48db3f [Emor-nj] uniformly modify all related scripts. 35215b9 [Emor-nj] Modify the logic in the beeline script that specifies the KYUUBI_HOME environment variable. Authored-by: Emor-nj <[email protected]> Signed-off-by: Cheng Pan <[email protected]>

Modify the logic in the beeline script that specifies the KYUUBI_HOME…

35215b9

… environment variable.

github-actions bot added module:hive module:ctl labels Jan 17, 2024

uniformly modify all related scripts.

f48db3f

github-actions bot added module:kubernetes module:server kind:build kind:deploy labels Jan 17, 2024

cxzl25 changed the title ~~[KYUUBI #5987]Modify the logic in the beeline script that specifies the KYUUBI_HOME environment variable.~~ [KYUUBI #5987] Modify the logic in the beeline script that specifies the KYUUBI_HOME environment variable Jan 17, 2024

pan3793 reviewed Jan 17, 2024

View reviewed changes

github-actions bot removed module:kubernetes kind:build labels Jan 17, 2024

pan3793 changed the title ~~[KYUUBI #5987] Modify the logic in the beeline script that specifies the KYUUBI_HOME environment variable~~ [KYUUBI #5987] Enhance KYUUBI_HOME detection in shell script to handle softlink cases Jan 17, 2024

pan3793 approved these changes Jan 17, 2024

View reviewed changes

Remove unnecessary exports.

a101428

Emor-nj force-pushed the kyuubi-beeline branch from 57504e1 to a101428 Compare January 17, 2024 09:47

fix load-kyuubi-env.sh

cc70387

pan3793 reviewed Jan 17, 2024

View reviewed changes

fix KYUUBI_HOME is always empty in load-kyuubi-env.sh

3a988e0

pan3793 added this to the v1.8.1 milestone Jan 17, 2024

pan3793 assigned Emor-nj Jan 17, 2024

pan3793 closed this in 7b38889 Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KYUUBI #5987] Enhance KYUUBI_HOME detection in shell script to handle softlink cases #5988

[KYUUBI #5987] Enhance KYUUBI_HOME detection in shell script to handle softlink cases #5988

Emor-nj commented Jan 17, 2024 •

edited

Loading

Emor-nj commented Jan 17, 2024

pan3793 commented Jan 17, 2024

pan3793 commented Jan 17, 2024

pan3793 commented Jan 17, 2024

Emor-nj commented Jan 17, 2024

Emor-nj commented Jan 17, 2024

pan3793 commented Jan 17, 2024

Emor-nj commented Jan 17, 2024

pan3793 Jan 17, 2024

pan3793 left a comment

pan3793 Jan 17, 2024

Emor-nj Jan 17, 2024

pan3793 commented Jan 17, 2024

	export KYUUBI_HOME="${KYUUBI_HOME:-"$(cd "$(dirname "$0")"/.. \|\| exit; pwd)"}"
	export KYUUBI_HOME="$(cd "$(dirname "$0")"/.. \|\| exit; pwd)"

[KYUUBI #5987] Enhance KYUUBI_HOME detection in shell script to handle softlink cases #5988

[KYUUBI #5987] Enhance KYUUBI_HOME detection in shell script to handle softlink cases #5988

Conversation

Emor-nj commented Jan 17, 2024 • edited Loading

🔍 Description

Issue References 🔗

Describe Your Solution 🔧

Types of changes 🔖

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklist 📝

Emor-nj commented Jan 17, 2024

pan3793 commented Jan 17, 2024

pan3793 commented Jan 17, 2024

pan3793 commented Jan 17, 2024

Emor-nj commented Jan 17, 2024

Emor-nj commented Jan 17, 2024

pan3793 commented Jan 17, 2024

Emor-nj commented Jan 17, 2024

pan3793 Jan 17, 2024

Choose a reason for hiding this comment

pan3793 left a comment

Choose a reason for hiding this comment

pan3793 Jan 17, 2024

Choose a reason for hiding this comment

Emor-nj Jan 17, 2024

Choose a reason for hiding this comment

pan3793 commented Jan 17, 2024

Emor-nj commented Jan 17, 2024 •

edited

Loading