-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix](cloud) Fix auto-start functionality when encountering TVF and external queries #59963
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
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
TPC-H: Total hot run time: 31165 ms |
TPC-DS: Total hot run time: 173793 ms |
ClickBench: Total hot run time: 26.95 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
deardeng
left a comment
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.
Code Review Summary
This PR addresses an important issue with auto-start functionality when encountering TVF and external queries. The refactoring improves code quality, but there are some critical issues that need to be addressed:
Critical Issues
-
Semantic Mismatch in Timeout Configuration (RewriteJob.java:45, SimpleJobScheduler.java:45)
Config.auto_start_wait_to_resume_timesrepresents retry count, not seconds- Using it directly as a timeout in seconds is semantically incorrect
- While it works with default value (300), it's confusing and error-prone
-
Warmup Flag Not Reset (CloudSystemInfoService.java:1619)
- The
warmupflag is set totruebut never reset - This could cause subsequent queries to be incorrectly treated as warmup queries
- Should use try-finally to ensure cleanup
- The
Positive Aspects
- Good code refactoring: extraction of helper methods improves readability
- Excellent log optimization to reduce spam
- Better use of ThreadLocalRandom for thread safety
- Improved error handling with parseClusterStatusOrNull
Minor Issues
- Field naming inconsistency in SummaryProfile (isWarmUp vs isWarmup methods)
Please address the critical issues before merging.
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/scheduler/SimpleJobScheduler.java
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/cloud/system/CloudSystemInfoService.java
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/cloud/system/CloudSystemInfoService.java
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/cloud/system/CloudSystemInfoService.java
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/common/profile/SummaryProfile.java
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/rewrite/RewriteJob.java
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/cloud/system/CloudSystemInfoService.java
Show resolved
Hide resolved
|
run buildall |
TPC-H: Total hot run time: 32960 ms |
ClickBench: Total hot run time: 28.32 s |
FE Regression Coverage ReportIncrement line coverage |
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
15be4cf to
e462c91
Compare
|
run buildall |
TPC-H: Total hot run time: 30771 ms |
ClickBench: Total hot run time: 28.41 s |
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)