[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: KUDU-2101 Include a table summary at the bottom
..


KUDU-2101 Include a table summary at the bottom

This add a table summary to the bottom of ksck. For each table, the
table contains a status and information about the total number of
tablets and the number of healthy, under-replicated, and unhealthy
tablets. A tablet with consensus mismatch is counted as unhealthy, to
make the table easier for less experienced users to understand.

See ksck-test for sample output.

Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Reviewed-on: http://gerrit.cloudera.org:8080/7707
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans 
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
3 files changed, 152 insertions(+), 34 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-2101 Include a table summary at the bottom
..


Patch Set 4: Code-Review+2

> Yes, a machine-readable output format would be nice. Howabout I
 > follow up with a patch for that?

 > I think a flag for machine-parseable (eg json) output would be nice
 > if we have a need to parse it. Otherwise what's the purpose of the
 > no-summary flag?

no-sommary flag in case someone relies on the current output format.

 > Yes, a machine-readable output format would be nice. Howabout I
 > follow up with a patch for that?

Sounds good!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: KUDU-2101 Include a table summary at the bottom
..


Patch Set 4:

Yes, a machine-readable output format would be nice. Howabout I follow up with 
a patch for that?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-2101 Include a table summary at the bottom
..


Patch Set 4:

I think a flag for machine-parseable (eg json) output would be nice if we have 
a need to parse it. Otherwise what's the purpose of the no-summary flag?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-2101 Include a table summary at the bottom
..


Patch Set 4:

Wondering... should we had a flag to disable the summary at the end? Maybe a 
way to have a less verbose output. Or maybe the reverse, have a flag to not 
show all the details and only have the summary?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-2101 Include a table summary at the bottom
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: KUDU-2101 Include a table summary at the bottom
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7707/3/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

PS3, Line 235:  == 
> On second thought, I think I might have misled you on this, maybe it should
Whoops.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2101 Include a table summary at the bottom
..

KUDU-2101 Include a table summary at the bottom

This add a table summary to the bottom of ksck. For each table, the
table contains a status and information about the total number of
tablets and the number of healthy, under-replicated, and unhealthy
tablets. A tablet with consensus mismatch is counted as unhealthy, to
make the table easier for less experienced users to understand.

See ksck-test for sample output.

Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
3 files changed, 152 insertions(+), 34 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: KUDU-2101 Include a table summary at the bottom
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7707/2/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

PS2, Line 235:   if (left.TableStatus() == CheckResult::OK && 
right.TableStatus() == CheckResult::OK) {
 : return left.name < right.name;
 :   }
 :   if (left.TableStatus() == CheckResult::OK && 
right.TableStatus() != CheckResult::OK) {
 : return true;
 :   }
 :   if (left.TableStatus() != CheckResult::OK && 
right.TableStatus() == CheckResult::OK) {
 : return false;
 :   }
 :   if (left.TableStatus() != CheckResult::OK && 
right.TableStatus() != CheckResult::OK) {
 : return left.name < right.name;
 :   }
> slightly simpler option:
Done


PS2, Line 249: col_names
> do you need this variable? I would guess you could just pass the brace list
No, don't need it, but I thought it was a bit more readable. Will change.


PS2, Line 265: row
> same - need the var?
Ditto


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2101 Include a table summary at the bottom
..

KUDU-2101 Include a table summary at the bottom

This add a table summary to the bottom of ksck. For each table, the
table contains a status and information about the total number of
tablets and the number of healthy, under-replicated, and unhealthy
tablets. A tablet with consensus mismatch is counted as unhealthy, to
make the table easier for less experienced users to understand.

See ksck-test for sample output.

Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
3 files changed, 152 insertions(+), 34 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-2101 Include a table summary at the bottom
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7707/2/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

PS2, Line 235:   if (left.TableStatus() == CheckResult::OK && 
right.TableStatus() == CheckResult::OK) {
 : return left.name < right.name;
 :   }
 :   if (left.TableStatus() == CheckResult::OK && 
right.TableStatus() != CheckResult::OK) {
 : return true;
 :   }
 :   if (left.TableStatus() != CheckResult::OK && 
right.TableStatus() == CheckResult::OK) {
 : return false;
 :   }
 :   if (left.TableStatus() != CheckResult::OK && 
right.TableStatus() != CheckResult::OK) {
 : return left.name < right.name;
 :   }
slightly simpler option:

  return std::make_pair(left.TableStatus() == CheckResult::OK, left.name) <
 std::make_pair(right.TableStatus() == CheckResult::OK, right.name);

does that work?


PS2, Line 249: col_names
do you need this variable? I would guess you could just pass the brace list 
below


PS2, Line 265: row
same - need the var?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: KUDU-2101 Include a table summary at the bottom
..


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7707/1/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

PS1, Line 222: push_back
> emplace_back(std::move(ts))
Done


Line 231:   // The table summary is in alphabetical order by name.
> is this best or should we put the unhealthy tables at the bottom, so that w
Done


PS1, Line 239:   vector names;
 :   vector statuses;
 :   vector total_tablets;
 :   vector num_healthy;
 :   vector num_underreplicated;
 :   vector num_unhealthy;
> is using the 'AddRow' interface not fewer lines of code / easier to follow 
Done


PS1, Line 255: UNHEALTHY
> I think unavailable is better but the current unhealthy is better aligned w
Unavailable is probably better. An under-replicated tablet is available, but 
it's not healthy. I think it's straightforward without an explanation to a 
casual Hadoop admin but I can add some words if you disagree.


PS1, Line 255: UNHEALTHY
> I find "UNHEALTHY" to be a little confusing. Should we say "unavailable" or
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2101 Include a table summary at the bottom
..

KUDU-2101 Include a table summary at the bottom

This add a table summary to the bottom of ksck. For each table, the
table contains a status and information about the total number of
tablets and the number of healthy, under-replicated, and unhealthy
tablets. A tablet with consensus mismatch is counted as unhealthy, to
make the table easier for less experienced users to understand.

See ksck-test for sample output.

Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
3 files changed, 164 insertions(+), 34 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-2101 Include a table summary at the bottom
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7707/1/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

PS1, Line 255: UNHEALTHY
> I find "UNHEALTHY" to be a little confusing. Should we say "unavailable" or
I think unavailable is better but the current unhealthy is better aligned with 
OK state of being healthy. 
Maybe we just need to print out an explanation?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-2101 Include a table summary at the bottom
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7707/1/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

PS1, Line 222: push_back
emplace_back(std::move(ts))


Line 231:   // The table summary is in alphabetical order by name.
is this best or should we put the unhealthy tables at the bottom, so that when 
you have more than a page full of tables the bad ones stick out?


PS1, Line 239:   vector names;
 :   vector statuses;
 :   vector total_tablets;
 :   vector num_healthy;
 :   vector num_underreplicated;
 :   vector num_unhealthy;
is using the 'AddRow' interface not fewer lines of code / easier to follow here?


PS1, Line 255: UNHEALTHY
I find "UNHEALTHY" to be a little confusing. Should we say "unavailable" or 
something to be more precise? I was a little confused when I saw output that 
said "0 under-replicated but 1 unhealthy". Is under-replicated not unhealthy in 
some sense?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2101 Include a table summary at the bottom

[Will Berkeley (Code Review)]

Will Berkeley has uploaded a new change for review.

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

Change subject: KUDU-2101 Include a table summary at the bottom
..

KUDU-2101 Include a table summary at the bottom

This add a table summary to the bottom of ksck. For each table, the
table contains a status and information about the total number of
tablets and the number of healthy, under-replicated, and unhealthy
tablets. A tablet with consensus mismatch is counted as unhealthy, to
make the table easier for less experienced users to understand.

See ksck-test for sample output.

Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
3 files changed, 163 insertions(+), 34 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I1913352e3a1f91b4bb07e2f5001c8cc94d5155d4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley