impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6190/6246: Add instances tab and event sequence
Date Fri, 22 Dec 2017 00:07:16 GMT
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8758 )

Change subject: IMPALA-6190/6246: Add instances tab and event sequence
......................................................................


Patch Set 9:

(6 comments)

LGTM. Some minor comments.

http://gerrit.cloudera.org:8080/#/c/8758/9/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:

http://gerrit.cloudera.org:8080/#/c/8758/9/be/src/runtime/fragment-instance-state.h@111
PS9, Line 111: TFInstanceState::type
const TFInstanceState::type


http://gerrit.cloudera.org:8080/#/c/8758/9/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:

http://gerrit.cloudera.org:8080/#/c/8758/9/be/src/runtime/fragment-instance-state.cc@63
PS9, Line 63: /// Events that are tracked in a separate timeline for each fragment instance.
            : static const string INSTANCE_EVENT_TIMELINE_NAME =
            :     "Fragment Instance Lifecycle Event Timeline";
            : 
            : /// Indicates that the call to Prepare() has been completed.
            : static const string PREPARE_EVENT_NAME = "Prepare Finished";
            : 
            : /// Indicates that the call to Open() has been completed.
            : static const string OPEN_EVENT_NAME = "Open Finished";
            : 
            : /// Indicates that the first row batch has been returned from the instance's
tree through
            : /// GetNext().
            : static const string FIRST_BATCH_PRODUCED_EVENT_NAME = "First Batch Produced";
            : 
            : /// Indicates that the first row batch has been sent to the instance's sink.
            : static const string FIRST_BATCH_SENT_EVENT_NAME = "First Batch Sent";
            : 
            : /// Indicates that this instance's execution has finished and the final status
report is
            : /// about to be sent to the coordinator.
            : static const string EXEC_INTERNAL_EVENT_NAME = "ExecInternal Finished";
Please feel free to disagree but given each of these variables are only used once in MarkEvent()
below, the code seems more straightforward and less verbose if we just directly call MarkEvent()
with the given string:

e.g. MarkEvent("Open Finished");


http://gerrit.cloudera.org:8080/#/c/8758/9/be/src/runtime/fragment-instance-state.cc@84
PS9, Line 84: /// Labels to send to the debug webpages to display the current state to the
user.
            : static const string FINSTANCE_STATE_LABELS[] = {
            :     "Waiting for Exec",         // WAITING_FOR_EXEC
            :     "Waiting for Codegen",      // WAITING_FOR_CODEGEN
            :     "Waiting for Prepare",      // WAITING_FOR_PREPARE
            :     "Waiting for First Batch",  // WAITING_FOR_OPEN
            :     "Waiting for First Batch",  // WAITING_FOR_FIRST_BATCH
            :     "First batch produced",     // FIRST_BATCH_PRODUCED
            :     "Producing Data",           // PRODUCING_DATA
            :     "Last batch sent",          // END_OF_STREAM
            :     "Finished"                  // FINISHED
            : };
Can this be hidden inside GetFInstanceStateDescription() ?


http://gerrit.cloudera.org:8080/#/c/8758/9/be/src/runtime/fragment-instance-state.cc@546
PS9, Line 546: GetFInstanceStateDescription
A slightly shorter name to consider is: ExecStateToString()


http://gerrit.cloudera.org:8080/#/c/8758/9/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

http://gerrit.cloudera.org:8080/#/c/8758/9/common/thrift/ImpalaInternalService.thrift@607
PS9, Line 607: TFInstanceState
To avoid confusing with FragmentInstanceState in the BE code, do you think TFInstanceExecState
a better name ?


http://gerrit.cloudera.org:8080/#/c/8758/9/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

http://gerrit.cloudera.org:8080/#/c/8758/9/tests/webserver/test_web_pages.py@182
PS9, Line 182: test_query_details
test_backend_states() ?



-- 
To view, visit http://gerrit.cloudera.org:8080/8758
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I626456b6afa9101eeeeffd5cda10c4096d63d7f9
Gerrit-Change-Number: 8758
Gerrit-PatchSet: 9
Gerrit-Owner: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Dec 2017 00:07:16 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message