github-actions[bot] commented on code in PR #64923:
URL: https://github.com/apache/doris/pull/64923#discussion_r3523305570
##########
regression-test/suites/query_profile/adaptive_pipeline_task_serial_read_on_limit.groovy:
##########
@@ -106,20 +102,25 @@ suite('adaptive_pipeline_task_serial_read_on_limit') {
def verifyQueryProfile = { stmt, serialReadOnLimit ->
sql stmt
String profileId = sql("select last_query_id()")[0][0].toString()
+ logger.info("Captured last_query_id() for statement [${stmt}]:
${profileId}")
assertTrue(verifyProfileContent(context, profileId, stmt,
serialReadOnLimit))
}
- verifyQueryProfile("select * from
adaptive_pipeline_task_serial_read_on_limit limit 10;", true)
- verifyQueryProfile("select * from
adaptive_pipeline_task_serial_read_on_limit limit 10000;", true)
+ // Temporarily skip these profile-detail assertions. The target queries
are very short, and after
+ // they finish the profile detail API may no longer have a coordinator and
can return
+ // "ID <query_id> does not exist" even though last_query_id() matches a
successful audit log entry.
+ // Keep the assertions here so they can be restored once profile download
by query id is stable.
+ // verifyQueryProfile("select * from
adaptive_pipeline_task_serial_read_on_limit limit 10;", true)
Review Comment:
This leaves the suite effectively setup-only: `verifyQueryProfile` is still
defined above, but every call that checks `MaxScanConcurrency` is commented
out, including the positive limit cases, the threshold/predicate cases, and the
`enable_adaptive_pipeline_task_serial_read_on_limit=false` case. The BE branch
still depends on the session variable, `tnode.limit`, and conjuncts, so without
at least one executable end-to-end assertion a regression in FE propagation or
the final profile value would pass this suite. Please restore the assertions
through a stable profile lookup path instead of disabling all of them.
##########
regression-test/suites/query_profile/s3_load_profile_test.groovy:
##########
@@ -30,12 +30,13 @@ def getProfile = { masterHTTPAddr, id ->
// ref
https://github.com/apache/doris/blob/3525a03815814f66ec78aa2ad6bbd9225b0e7a6b/regression-test/suites/load_p0/broker_load/test_s3_load.groovy
suite('s3_load_profile_test') {
- sql "set enable_profile=true;"
- sql "set profile_level=2;"
- def s3Endpoint = getS3Endpoint()
- def s3Region = getS3Region()
- sql "drop table if exists s3_load_profile_test_dup_tbl_basic;"
- sql """
+ setFeConfigTemporary(["profile_waiting_time_for_spill_seconds": 60]) {
Review Comment:
This suite now changes `profile_waiting_time_for_spill_seconds` on all FEs,
but it still runs in the normal concurrent suite pool. `setFeConfigTemporary`
uses `ADMIN SET ALL FRONTENDS CONFIG` and only restores the old value in
`finally`, so another normal suite running at the same time can observe the
60-second profile spill wait. `scanner_profile` was moved to `nonConcurrent`
for the same temporary FE config; please isolate this suite as well or avoid
the global config change.
##########
regression-test/framework/src/main/groovy/org/apache/doris/regression/action/ProfileAction.groovy:
##########
@@ -97,20 +121,67 @@ class ProfileAction implements SuiteAction {
def jsonSlurper2 = new JsonSlurper()
def profileText = jsonSlurper2.parseText(profileResp).data
- // Convert HTML entities and actual NBSPs to regular spaces,
- // retain line breaks, and then compress multiple consecutive
spaces into a single space
- profileText = profileText.replace(" ", " ")
- profileText = profileText.replace("</br>", "\n")
- // Replace the actual NBSP characters with regular spaces
- profileText = profileText.replace('\u00A0' as char, ' ' as char)
- // Compress two or more consecutive regular spaces into a single
space (without affecting line breaks)
- profileText = profileText.replaceAll(" {2,}", " ")
- result.text = profileText
+ result.text = normalizeProfileText(profileText)
}
profileCli.run()
return result.text
}
+ String getProfile(String profileId, List<String> requiredContents, long
timeoutMs = DEFAULT_PROFILE_WAIT_TIMEOUT_MS,
+ long intervalMs = DEFAULT_PROFILE_WAIT_INTERVAL_MS) {
+ return waitProfile({ getProfile(profileId) }, requiredContents,
"Profile ${profileId}", timeoutMs, intervalMs)
+ }
+
+ String waitProfile(Closure<String> profileFetcher, List<String>
requiredContents = [],
+ String profileDescription = "Profile", long timeoutMs =
DEFAULT_PROFILE_WAIT_TIMEOUT_MS,
+ long intervalMs = DEFAULT_PROFILE_WAIT_INTERVAL_MS) {
+ String profileText = ""
+ long deadline = System.currentTimeMillis() + timeoutMs
+ while (System.currentTimeMillis() <= deadline) {
+ String currentProfileText = profileFetcher.call()
+ if (currentProfileText != null && !currentProfileText.isEmpty()) {
+ profileText = currentProfileText
+ }
+ if (isProfileReady(currentProfileText, requiredContents)) {
+ return currentProfileText
+ }
+ log.info("{} is not ready, required contents: {}",
profileDescription, requiredContents)
+ Thread.sleep(intervalMs)
+ }
+ return profileText
Review Comment:
This timeout path still bypasses the completion check. `isProfileReady()`
now requires `Profile Completion State: COMPLETE`, but when the deadline
expires this method returns the last non-empty profile even if readiness never
succeeded, and `getProfileBySql()` has the same found-but-not-ready
fallthrough. The changed callers then assert only counters like `TaskCpuTime`,
`MaxScanConcurrency`, `NumScanners`, or `RowsProduced`, so a `COLLECTING`
profile that already contains those strings can still pass. Please fail loudly,
or otherwise return a readiness result, when the wait expires without
`COMPLETE`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
