impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
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. ( )

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

Patch Set 10:


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.
File be/src/runtime/fragment-instance-state.h:
PS10, Line 174: 
nit: vertical whitespace doesn't seem to improve readability here
PS10, Line 197: Updated
Maybe "Only updated" to be clearer.
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
File be/src/runtime/
PS10, Line 73: 
nit: unnecessary vertical whitespace.
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
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.
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.
File be/src/util/runtime-profile-counters.h:
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
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Joe McDonnell <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Wed, 03 Jan 2018 17:42:46 +0000
Gerrit-HasComments: Yes

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