[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 6: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


IMPALA-5811: Add 'backends' tab to query details pages

Add a 'backends' tab to query details pages which shows:

  * host
  * total number of fragment instances for that query on that backend
  * number of still-running fragment instances
  * if the backend is complete (i.e. all instances finished)
  * peak memory consumption
  * the time, in ms, since a status report was received at the
  * coordinator from that backend.

The table refreshes itself every second, controllable by a check-box. If
the query has completed, no information is displayed.

Testing: Add a new smoketest to test_web_pages.py.

Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Reviewed-on: http://gerrit.cloudera.org:8080/7711
Reviewed-by: Dan Hecht 
Tested-by: Impala Public Jenkins
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M tests/webserver/test_web_pages.py
A www/query_backends.tmpl
M www/query_detail_tabs.tmpl
9 files changed, 237 insertions(+), 34 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Dan Hecht: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 6:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1139/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 5:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/7711/5/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

Line 571:   value->AddMember("done", IsDone(), document->GetAllocator());
> 'done' is more of an internal concept now (and I think that's better), so n
I think it's useful, even as a derived value - users of this page want to see 
which backends are still working on a query (e.g. to see which is hung).


PS5, Line 580: last_heard_from
> that name doesn't sound like a relative time. maybe time_since_last_report 
Done


http://gerrit.cloudera.org:8080/#/c/7711/5/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:

Line 116:   void ToJson(rapidjson::Value* value, rapidjson::Document* doc);
> let's add a quick comment for that.
Done


Line 219:   int64_t last_report_time_ms_ = 0;
> specify what clock that is. is it unix millis or monotoic millis or somethi
Done


http://gerrit.cloudera.org:8080/#/c/7711/5/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

Line 190:   void BackendsToJson(rapidjson::Document* document);
> document
Done


http://gerrit.cloudera.org:8080/#/c/7711/5/be/src/service/impala-http-handler.h
File be/src/service/impala-http-handler.h:

Line 94:   void QueryBackendsHandler(
> document
Done - I haven't followed the other examples of providing an example json 
document as I think that's a little verbose, and tightly coupled to the 
implementation.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Henry Robinson (Code Review)]

Hello Bharath Vissapragada, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7711

to look at the new patch set (#6).

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..

IMPALA-5811: Add 'backends' tab to query details pages

Add a 'backends' tab to query details pages which shows:

  * host
  * total number of fragment instances for that query on that backend
  * number of still-running fragment instances
  * if the backend is complete (i.e. all instances finished)
  * peak memory consumption
  * the time, in ms, since a status report was received at the
  * coordinator from that backend.

The table refreshes itself every second, controllable by a check-box. If
the query has completed, no information is displayed.

Testing: Add a new smoketest to test_web_pages.py.

Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M tests/webserver/test_web_pages.py
A www/query_backends.tmpl
M www/query_detail_tabs.tmpl
9 files changed, 237 insertions(+), 34 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/7711/6
-- 
To view, visit http://gerrit.cloudera.org:8080/7711
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 5:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/7711/5/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

Line 571:   value->AddMember("done", IsDone(), document->GetAllocator());
'done' is more of an internal concept now (and I think that's better), so not 
sure we need to print it. It's derived from status and num_remaining_instances 
anyway. 

I'm not super opposed to having it though, if you feel it helps.


PS5, Line 580: last_heard_from
that name doesn't sound like a relative time. maybe time_since_last_report or 
time_since_last_report_ms?


http://gerrit.cloudera.org:8080/#/c/7711/5/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:

Line 116:   void ToJson(rapidjson::Value* value, rapidjson::Document* doc);
let's add a quick comment for that.


Line 219:   int64_t last_report_time_ms_ = 0;
specify what clock that is. is it unix millis or monotoic millis or something 
else?


http://gerrit.cloudera.org:8080/#/c/7711/5/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

Line 190:   void BackendsToJson(rapidjson::Document* document);
document


http://gerrit.cloudera.org:8080/#/c/7711/5/be/src/service/impala-http-handler.h
File be/src/service/impala-http-handler.h:

Line 94:   void QueryBackendsHandler(
document


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 5: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7711/4/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

Line 183:   assert len(response_json['backend_states']) == 3
> This check is only accurate on the 3-node minicluster right - we probably n
Good point! Changed the check to be > 0 - more robust to other test 
environments without having to skip it (and also won't be sensitive to changes 
in parallelism for these query plans).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Henry Robinson (Code Review)]

Hello Bharath Vissapragada, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7711

to look at the new patch set (#5).

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..

IMPALA-5811: Add 'backends' tab to query details pages

Add a 'backends' tab to query details pages which shows:

  * host
  * total number of fragment instances for that query on that backend
  * number of still-running fragment instances
  * if the backend is complete (i.e. all instances finished)
  * peak memory consumption
  * the time, in ms, since a status report was received at the
  * coordinator from that backend.

The table refreshes itself every second, controllable by a check-box. If
the query has completed, no information is displayed.

Testing: Add a new smoketest to test_web_pages.py.

Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M tests/webserver/test_web_pages.py
A www/query_backends.tmpl
M www/query_detail_tabs.tmpl
9 files changed, 231 insertions(+), 34 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/7711/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7711
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 4: Code-Review+1

(1 comment)

LGTM aside from one minor thing. Will give others a chance to look and +2

http://gerrit.cloudera.org:8080/#/c/7711/4/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

Line 183:   assert len(response_json['backend_states']) == 3
This check is only accurate on the 3-node minicluster right - we probably need 
to skip the test if it's a local fs or remote cluster.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 3:

(2 comments)

Added a test, thanks for the suggestion!

http://gerrit.cloudera.org:8080/#/c/7711/3/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

PS3, Line 557: IsDone
> after Thomas' change, the callers of this are responsible for taking the lo
Done


PS3, Line 566: last_report_time_ms_
> This function is inconsistent about whether it acquires 'lock_' when readin
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 3:

(1 comment)

Could we add a basic test to run a couple of queries (including a DDL statement 
if that caused a crash earlier) and ping the backends page? I don't want to 
hold this up but it would be nice to avoid a similar bug creeping in again.

http://gerrit.cloudera.org:8080/#/c/7711/3/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

PS3, Line 566: last_report_time_ms_
This function is inconsistent about whether it acquires 'lock_' when reading 
the fields - in some cases it goes through the public accessors (IsDone(), 
GetPeakConsumption(), etc) that acquire lock_ but in other case it is touching 
the fields directly.

It's not a big deal but it's a bit confusing.

It might be better to just grab 'lock_' in this function and get the fields 
directly.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7711/3/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

PS3, Line 557: IsDone
after Thomas' change, the callers of this are responsible for taking the lock.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7711/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

Line 556:   value->AddMember("num_instances", fragments_.size(), 
document->GetAllocator());
> I couldn't find a very easy way to plumb it through. Perhaps we could leave
hmm, I'm ok with it. May be add a TODO if you think its worth adding in the 
future.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7711/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

Line 556:   value->AddMember("num_instances", fragments_.size(), 
document->GetAllocator());
> I think we should include some form of total_ranges_complete_ in this page.
I couldn't find a very easy way to plumb it through. Perhaps we could leave 
that to a follow-on patch?


PS1, Line 569: status_
> use GetStatus()? It takes the BackendState::lock_
Done


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

PS1, Line 710: request_state
> UNLIKELY(request_state.get()...) ?
I think UNLIKELY() is best used when we have branches whose mispredictions are 
expensive. That's not really the case here.


http://gerrit.cloudera.org:8080/#/c/7711/1/www/query_backends.tmpl
File www/query_backends.tmpl:

Line 57: 
> Does it make sense to add a checkbox to show only backends with unfinished 
The table can be sorted so that unfinished backends can be sorted to the top.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Henry Robinson (Code Review)]

Henry Robinson has uploaded a new patch set (#3).

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..

IMPALA-5811: Add 'backends' tab to query details pages

Add a 'backends' tab to query details pages which shows:

  * host
  * total number of fragment instances for that query on that backend
  * number of still-running fragment instances
  * if the backend is complete (i.e. all instances finished)
  * peak memory consumption
  * the time, in ms, since a status report was received at the
  * coordinator from that backend.

The table refreshes itself every second, controllable by a check-box. If
the query has completed, no information is displayed.

Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
A www/query_backends.tmpl
M www/query_detail_tabs.tmpl
8 files changed, 172 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/7711/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7711
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7711/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

Line 556:   value->AddMember("num_instances", fragments_.size(), 
document->GetAllocator());
I think we should include some form of total_ranges_complete_ in this page. I'd 
prefer something like num_remaining_scan_ranges, if its easy to plumb that 
value (completed scan ranges is available directly in the backend state). I've 
seen many queries stuck in reading scan ranges (potentially due to some 
underlying FS problem) causing the whole pipeline to hang. So this metric might 
help such cases.


PS1, Line 569: status_
use GetStatus()? It takes the BackendState::lock_


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

PS1, Line 710: request_state
UNLIKELY(request_state.get()...) ?


http://gerrit.cloudera.org:8080/#/c/7711/1/www/query_backends.tmpl
File www/query_backends.tmpl:

Line 57: 
Does it make sense to add a checkbox to show only backends with unfinished 
fragments? Typically while debugging queries with tons of fragments we will be 
interested only in laggers.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 2:

That's deliberate - preserve the last seen set of states. I could change 
something to add an indicator if the coordinator has stopped responding, but 
that would be pretty tricky to do. Note that this behaviour is the same as 
other pages that simply stop updating when the query is done.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 2:

> Presumably the query was a DDL statement, so there was no
 > coordinator. Thanks for the report, hopefully fixed now.
 > 
 > Screenshot at 
 > https://gist.githubusercontent.com/henryr/1a50fc1ec7474353b84343815c9f867f/raw/f23d873c8d39c10dc00ffc4a1c30a9dbec3e0251/screen-shot.png

The segfault is gone and it works for me now. I noticed that after the query 
finishes the list is not updated anymore and looks like it's stuck: 
https://user-images.githubusercontent.com/151514/29438035-09de6f14-8369-11e7-9dba-83e3ef395baf.png

Is there an easy way around it?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Henry Robinson (Code Review)]

Henry Robinson has uploaded a new patch set (#2).

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..

IMPALA-5811: Add 'backends' tab to query details pages

Add a 'backends' tab to query details pages which shows:

  * host
  * total number of fragment instances for that query on that backend
  * number of still-running fragment instances
  * if the backend is complete (i.e. all instances finished)
  * peak memory consumption
  * the time, in ms, since a status report was received at the
  * coordinator from that backend.

The table refreshes itself every second, controllable by a check-box. If
the query has completed, no information is displayed.

Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
A www/query_backends.tmpl
M www/query_detail_tabs.tmpl
8 files changed, 171 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/7711/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7711
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 1:

Presumably the query was a DDL statement, so there was no coordinator. Thanks 
for the report, hopefully fixed now. 

Screenshot at 
https://gist.githubusercontent.com/henryr/1a50fc1ec7474353b84343815c9f867f/raw/f23d873c8d39c10dc00ffc4a1c30a9dbec3e0251/screen-shot.png

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 1:

Great supportability change. Mind adding a screenshot of the newly added tab?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..


Patch Set 1:

I tried this on my local machine while a query was running and hit a segfault 
in BackendsToJson:

C  [libpthread.so.0+0xa404]  pthread_mutex_lock+0x4
C  [impalad+0xe122fa]  
boost::lock_guard::lock_guard(boost::mutex&)+0x2a
C  [impalad+0x16dc68f]  
impala::Coordinator::BackendsToJson(rapidjson::GenericDocument*)+0x37
C  [impalad+0x1160188]  
impala::ImpalaHttpHandler::QueryBackendsHandler(std::map > const&, 
rapidjson::GenericDocument*)+0x28c

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5811: Add 'backends' tab to query details pages

[Henry Robinson (Code Review)]

Henry Robinson has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7711

Change subject: IMPALA-5811: Add 'backends' tab to query details pages
..

IMPALA-5811: Add 'backends' tab to query details pages

Add a 'backends' tab to query details pages which shows:

  * host
  * total number of fragment instances for that query on that backend
  * number of still-running fragment instances
  * if the backend is complete (i.e. all instances finished)
  * peak memory consumption
  * the time, in ms, since a status report was received at the
  * coordinator from that backend.

The table refreshes itself every second, controllable by a check-box. If
the query has completed, no information is displayed.

Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
A www/query_backends.tmpl
M www/query_detail_tabs.tmpl
8 files changed, 171 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/7711/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7711
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib5b3b0fb8f4188da56da593199f41ce6fab99767
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson