github-actions[bot] commented on code in PR #61828: URL: https://github.com/apache/doris/pull/61828#discussion_r3417820280
########## regression-test/suites/query_profile/test_profile_collection_completed.groovy: ########## @@ -0,0 +1,94 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +/** + * Verifies that SummaryProfile.queryFinished() writes the + * "Is Profile Collection Completed: true" field into the profile text. + * + * This field is written after Coordinator.waitForFragmentsDone() returns, + * guaranteeing all BE pipeline task profiles are merged. It allows pollers + * to detect profile readiness without fixed sleeps or heuristics. + */ +suite("test_profile_collection_completed") { + + def getProfileText = { String queryId -> + def addr = context.config.feHttpAddress + def user = context.config.feHttpUser ?: "root" + def pass = context.config.feHttpPassword ?: "" + def encoding = Base64.getEncoder().encodeToString("${user}:${pass}".getBytes("UTF-8")) + def conn = new URL("http://${addr}/api/profile/text?query_id=${queryId}";).openConnection() + conn.setRequestMethod("GET") + conn.setRequestProperty("Authorization", "Basic ${encoding}") + conn.setConnectTimeout(5000) + conn.setReadTimeout(10000) + try { + return conn.getInputStream().getText() + } catch (Exception e) { + return "" + } + } + + // Poll until the completion marker appears, or timeout after 10 s. + def waitForCompletionMarker = { String queryId -> + def marker = "Is Profile Collection Completed: true" + def maxAttempts = 33 // 33 × 300 ms ≈ 10 s + for (int i = 0; i < maxAttempts; i++) { + Thread.sleep(300) + def text = getProfileText(queryId) + if (text.contains(marker)) { + return text + } + } + return getProfileText(queryId) + } + + sql "SET enable_profile = true" + sql "SET enable_sql_cache = false" + + // Use a multi-instance query so the fragment pipeline is exercised + // and a proper execution profile is written (simple SET statements are skipped). + sql "DROP TABLE IF EXISTS test_profile_complete_t" + sql """ + CREATE TABLE test_profile_complete_t ( + k1 INT NOT NULL, + v1 INT + ) ENGINE=OLAP + DUPLICATE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 4 + PROPERTIES ("replication_num" = "1") + """ + + sql "INSERT INTO test_profile_complete_t VALUES (1, 10), (2, 20), (3, 30)" + + sql "SELECT count(*) FROM test_profile_complete_t" + + def queryIdResult = sql "SELECT last_query_id()" + def queryId = queryIdResult[0][0].toString() + logger.info("query_id for completion marker test: ${queryId}") + + def profileText = waitForCompletionMarker(queryId) + + assertTrue( + profileText.contains("Is Profile Collection Completed: true"), + "Expected 'Is Profile Collection Completed: true' in profile for query ${queryId}. " + + "Profile length=${profileText.length()}, first 500 chars: ${profileText.take(500)}" + ) + + logger.info("'Is Profile Collection Completed: true' verified in profile for query ${queryId}") + + sql "DROP TABLE IF EXISTS test_profile_complete_t" Review Comment: Doris regression-test rules say to drop tables before a case, not after it, so the environment remains available for debugging failures. This suite already drops `test_profile_complete_t` before creating it; please remove the final drop. ########## fe/fe-core/src/main/java/org/apache/doris/common/profile/SummaryProfile.java: ########## @@ -508,6 +512,10 @@ public void update(Map<String, String> summaryInfo) { // This method is used to display the final data status when the overall query ends. // This can avoid recalculating some strings and so on every time during the update process. public void queryFinished() { + // Mark profile collection as complete. This is written last, after all BE fragment + // profiles have been merged (called post-waitForFragmentsDone). Pollers can wait for + // this field to avoid reading a partial profile. Review Comment: This does not actually prove profile collection is complete before pollers can observe the marker. `Profile.updateSummary(..., true)` calls `summaryProfile.queryFinished()` when the statement/coordinator is finished, while `Profile` explicitly documents that `isQueryFinished` does not mean async BE profile reports have finished and that collection completion is tracked by `ExecutionProfile.isCompleted()`. Running profiles can already be read from `ProfileManager`, and each `addInfoString()` is separately locked, so a client polling profile text can see this new field before late BE fragment reports are merged. This method also still appends `SPLITS_ASSIGNMENT_WEIGHT` after the marker when `show_split_profile_info` is enabled. Please set the marker from the actual execution-profile-completed condition, or change the contract/name to statement-finished and keep it from being used as a collection-complete signal. -- 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]
