-
Notifications
You must be signed in to change notification settings - Fork 917
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
[KYUUBI #4515] Capturing process id for kyuubi server launched using run command #6374
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6374 +/- ##
============================================
- Coverage 58.47% 58.44% -0.04%
Complexity 24 24
============================================
Files 653 653
Lines 39881 39895 +14
Branches 5481 5482 +1
============================================
- Hits 23319 23315 -4
- Misses 14070 14080 +10
- Partials 2492 2500 +8 ☔ View full report in Codecov by Sentry. |
bin/kyuubi
Outdated
exit 1 | ||
fi | ||
|
||
run_pid="$(ps -ef | grep kyuubi | grep java | grep -v grep | awk '{print $2}')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grep kyuubi
is not sufficient to ensure that this is a Kyuubi Server process. For example, Kyuubi Engine processes also havekyuubi
in their info.- User may have multiple Kyuubi installations on a single machine. We need to ensure that this Kyuubi Server process is launched using current Kyuubi installation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @zhouyifan279 for your feedback. I will work on these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zhouyifan279 , I have updated the implementation, now I am making the selection on
- org.apache.kyuubi.server.KyuubiServer - Ensures its an server process
- $KYUUBI_CONF_DIR - Kyuubi conf dir path, displayed in the complete process details. This identifier is choosen to differentiate between the instances, because different instances cant run with same conf, as it will cause port already in use error.
Please review the changes.
Thanks
bbee33a
to
c4ec781
Compare
…using run command
c69ab1b
to
6be56ea
Compare
Hi @pan3793 @bowenliang123 @zhouyifan279. Please review the implementation. Thanks |
…run command
🔍 Description
Issue References 🔗
This pull request fixes # #4515
Describe Your Solution 🔧
Currently, the process ID (PID) for a Kyuubi server launched using bin/kyuubi run is not being captured. This leads to the bin/kyuubi status command incorrectly reporting that the server is not running.
Furthermore, if the bin/kyuubi run command is initiated at the launch of a Docker container, subsequent attempts to run bin/kyuubi start will fail due to the error "ports already occupied".
To resolve these issues and ensure synchronization between the status of the Kyuubi server whether it's launched in the foreground with bin/kyuubi run or in the background with bin/kyuubi start, two changes have been made:
Types of changes 🔖
Test Plan 🧪
Behavior Without This Pull Request ⚰️
bin/kyuubi run -> Launches kyuubi server
bin/kyuubi status -> Kyuubi is not running
bin/kyuubi start -> Fails with port already occupied
Behavior With This Pull Request 🎉
bin/kyuubi run -> Launches kyuubi server
bin/kyuubi status -> Kyuubi is running (PID)
bin/kyuubi start -> Kyuubi running as process PID. Stop it first.
Related Unit Tests
Checklist 📝
Be nice. Be informative.