Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/23777 )
Change subject: IMPALA-14370: [Patch 1 of 2] - OpenTelemetry Query Tracing Skips Queries with Leading Comments ...................................................................... Patch Set 18: (9 comments) http://gerrit.cloudera.org:8080/#/c/23777/18//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23777/18//COMMIT_MSG@10 PS18, Line 10: rudimentry nit: "rudimentary" http://gerrit.cloudera.org:8080/#/c/23777/18//COMMIT_MSG@36 PS18, Line 36: cluste nit: "cluster" http://gerrit.cloudera.org:8080/#/c/23777/18/be/src/observe/otel.h File be/src/observe/otel.h: http://gerrit.cloudera.org:8080/#/c/23777/18/be/src/observe/otel.h@50 PS18, Line 50: * nit: try using reference instead of pointer for input parameters http://gerrit.cloudera.org:8080/#/c/23777/18/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/23777/18/be/src/service/client-request-state.cc@130 PS18, Line 130: &query_ctx_.client_request)) { nit: the original identation looks more clear for me if (FLAGS_otel_trace_enabled && should_otel_trace_query( query_ctx_.session.session_type, &query_ctx_.client_request)) { http://gerrit.cloudera.org:8080/#/c/23777/18/fe/src/main/java/org/apache/impala/service/FeSupport.java File fe/src/main/java/org/apache/impala/service/FeSupport.java: http://gerrit.cloudera.org:8080/#/c/23777/18/fe/src/main/java/org/apache/impala/service/FeSupport.java@180 PS18, Line 180: do_trace nit: we use camel case in Java code, i.e. "doTrace" http://gerrit.cloudera.org:8080/#/c/23777/18/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/23777/18/fe/src/main/java/org/apache/impala/service/Frontend.java@3014 PS18, Line 3014: Dermine nit: "Determine" http://gerrit.cloudera.org:8080/#/c/23777/18/fe/src/main/java/org/apache/impala/service/Frontend.java@3342 PS18, Line 3342: || parsedStmt.isInvalidateMetadata() Just curious, why we track IM commands but excluding REFRESH? http://gerrit.cloudera.org:8080/#/c/23777/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteParsedStatement.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteParsedStatement.java: http://gerrit.cloudera.org:8080/#/c/23777/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteParsedStatement.java@122 PS18, Line 122: public boolean isValuesStmt() { return false; } ValuesStmt is a subclass of QueryStmt: ValuesStmt -> UnionStmt -> SetOperationStmt -> QueryStmt. I'm not sure how calcite-planner handles values() statement but Is it OK to always return false here? http://gerrit.cloudera.org:8080/#/c/23777/18/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/23777/18/tests/common/impala_test_suite.py@1829 PS18, Line 1829: assert_log_contains assert_log_contains() already has a loop of retry and sleep (interval=1). Do we really need this wrapper? Or maybe add a period_s parameter to assert_log_contains() is enough? https://github.com/apache/impala/blob/20220fb9232b94d228383fe693a383d2c71a4733/tests/common/impala_test_suite.py#L1817 -- To view, visit http://gerrit.cloudera.org:8080/23777 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1425b32006f81586bf75c2e4045d23bab91e1611 Gerrit-Change-Number: 23777 Gerrit-PatchSet: 18 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Wed, 25 Mar 2026 05:56:41 +0000 Gerrit-HasComments: Yes
