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

[KYUUBI #4515] Capturing process id for kyuubi server launched using run command #6374

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Madhukar98
Copy link

…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:

  1. A get_pid function has been created to capture the PID of the process launched using the run command.
  2. A check has been added before starting a Kyuubi server to ensure that it's not already running in a different mode.

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 ⚰️

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.

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.44%. Comparing base (78e104d) to head (bbee33a).
Report is 226 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@cxzl25 cxzl25 changed the title [KYUUBI #4515] Capturing process id for kyuubi server launched using … [KYUUBI #4515] Capturing process id for kyuubi server launched using run command May 9, 2024
bin/kyuubi Outdated
exit 1
fi

run_pid="$(ps -ef | grep kyuubi | grep java | grep -v grep | awk '{print $2}')"
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. grep kyuubi is not sufficient to ensure that this is a Kyuubi Server process. For example, Kyuubi Engine processes also have kyuubi in their info.
  2. 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.

Copy link
Author

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.

Copy link
Contributor

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

  1. org.apache.kyuubi.server.KyuubiServer - Ensures its an server process
  2. $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

@Madhukar98 Madhukar98 force-pushed the sync_run_start branch 2 times, most recently from bbee33a to c4ec781 Compare October 26, 2024 20:50
@Madhukar525722
Copy link
Contributor

Madhukar525722 commented Oct 28, 2024

Hi @pan3793 @bowenliang123 @zhouyifan279. Please review the implementation. Thanks

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

Successfully merging this pull request may close these issues.

4 participants