-
Notifications
You must be signed in to change notification settings - Fork 951
[WIP][KYUUBI #3723]Add SQL session state management and save metadata #7143
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
base: master
Are you sure you want to change the base?
Conversation
- Add a new SessionState enum to define possible session states. - Insert or update metadata at different stages of the session lifecycle.
Hi @turboFei and other maintainers, I've just submitted this as a Draft PR for some early feedback. Could you please take a look at the general approach and let me know if there are any initial suggestions? Thanks! |
@@ -73,6 +77,39 @@ class KyuubiSessionImpl( | |||
case (key, value) => sessionConf.set(key, value) | |||
} | |||
|
|||
val (clusterManager, kubernetesInfo) = engine.engineType match { |
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.
it is not accurate here
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.
For the clusterManager
, I think you can leave it empty and update it after getting the engine info.
But seems it does not matter.
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.
FYI:
kyuubi/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/AdminResource.scala
Lines 311 to 312 in 5f4b1f0
val appMgrInfo = | |
engineNode.attributes.get(KyuubiReservedKeys.KYUUBI_ENGINE_APP_MGR_INFO_KEY) |
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.
Got it, thanks for the guidance! I will get this updated.
SessionState.validateTransition(state, newState) | ||
newState match { | ||
case SessionState.RUNNING => | ||
info(s"Session is ready.") |
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.
this kind of log is meaningless without session identifier
case SessionState.RUNNING => | ||
info(s"Session is ready.") | ||
case SessionState.ERROR | SessionState.TIMEOUT | SessionState.CLOSED => | ||
info(s"Session is terminated.") |
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.
ditto
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.
and please log the state
Why are the changes needed?
#3723
How was this patch tested?
No,it's just a draft.
Was this patch authored or co-authored using generative AI tooling?
No.