[KYUUBI #7244][SERVER] Skip invalid batch metadata during recovery#7521
[KYUUBI #7244][SERVER] Skip invalid batch metadata during recovery#7521hutiefang76 wants to merge 1 commit into
Conversation
|
|
||
| private def createBatchSessionFromRecoverySafely( | ||
| metadata: Metadata): Option[KyuubiBatchSession] = { | ||
| metadata.requestConf.collectFirst { case (key, null) => key } match { |
There was a problem hiding this comment.
has this bug been fixed? if so, document the history and the affected version
There was a problem hiding this comment.
Yes. I documented the history and affected versions in the PR description and verified them from the repository tags: batch-session recovery was introduced by KYUUBI #2373 (first released in 1.6.0-incubating), while Batch API V2 was introduced by #4790 (first released in 1.8.0). The reported V2 recovery failure is therefore confirmed for 1.8.0+ and current master. #6884 later added the explicit batch-reassignment recovery path; the helper in this PR guards both normal startup recovery and that reassignment path.
There was a problem hiding this comment.
what I mean bug fix is, filtering out the null-valued config when creating the batch ...
I remember having fixed it, seems not to have been pushed to upstream ...
There was a problem hiding this comment.
Thanks for clarifying. I reworked the patch around creation-time filtering in 34dcff8:
- The REST boundary removes null-valued entries before user/batch-id lookup and V2 metadata persistence.
- The shared createBatchSession boundary sanitizes both the configuration and attached metadata, so legacy INITIALIZED submission and PENDING/RUNNING recovery are covered as well.
- The previous broad recovery catch/ERROR transition was removed; unrelated recovery failures keep their existing behavior.
I added separate tests for REST persistence, INITIALIZED submission, and recovery. The targeted lifecycle suite passes 2/2; the new REST test passes in both V1 and V2 suites. I also documented the full test evidence, including the two unrelated Spark log-fixture failures reproduced on clean origin/master, in the PR description.
|
Thanks for the reminder. I updated the PR description with the history and affected-version scope: general batch-session recovery exists from 1.6.0-incubating+, Batch API V2 was introduced in 1.8.0+, and this issue is confirmed for the reported Batch V2 path on 1.8.0+ and current master. The patch also guards the current master explicit batch reassignment recovery path added later. |
|
Thanks for the fix. It addresses the recovery failure, but I’m still concerned about the full lifecycle of invalid metadata, so I’d prefer to hold off on merging for now. |
c7480c1 to
34dcff8
Compare
Why are the changes needed?
Null-valued batch configuration entries can reach persisted batch metadata and later fail when
KyuubiBatchSessioncopiesrequestConfintoKyuubiConf, whoseset(key, value)rejects null values.The fix now covers the full lifecycle instead of turning a recovery-time validation error into a terminal batch state:
createBatchSessionboundary, covering V1 creation, V2INITIALIZEDsubmission, andPENDING/RUNNINGrecovery;ERROR.The shared sanitizer also provides compatibility for legacy metadata that already contains a null value.
How was this patch tested?
BatchesResourceSuitecoverage proving V1 and V2 requests persist metadata without the null-valued entry.KyuubiSessionManagerSuitecoverage for bothINITIALIZEDsubmission andPENDING/RUNNINGrecovery; the batch remains recoverable and the null key is absent from the session configuration.open batch sessionlog assertions failed because the local Spark fixture did not emitSubmitted application: Spark Pi; the same two failures were reproduced on a detached cleanorigin/masterworktree with the sameSPARK_HOME.dev/reformatgit diff --checkWas this patch authored or co-authored using generative AI tooling?
Assisted-by: OpenAI Codex
Bug history and affected versions