Tim Armstrong 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 10: (8 comments) Did an initial pass. I need to think a bit more about test coverage and the javascript but I thought it was worth pushing out these comments. http://gerrit.cloudera.org:8080/#/c/8758/10/be/src/runtime/fragment-instance-state.h File be/src/runtime/fragment-instance-state.h: http://gerrit.cloudera.org:8080/#/c/8758/10/be/src/runtime/fragment-instance-state.h@174 PS10, Line 174: nit: vertical whitespace doesn't seem to improve readability here http://gerrit.cloudera.org:8080/#/c/8758/10/be/src/runtime/fragment-instance-state.h@197 PS10, Line 197: Updated Maybe "Only updated" to be clearer. http://gerrit.cloudera.org:8080/#/c/8758/10/be/src/runtime/fragment-instance-state.h@282 PS10, Line 282: void UpdateState(const StateEvent event); It seems worth documenting the state machine in the header, maybe in the enum definition (or at least mentioning that the body of UpdateState() should be consulted to determine valid transitions) http://gerrit.cloudera.org:8080/#/c/8758/10/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/8758/10/be/src/runtime/fragment-instance-state.cc@73 PS10, Line 73: nit: unnecessary vertical whitespace. http://gerrit.cloudera.org:8080/#/c/8758/10/be/src/runtime/fragment-instance-state.cc@137 PS10, Line 137: UpdateState(StateEvent::PREPARE_START); Maybe remove some of the vertical whitespace? It doesn't really help in understanding the structure when stretches of code are double spaced http://gerrit.cloudera.org:8080/#/c/8758/10/be/src/runtime/fragment-instance-state.cc@222 PS10, Line 222: RETURN_IF_ERROR(codegen->FinalizeModule()); We need to undo this change, we deliberately had the the heavyweight codegen in Open() since it's heavyweight and Prepare() has limitations about cancellations. http://gerrit.cloudera.org:8080/#/c/8758/10/be/src/runtime/fragment-instance-state.cc@389 PS10, Line 389: case StateEvent::PREPARE_START: nit: we generally indent the case labels one level from the switch statement. We typically also don't have whitespace after "break" but I don't feel strongly about that. http://gerrit.cloudera.org:8080/#/c/8758/10/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: http://gerrit.cloudera.org:8080/#/c/8758/10/be/src/util/runtime-profile-counters.h@325 PS10, Line 325: SortEvents(); I'm confused about the invariants here. Why is calling SortEvents() necessary if 'events' is always in ascending order? -- 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: 10 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-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 03 Jan 2018 17:42:46 +0000 Gerrit-HasComments: Yes
