impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6190/6246: Add instances tab and event sequence
Date Wed, 03 Jan 2018 17:42:46 GMT
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 <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-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Jan 2018 17:42:46 +0000
Gerrit-HasComments: Yes

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