Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-2198: Differentiate queries in exceptional states in web 
UI
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/2625/1/be/src/service/impala-server-callbacks.cc
File be/src/service/impala-server-callbacks.cc:

Line 303:       num_waiting_queries++;
> single line, prefer pre-increment
Done


http://gerrit.cloudera.org:8080/#/c/2625/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 1599: 
> blank line can be removed
Done


http://gerrit.cloudera.org:8080/#/c/2625/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 610: eos
> prefer "all_rows_returned"
Done


http://gerrit.cloudera.org:8080/#/c/2625/1/www/queries.tmpl
File www/queries.tmpl:

Line 60:   {{num_waiting_queries}} waiting on client
> this makes it sound like the queries are waiting on *some* client input (wh
I'm open to suggestions of the clearest thing to put here. It seems weird to 
say something about them being 'finished', since this is similar to the 
'completed queries' section below. For now I'll change it to 'waiting to be 
closed'. The difficulty of making this clear was the motivation behind the 
tooltip.


Line 61:   <sup><a href='#' data-toggle="tooltip" 
title="{{waiting-tooltip}}">[?]</a></sup>
> Better just to inline the tooltip text here.
I hadn't done this because I can't figure out any way to wrap an html string in 
the middle (i.e. because its over the 90 character line limit) without having 
the line break show up in the string when it is displayed. Any idea how to do 
this?


Line 77:     <th>Action</th>
> It would be good to have a measure of the time a query has been waiting for
The closest thing we already have that I can find would be using the difference 
between current time and QueryExecState::last_active_time, though I don't think 
this is quite the right thing - for example, if a query has an exception 
because it timed out, last_active_time isn't updated to the time when the 
exception happened, so it would start in the 'waiting' section with a 'waiting 
time' of about '--idle-query-timeout'

I'm happy to add something that tracks this, if we think its needed.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47d0b642ecb573fefbbf337b8c8f2c479b0d49b2
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Thomas Tauber-Marshall <[email protected]>
Gerrit-Reviewer: Henry Robinson <[email protected]>
Gerrit-Reviewer: Matthew Jacobs <[email protected]>
Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]>
Gerrit-HasComments: Yes

Reply via email to