[KYUUBI #5797] Support to describe engine and restart engine with command in current session

[beryllw]

🔍 Description

Issue References 🔗

This pull request fixes #5797

Describe Your Solution 🔧

Support to describe and register/de-register engine with command in current session

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

Behavior With This Pull Request 🎉

user can use sql KYUUBI DEREGISTER ENGINE to stop spark engine, KYUUBI REGISTER ENGINE to restart spark engine, and KYUUBI DESCRIBE[DESC] ENGINE to show spark engine info.

Related Unit Tests

---

Checklists

📝 Author Self Checklist

• My code follows the style guidelines of this project
• I have performed a self-review
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• This patch was not authored or co-authored using Generative Tooling

📝 Committer Pre-Merge Checklist

• Pull request title is okay.
• No license issues.
• Milestone correctly set?
• Test coverage is ok
• Assignees are selected.
• Minimum number of approvals
• No changes are requested

Be nice. Be informative.

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (c290dae) 61.57% compared to head (651bf5f) 61.58%.
Report is 8 commits behind head on master.

❗ Current head 651bf5f differs from pull request most recent head 9c40671. Consider uploading reports for the commit 9c40671 to get more accurate results

Files	Patch %	Lines
...pache/kyuubi/sql/plan/command/DescribeEngine.scala	94.11%	1 Missing ⚠️
...apache/kyuubi/sql/plan/command/RestartEngine.scala	94.44%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #5871      +/-   ##
============================================
+ Coverage     61.57%   61.58%   +0.01%     
  Complexity       23       23              
============================================
  Files           616      618       +2     
  Lines         36386    36441      +55     
  Branches       4978     4978              
============================================
+ Hits          22404    22442      +38     
- Misses        11570    11576       +6     
- Partials       2412     2423      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[beryllw]

@turboFei @pan3793 cc

[wForget]

This will re-execute openEngineSession and we need to consider whether some states are as expected.

• openedTime will be reset: sessionEvent.openedTime = System.currentTimeMillis()
• do we need to update engineLastAlive?
• EngineRef#builder is changed, volatile required?

[beryllw]

fixed engineLastAlive , EngineRef#builder.
I think openedTime means engine openedTime, so we don't need to update openedTime.And i changed the org.apache.kyuubi.events.KyuubiSessionEvent#openedTime comment.

[turboFei]

You shall need to wrap this method with _client.withLockAcquired.

[turboFei]

in case that other operations access the _client during restarting engine.

[wForget]

We may not be able to use the lock of _client because _client will be assigned a new object after this method.

[wForget]

in case that other operations access the _client during restarting engine.

This seems difficult to avoid

[wForget]

Or we need to add ReadWriteLock for the client method of KyuubiSessionImpl.

[beryllw]

Or we need to add ReadWriteLock for the client method of KyuubiSessionImpl.

submit Operation from multiple threads will meet problem , I will fix it.

[turboFei]

please rebase your code.

[beryllw]

please rebase your code.

OK

[bowenliang123]

I have some concerns and suggestions,

1. separate two features in each PR, which are not coupled with each other.
2. clarify the gap between description and implementation , as the title and description says de-register engine while it implements re-start engine
3. describe the design and reason why we have to have restarting engine, which is not that reasonable to me
4. is that reslaunching engine a sync operation, and what happen if the restaring engine breaks the timeouts for either launching or session
5. authorization first. What happens if the current engine is used by other sessions even after deregisted. In what case and combination situation allows one to restart the engine?

[wForget]

3. describe the design and reason why we have to have restarting engine, which is not that reasonable to me

There are some related discussions in #5797 (comment)

is that reslaunching engine a sync operation

I think we should aim to make this a synchronous operation

and what happen if the restaring engine breaks the timeouts for either launching or session

I think it's similar to the error that occurs when opening a session

5. authorization first. What happens if the current engine is used by other sessions even after deregisted. In what case and combination situation allows one to restart the engine?

The engine will go offline gracefully without affecting other sessions.

[beryllw]

clarify the gap between description and implementation , as the title and description says de-register engine while it implements re-start engine

fixed

separate two features in each PR, which are not coupled with each other.

desc engine is simple, seems no need to separate

[beryllw]

Some errors occurred in the BatchesResource test, try to understand the relevant code to solve it.

[beryllw]

1. separate two features in each PR, which are not coupled with each other.

For restart engine not simple, i will separate two features in each PR.Support desc engine in #5931

When multi-threads use a session to submit operations, restart engine will close the current KyuubiSyncThriftClient session, and this will cause other operations of the client to fail.

The temporary purpose and requirements are #5797 (comment). I will design and implement in later. Are there any other supplementary scenarios and requirements?