Lars Volker 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) Thanks for the comments, please see PS 10. 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 Done 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 use Done 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 : }; I moved it to the function. However, in C++11 static variables in functions are initialized in a thread-safe way and thus this comes at a runtime cost: > If multiple threads attempt to initialize the same static local variable > concurrently, the initialization occurs exactly once (similar behavior can be > obtained for arbitrary functions with std::call_once). > > Note: usual implementations of this feature use variants of the > double-checked locking pattern, which reduces runtime overhead for > already-initialized local statics to a single non-atomic boolean comparison. http://en.cppreference.com/w/cpp/language/storage_duration 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() Done 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 Done 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() ? Done -- 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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Comment-Date: Fri, 22 Dec 2017 11:44:10 +0000 Gerrit-HasComments: Yes
