You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2017/09/14 07:07:09 UTC

[kudu-CR] Add mustache template for /table 

Will Berkeley has uploaded a new change for review.

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

Change subject: Add mustache template for /table
......................................................................

Add mustache template for /table

As a run-up to improving the web ui experience when there are multiple
masters, this patch switches the /table page to be based on a mustache
template.

Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/webui_util.cc
M src/kudu/server/webui_util.h
A www/table.mustache
M www/tables.mustache
6 files changed, 314 insertions(+), 181 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/8067/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8067
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add mustache template for /table

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8067 )

Change subject: Add mustache template for /table
......................................................................


Patch Set 10: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-Change-Number: 8067
Gerrit-PatchSet: 10
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Sep 2017 18:40:47 +0000
Gerrit-HasComments: No

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: Add mustache template for /table
......................................................................

Add mustache template for /table

As a run-up to improving the web ui experience when there are multiple
masters, this patch switches the /table page to be based on a mustache
template.

Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/webui_util.cc
M src/kudu/server/webui_util.h
A www/table.mustache
M www/tables.mustache
6 files changed, 315 insertions(+), 182 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/8067/8
-- 
To view, visit http://gerrit.cloudera.org:8080/8067
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-Change-Number: 8067
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: Add mustache template for /table
......................................................................

Add mustache template for /table

As a run-up to improving the web ui experience when there are multiple
masters, this patch switches the /table page to be based on a mustache
template.

Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/webui_util.cc
M src/kudu/server/webui_util.h
A www/table.mustache
M www/tables.mustache
6 files changed, 314 insertions(+), 182 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/8067/5
-- 
To view, visit http://gerrit.cloudera.org:8080/8067
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: Add mustache template for /table
......................................................................

Add mustache template for /table

As a run-up to improving the web ui experience when there are multiple
masters, this patch switches the /table page to be based on a mustache
template.

Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/webui_util.cc
M src/kudu/server/webui_util.h
A www/table.mustache
M www/tables.mustache
6 files changed, 314 insertions(+), 182 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/8067/2
-- 
To view, visit http://gerrit.cloudera.org:8080/8067
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add mustache template for /table

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Add mustache template for /table
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8067/5/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

Line 209:     (*output)["error"] = Substitute("Master is not ready: ", l.first_failed_status().ToString());
The new Substitute calls in this function are missing a $0. Not captured by a unit test?


http://gerrit.cloudera.org:8080/#/c/8067/5/src/kudu/master/master-path-handlers.h
File src/kudu/master/master-path-handlers.h:

PS5, Line 62: to
Duplicate


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: Add mustache template for /table
......................................................................

Add mustache template for /table

As a run-up to improving the web ui experience when there are multiple
masters, this patch switches the /table page to be based on a mustache
template.

Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/webui_util.cc
M src/kudu/server/webui_util.h
A www/table.mustache
M www/tables.mustache
6 files changed, 314 insertions(+), 182 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/8067/6
-- 
To view, visit http://gerrit.cloudera.org:8080/8067
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.

Change subject: Add mustache template for /table
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8067/5/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

Line 209:     (*output)["error"] = Substitute("Master is not ready: ", l.first_failed_status().ToString());
> The new Substitute calls in this function are missing a $0. Not captured by
Fixed. Same mistake as the KUDU-501 patch that sits on top of this one. Will be covered by the webserver stress test changes in that patch.


http://gerrit.cloudera.org:8080/#/c/8067/5/src/kudu/master/master-path-handlers.h
File src/kudu/master/master-path-handlers.h:

PS5, Line 62: to
> Duplicate
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: Add mustache template for /table
......................................................................

Add mustache template for /table

As a run-up to improving the web ui experience when there are multiple
masters, this patch switches the /table page to be based on a mustache
template.

Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/webui_util.cc
M src/kudu/server/webui_util.h
A www/table.mustache
M www/tables.mustache
6 files changed, 314 insertions(+), 181 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/8067/4
-- 
To view, visit http://gerrit.cloudera.org:8080/8067
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: Add mustache template for /table
......................................................................

Add mustache template for /table

As a run-up to improving the web ui experience when there are multiple
masters, this patch switches the /table page to be based on a mustache
template.

Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/webui_util.cc
M src/kudu/server/webui_util.h
A www/table.mustache
M www/tables.mustache
6 files changed, 314 insertions(+), 182 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/8067/3
-- 
To view, visit http://gerrit.cloudera.org:8080/8067
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.

Change subject: Add mustache template for /table
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8067/1/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

Line 336:     state_json["percentage"] = tablets.size() == 0 ? "0.0" : StringPrintf("%.2f", percentage);
> warning: the 'empty' method should be used to check for emptiness instead o
Done


Line 347: void MasterPathHandlers::HandleMasters(const Webserver::WebRequest& req,
> warning: parameter 'req' is unused [misc-unused-parameters]
Done


Line 632:   } else {
> warning: do not use 'else' after 'return' [readability-else-after-return]
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/8067 )

Change subject: Add mustache template for /table
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8067/8/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

http://gerrit.cloudera.org:8080/#/c/8067/8/src/kudu/master/master-path-handlers.cc@240
PS8, Line 240:       (*output)["state_msg"] = l.data().pb.state_msg();
> er, I just mean that the local variable 'state_msg' is already set to l.dat
Ah, I see :). Done.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-Change-Number: 8067
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Sep 2017 18:22:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add mustache template for /table

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Add mustache template for /table
......................................................................


Patch Set 7: Code-Review+1

I think Todd should take another look.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/8067 )

Change subject: Add mustache template for /table
......................................................................


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8067/5/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

http://gerrit.cloudera.org:8080/#/c/8067/5/src/kudu/master/master-path-handlers.cc@209
PS5, Line 209:     (*output)["error"] = Substitute("Master is not ready: ", l.first_failed_status().ToString());
> You missed a couple other Substitute() calls below.
Done


http://gerrit.cloudera.org:8080/#/c/8067/8/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

http://gerrit.cloudera.org:8080/#/c/8067/8/src/kudu/master/master-path-handlers.cc@240
PS8, Line 240:     }
> should this just be 'state_msg'?
I don't understand. AFAICT state_msg() is never set in the current code. If state = "foo" and state_msg = "bar" then the old formatting would display "foobar". The new formatting would display "foo (bar)", so the change here is future-proofing.


http://gerrit.cloudera.org:8080/#/c/8067/8/src/kudu/master/master-path-handlers.cc@280
PS8, Line 280: = link_pair.sec
> nit: you could std::move this and same on the next line, rght?
Done


http://gerrit.cloudera.org:8080/#/c/8067/8/src/kudu/master/master-path-handlers.cc@603
PS8, Line 603: }
> worth std::moveing text, target?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-Change-Number: 8067
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Sep 2017 17:59:29 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add mustache template for /table

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Add mustache template for /table
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8067/5/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

Line 209:     (*output)["error"] = Substitute("Master is not ready: $0", l.first_failed_status().ToString());
> Fixed. Same mistake as the KUDU-501 patch that sits on top of this one. Wil
You missed a couple other Substitute() calls below.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8067 )

Change subject: Add mustache template for /table
......................................................................

Add mustache template for /table

As a run-up to improving the web ui experience when there are multiple
masters, this patch switches the /table page to be based on a mustache
template.

Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Reviewed-on: http://gerrit.cloudera.org:8080/8067
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/webui_util.cc
M src/kudu/server/webui_util.h
A www/table.mustache
M www/tables.mustache
6 files changed, 315 insertions(+), 182 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-Change-Number: 8067
Gerrit-PatchSet: 11
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add mustache template for /table

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8067 )

Change subject: Add mustache template for /table
......................................................................


Patch Set 8: Code-Review+1

(3 comments)

A couple nits but generally LGTM (looked in detail at the escaping in particular)

http://gerrit.cloudera.org:8080/#/c/8067/8/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

http://gerrit.cloudera.org:8080/#/c/8067/8/src/kudu/master/master-path-handlers.cc@240
PS8, Line 240:       (*output)["state_msg"] = l.data().pb.state_msg();
should this just be 'state_msg'?


http://gerrit.cloudera.org:8080/#/c/8067/8/src/kudu/master/master-path-handlers.cc@280
PS8, Line 280: link_pair.first
nit: you could std::move this and same on the next line, rght?


http://gerrit.cloudera.org:8080/#/c/8067/8/src/kudu/master/master-path-handlers.cc@603
PS8, Line 603:   return std::make_pair(text, target);
worth std::moveing text, target?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-Change-Number: 8067
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Sep 2017 17:38:03 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: Add mustache template for /table
......................................................................

Add mustache template for /table

As a run-up to improving the web ui experience when there are multiple
masters, this patch switches the /table page to be based on a mustache
template.

Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/webui_util.cc
M src/kudu/server/webui_util.h
A www/table.mustache
M www/tables.mustache
6 files changed, 314 insertions(+), 182 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/8067/7
-- 
To view, visit http://gerrit.cloudera.org:8080/8067
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add mustache template for /table

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8067 )

Change subject: Add mustache template for /table
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8067/8/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

http://gerrit.cloudera.org:8080/#/c/8067/8/src/kudu/master/master-path-handlers.cc@240
PS8, Line 240:       (*output)["state_msg"] = l.data().pb.state_msg();
> I don't understand. AFAICT state_msg() is never set in the current code. If
er, I just mean that the local variable 'state_msg' is already set to l.data().pb.state_msg() so you can just use that local variable name rather than calling l.data().pb.state_msg() again?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-Change-Number: 8067
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Sep 2017 18:16:39 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: Add mustache template for /table
......................................................................

Add mustache template for /table

As a run-up to improving the web ui experience when there are multiple
masters, this patch switches the /table page to be based on a mustache
template.

Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/webui_util.cc
M src/kudu/server/webui_util.h
A www/table.mustache
M www/tables.mustache
6 files changed, 315 insertions(+), 182 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/8067/9
-- 
To view, visit http://gerrit.cloudera.org:8080/8067
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-Change-Number: 8067
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: Add mustache template for /table
......................................................................

Add mustache template for /table

As a run-up to improving the web ui experience when there are multiple
masters, this patch switches the /table page to be based on a mustache
template.

Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/webui_util.cc
M src/kudu/server/webui_util.h
A www/table.mustache
M www/tables.mustache
6 files changed, 315 insertions(+), 182 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/8067/10
-- 
To view, visit http://gerrit.cloudera.org:8080/8067
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-Change-Number: 8067
Gerrit-PatchSet: 10
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add mustache template for /table

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8067 )

Change subject: Add mustache template for /table
......................................................................


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8067/11/src/kudu/master/master-path-handlers.h
File src/kudu/master/master-path-handlers.h:

http://gerrit.cloudera.org:8080/#/c/8067/11/src/kudu/master/master-path-handlers.h@62
PS11, Line 62: to
Looks like this duplicate 'to' snuck back in.


http://gerrit.cloudera.org:8080/#/c/8067/11/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

http://gerrit.cloudera.org:8080/#/c/8067/11/src/kudu/master/master-path-handlers.cc@210
PS11, Line 210:     (*output)["error"] = Substitute("Master is not ready: ", l.first_failed_status().ToString());
And the Substitute() calls lost their $0?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-Change-Number: 8067
Gerrit-PatchSet: 11
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 22 Sep 2017 17:01:31 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add mustache template for /table

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.

Change subject: Add mustache template for /table
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8067/1/src/kudu/master/master-path-handlers.h
File src/kudu/master/master-path-handlers.h:

Line 63:   // or an empty string if no http address is available for the tserver.
> nit: indent 'or' here so it looks like part of the above bullet point
Done


http://gerrit.cloudera.org:8080/#/c/8067/1/src/kudu/server/webui_util.cc
File src/kudu/server/webui_util.cc:

Line 44:     col_json["name"] = col.name();
> can you remind me how escaping works with the mustache template stuff? we d
Double braces vs 3 braces is escaped vs. unescaped, respectively.
{{my_var}} -> escaped

{{{your_var}}} -> not escaped


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add mustache template for /table

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Add mustache template for /table
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8067/1/src/kudu/master/master-path-handlers.h
File src/kudu/master/master-path-handlers.h:

Line 63:   // or an empty string if no http address is available for the tserver.
nit: indent 'or' here so it looks like part of the above bullet point


http://gerrit.cloudera.org:8080/#/c/8067/1/src/kudu/server/webui_util.cc
File src/kudu/server/webui_util.cc:

Line 44:     col_json["name"] = col.name();
can you remind me how escaping works with the mustache template stuff? we do have to worry about it or are all substitutions always escaped when substituted into the template?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes