Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19417 )
Change subject: IMPALA-11823: Add more items to Impala web UI queries page ...................................................................... Patch Set 2: (8 comments) Thanks for contributing to Impala! The new pages look good to me. I still need some time to review the correctness of new metrics. Post some comments first. http://gerrit.cloudera.org:8080/#/c/19417/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19417/2//COMMIT_MSG@10 PS2, Line 10: can visually see nit: replace with "show" http://gerrit.cloudera.org:8080/#/c/19417/2//COMMIT_MSG@13 PS2, Line 13: nit: remove one space here http://gerrit.cloudera.org:8080/#/c/19417/2/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/19417/2/be/src/scheduling/admission-controller.cc@1297 PS2, Line 1297: int64_t& wait_start_time_ms, int64_t& wait_end_time_ms, These are output parameters. In our code style, we try to use pointers instead of references for output parameters. Could you also reorder the parameters so input parameters are in front of output parameters? i.e. Status AdmissionController::WaitOnQueued(const UniqueIdPB& query_id, int64_t timeout_ms, unique_ptr<QuerySchedulePB>* schedule_result, int64_t* wait_start_time_ms, int64_t* wait_end_time_ms, bool* wait_timed_out) Impala C++ Style Guide: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536 http://gerrit.cloudera.org:8080/#/c/19417/2/be/src/scheduling/admission-controller.cc@1300 PS2, Line 1300: Should we initialize 'wait_start_time_ms' and 'wait_end_time_ms' at the begining? Not sure if their values will be undefined in some quick return paths. http://gerrit.cloudera.org:8080/#/c/19417/2/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/19417/2/be/src/service/client-request-state.h@657 PS2, Line 657: Are nit: remove "Are" http://gerrit.cloudera.org:8080/#/c/19417/2/common/protobuf/admission_control_service.proto File common/protobuf/admission_control_service.proto: http://gerrit.cloudera.org:8080/#/c/19417/2/common/protobuf/admission_control_service.proto@193 PS2, Line 193: microseconds milliseconds? 1s = 1000 milliseconds. 1 milliseconds = 1000 milliseconds http://gerrit.cloudera.org:8080/#/c/19417/2/www/queries.tmpl File www/queries.tmpl: http://gerrit.cloudera.org:8080/#/c/19417/2/www/queries.tmpl@31 PS2, Line 31: <th>Details</th> : <th>Action</th> Not sure if moving "Details" and "Action" to the front will break any apps. Is there any motivations for moving? http://gerrit.cloudera.org:8080/#/c/19417/2/www/query_detail_tabs.tmpl File www/query_detail_tabs.tmpl: http://gerrit.cloudera.org:8080/#/c/19417/2/www/query_detail_tabs.tmpl@99 PS2, Line 99: record.rows[1].cells[4].textContent = json['record_json']['duration']; It's hard to maintain the cell indexes if we want to reorder some in the future. Can we define some constants and use them instead? E.g. HEADER_ROW = 0; DATA_ROW = 1; DURATION_INDEX = 4; QUEUED_DURATION_INDEX = 5; ... ROWS_FETCHED_INDEX = 11; and use them like record.rows[DATA_ROW].cells[DURATION_INDEX].textContent = json['record_json']['duration']; These constances can also be used in refresh_record(). -- To view, visit http://gerrit.cloudera.org:8080/19417 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I19c75461a6405025fa433ae84d2c94d013fcaacb Gerrit-Change-Number: 19417 Gerrit-PatchSet: 2 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Thu, 02 Feb 2023 12:06:29 +0000 Gerrit-HasComments: Yes
