[Impala-ASF-CR] IMPALA-11823: Add more items to Impala web UI queries page

["Quanlong Huang (Code Review)" <ge...@cloudera.org>]

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 <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Thu, 02 Feb 2023 12:06:29 +0000
Gerrit-HasComments: Yes