[fix](routine-load) Set cancel reason when parsing create routine load stmt failed in gsonPostProcess - #66173
Open
re20052 wants to merge 1 commit into
Open
[fix](routine-load) Set cancel reason when parsing create routine load stmt failed in gsonPostProcess#66173re20052 wants to merge 1 commit into
re20052 wants to merge 1 commit into
Conversation
…d stmt failed in gsonPostProcess
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29529 ms |
Contributor
TPC-DS: Total hot run time: 176438 ms |
Contributor
ClickBench: Total hot run time: 25 s |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: the new reason is visible immediately for most parser/validation failures, but it is neither checkpoint-durable nor actionable for the missing-database path; see the inline finding.
Critical checkpoints:
- Goal and tests: The one-line assignment reaches both
SHOW ROUTINE LOADand the FE RPC, but the two cases above leave the requested diagnostic empty or generic. No existing test invokes a failinggsonPostProcessand verifies display plus checkpoint/reload behavior. - Scope and clarity: The edit is small and focused, but persisting the diagnostic and using a context-rich database lookup are required for the stated end-to-end behavior.
- Concurrency and lifecycle: Image deserialization occurs before manager publication, so the assignment adds no lock or race risk. The relevant lifecycle is image load -> post-process -> checkpoint -> reload; that lifecycle exposes the durability defect.
- Configuration and compatibility: No configuration, FE-BE protocol, symbol, or storage-format incompatibility is introduced. An optional serialized reason must remain backward-readable.
- Parallel paths and conditions: Kafka/Kinesis subclasses share this base path. SQL SHOW and the FE RPC both call
getStateReason, so both are affected. The concrete special case is an absent database before parsing begins. - Testing and results: No local build or test was run because the authoritative review prompt forbids builds. Existing CI compile, FE UT, and regression checks pass. The FE coverage job failure is infrastructure-related (missing
avro-scanner.jarwhile producing its report), not proof of this change's coverage. - Observability, persistence, and data safety: The existing warning log retains the stack trace, but the user-facing reason needs the fixes above. No transaction, table-data write, visible-version, or FE-BE variable propagation path changes.
- Performance and other risks: The exceptional-path allocation is negligible. No additional correctness, locking, configuration, compatibility, or performance issue was substantiated.
User focus: no additional focus was supplied; the full PR was reviewed.
Review status: complete for reviewed head 928c7cba3893349ea70408f87b65613c6c4bd676 after convergence.
| } | ||
| } catch (Exception e) { | ||
| this.state = JobState.CANCELLED; | ||
| this.cancelReason = new ErrorReason(InternalErrorCode.INTERNAL_ERR, e.getMessage()); |
Contributor
There was a problem hiding this comment.
[P1] Make the reported cancellation reason durable and actionable
This assignment fixes only the current deserialization. RoutineLoadJob.write() uses GsonUtils.GSON, whose HiddenAnnotationExclusionStrategy omits fields without @SerializedName; cancelReason is unannotated even though state is persisted. A later checkpoint can therefore store CANCELLED but drop this reason, and if the original statement validates on the next restart, SHOW ALL ROUTINE LOAD is empty again. There is also a first-load path where getDb(dbId).get() handles a missing database by throwing NoSuchElementException, so e.getMessage() is only "No value present" and does not identify the database. Please use a context-rich lookup, persist the reason without overwriting an already persisted terminal reason, and add a failing-postprocess -> SHOW/RPC -> checkpoint -> reload test (including the missing-database case).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
When FE restarts and replays routine load jobs via
gsonPostProcess, if parsing the original SQL statement fails, the job state is set toCANCELLEDbut thecancelReasonis left null. This causes theReasonOfStateChangedcolumn to be empty when users executeSHOW ALL ROUTINE LOAD;, hiding the actual failure cause.This PR sets the exception message as the
cancelReasonwhen parsing fails, so users can see the exact reason (e.g. table not found) from theSHOW ALL ROUTINE LOAD;result.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)