[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Todd Lipcon (Code Review)
Todd Lipcon has submitted this change and it was merged.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool

This change ports fs_dump actions under 'kudu local_replica '.
Additionally this has following re-organizations:
- moved dump_cfile action under 'kudu fs dump cfile'.
- kudu-fs_list tool has been removed altogether,
  but some of the functionalities are retained under
  'local_replica' and 'fs dump' sub-actions.
- fs_tool library is stripped off, and all those
  routines are in respective action files.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Reviewed-on: http://gerrit.cloudera.org:8080/4305
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M docs/release_notes.adoc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_tablet.cc
11 files changed, 766 insertions(+), 823 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Todd Lipcon (Code Review)
Todd Lipcon has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 14: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Todd Lipcon (Code Review)
Todd Lipcon has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 8:

Just rebased and put this in a series with Dinesh's patch. Will commit if it 
passes

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Todd Lipcon (Code Review)
Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..

tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool

This change ports fs_dump actions under 'kudu local_replica '.
Additionally this has following re-organizations:
- moved dump_cfile action under 'kudu fs dump cfile'.
- kudu-fs_list tool has been removed altogether,
  but some of the functionalities are retained under
  'local_replica' and 'fs dump' sub-actions.
- fs_tool library is stripped off, and all those
  routines are in respective action files.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_tablet.cc
11 files changed, 766 insertions(+), 823 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/14
-- 
To view, visit http://gerrit.cloudera.org:8080/4305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 14:

Build Started http://104.196.14.100/job/kudu-gerrit/3391/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 13: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Dinesh Bhat (Code Review)
Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 13:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

PS9, Line 679: 
> Well, I think "tablet_id" does make more sense than something like "replica
cool, I adjusted few anomalies I found about 80 chars at few places. It wasn't 
related to this thread/comment, just some observation around these lines I 
think.


http://gerrit.cloudera.org:8080/#/c/4305/12/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

PS12, Line 676: _config", 
> Nit: should be "local Kudu replica" to be consistent with the description o
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 13:

Build Started http://104.196.14.100/job/kudu-gerrit/3378/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Dinesh Bhat (Code Review)
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..

tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool

This change ports fs_dump actions under 'kudu local_replica '.
Additionally this has following re-organizations:
- moved dump_cfile action under 'kudu fs dump cfile'.
- kudu-fs_list tool has been removed altogether,
  but some of the functionalities are retained under
  'local_replica' and 'fs dump' sub-actions.
- fs_tool library is stripped off, and all those
  routines are in respective action files.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_tablet.cc
11 files changed, 766 insertions(+), 823 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/13
-- 
To view, visit http://gerrit.cloudera.org:8080/4305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 9:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

PS9, Line 679: tablet
> yikes !! sorry, actually it's kinda odd that all our required params are 't
Well, I think "tablet_id" does make more sense than something like 
"replica_id", because the identifier is unique to the tablet rather than to the 
replica. That is, all replicas of this tablet share this same identifier, so it 
is not a useful way of identifying this replica.

And yes, we should still wrap at 80 chars; I'm not sure how this thread 
suggests otherwise?


http://gerrit.cloudera.org:8080/#/c/4305/12/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

PS12, Line 676: 
Nit: should be "local Kudu replica" to be consistent with the description of 
the local_replica mode.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Dinesh Bhat (Code Review)
Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 11:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

Line 70: DEFINE_int64(rowset_index, INT64_MAX,
> Why is INT64_MAX a better choice than -1? The advantage of -1 is that it's 
Absence of this flags on cmd-line will mean -1, which means even if we type 
'--rowset_index=-1' by mistake we end up printing everything. I preferred 
INT64_MAX over that. Given that this is meant for adv users, INT64_MAX kinda 
felt intuitive. I changed it back to (-1) anyways because I wasn't against 
either approach.


Line 384:nullptr, // MetricRegistry
> The MRS will be empty if the tablet isn't bootstrapped. So in effect, it's 
I see, ok removed now. removed fomr test too.


PS9, Line 679: 
> Nope, not changed.
yikes !! sorry, actually it's kinda odd that all our required params are 
'tablet_id' and help strings are not referring to tablet anymore :). But, 
laters may be. Also do we not want to honor 80 char wrap-ups ?


PS9, Line 721: 
> Looks like you missed this one.
Done


http://gerrit.cloudera.org:8080/#/c/4305/11/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

PS11, Line 637: tablet
> replica
Done


PS11, Line 645: tablet
> replica
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Dinesh Bhat (Code Review)
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..

tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool

This change ports fs_dump actions under 'kudu local_replica '.
Additionally this has following re-organizations:
- moved dump_cfile action under 'kudu fs dump cfile'.
- kudu-fs_list tool has been removed altogether,
  but some of the functionalities are retained under
  'local_replica' and 'fs dump' sub-actions.
- fs_tool library is stripped off, and all those
  routines are in respective action files.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_tablet.cc
11 files changed, 748 insertions(+), 819 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/12
-- 
To view, visit http://gerrit.cloudera.org:8080/4305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 12:

Build Started http://104.196.14.100/job/kudu-gerrit/3374/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 11:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

Line 70: DEFINE_int64(rowset_index, INT64_MAX,
> Err... updated to INT64_MAX.
Why is INT64_MAX a better choice than -1? The advantage of -1 is that it's 
actually printed in the help as "-1", whereas INT64_MAX looks like some huge 
number that a user might think has meaning as a real number.


Line 384:nullptr, // MetricRegistry
> I can take it out, but it looked like it was dumping some in-memory content
The MRS will be empty if the tablet isn't bootstrapped. So in effect, it's 
still doing the same thing as "dump rowset". Plus it's doing it in a poor way: 
it loads _all_ the data into an in-memory vector, and the data is loaded as 
debug strings, which aren't as useful as how "dump rowset" prints.


PS9, Line 521: }
 : 
> Done, doesn't the 4-space indent next-line apply here ?
Generally yes, but sometimes people deviate from that style to "pretty print" 
some stuff (like this), and when they do, I think it's better code etiquette to 
preserve that style, at least on a file-by-file basis.


PS9, Line 607: // Rowset index no
> Heh, somewhere came across in Strostrup book few weeks ago, this way of ini
Primitive types don't have constructors/destructors the way that classes and 
structs do. It's all just raw memory.


PS9, Line 679: 
> Done
Nope, not changed.


PS9, Line 721: 
> Should be 'replica' now.
Looks like you missed this one.


http://gerrit.cloudera.org:8080/#/c/4305/11/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

PS11, Line 637: tablet
replica


PS11, Line 645: tablet
replica


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Dinesh Bhat (Code Review)
Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

Line 70: DEFINE_int64(rowset_index, INT64_MAX,
> Yeah, plus checking with if(FLAGS_rowset_index) would yield unexpected resu
Err... updated to INT64_MAX.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 11:

Build Started http://104.196.14.100/job/kudu-gerrit/3370/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Dinesh Bhat (Code Review)
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..

tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool

This change ports fs_dump actions under 'kudu local_replica '.
Additionally this has following re-organizations:
- moved dump_cfile action under 'kudu fs dump cfile'.
- kudu-fs_list tool has been removed altogether,
  but some of the functionalities are retained under
  'local_replica' and 'fs dump' sub-actions.
- fs_tool library is stripped off, and all those
  routines are in respective action files.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_tablet.cc
11 files changed, 792 insertions(+), 817 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/11
-- 
To view, visit http://gerrit.cloudera.org:8080/4305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 10:

Build Started http://104.196.14.100/job/kudu-gerrit/3368/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Dinesh Bhat (Code Review)
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..

tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool

This change ports fs_dump actions under 'kudu local_replica '.
Additionally this has following re-organizations:
- moved dump_cfile action under 'kudu fs dump cfile'.
- kudu-fs_list tool has been removed altogether,
  but some of the functionalities are retained under
  'local_replica' and 'fs dump' sub-actions.
- fs_tool library is stripped off, and all those
  routines are in respective action files.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_tablet.cc
11 files changed, 792 insertions(+), 817 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Dinesh Bhat (Code Review)
Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 9:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

PS8, Line 114: 
> In the latest diff (PS9) it's still there.
Done


PS8, Line 476: 
 :   // TODO: it's awkward that whenever we want to iterate over 
deltas we also
 :   // need to open the CFileSet for the rowset. Ide
> Yes, but you didn't update the comment to mention unique_ptr instead of gsc
Done


PS8, Line 682:   .AddOptionalParameter("fs_data_dirs")
> But the two aren't equivalent. "All rowsets" means all on-disk rowsets, whi
What I meant was 'dump blocks' was dumping all rowsets for a tablet_id, only 
diff between the two actions were row_index, hence combined them now.


http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

Line 70: DEFINE_int64(rowset_index, 0, "Index of the rowset in local replica");
> But rowset index 0 is a valid index. You should default to -1 here, and mod
Yeah, plus checking with if(FLAGS_rowset_index) would yield unexpected results 
as well. I am using INT_MAX as default value instead of -1. Updated help 
accordingly.


PS9, Line 139:   fs_ptr->reset(new FsManager(Env::Default(), fs_opts));
 :   RETURN_NOT_OK((*fs_ptr)->Open());
> Nit: generally, we prefer to use the calling convention where the OUT param
Done


Line 384: Status DumpData(const RunnerContext& context) {
> I took a look at how this is implemented, and I think we should remove it t
I can take it out, but it looked like it was dumping some in-memory contents 
too(MRS), and wasn't sure about removing it. Perhaps we can consolidate 'dump 
data' and 'dump rowset' post 1.0 ? I didn't want to remove altogether and 
re-add later if we find something missing.


PS9, Line 434: bool metadata_only
> Every time this function is called, FLAGS_metadata_only is fed into this ar
Done


PS9, Line 521: cout << Indent(indent) << upd.key.ToString() << " "
 : << RowChangeList(upd.cell).ToString(schema) << endl;
> The old code (before removing the std:: prefixes) took care to align the <<
Done, doesn't the 4-space indent next-line apply here ?


PS9, Line 607: bool found{false};
> What's this? Why not "bool found = false;"?
Heh, somewhere came across in Strostrup book few weeks ago, this way of 
initializing is supported for built-in types too, but I am curious if the code 
in 'found = false' would resort to invoking constructor too for built-in types. 
May be not.


PS9, Line 667: tablet
> Replica.
Done


PS9, Line 669:   .AddOptionalParameter("rowset_index")
> Resort this list of parameters alphabetically.
Done


PS9, Line 679: tablet
> Should now be 'replica'.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-12 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 9:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

PS9, Line 687: kRownId
kRowId


http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

PS8, Line 114: 
> Agreed, done.
In the latest diff (PS9) it's still there.


PS8, Line 476: 
 :   // TODO: it's awkward that whenever we want to iterate over 
deltas we also
 :   // need to open the CFileSet for the rowset. Ide
> Done
Yes, but you didn't update the comment to mention unique_ptr instead of 
gscoped_ptr.


PS8, Line 682:   .AddOptionalParameter("fs_data_dirs")
> Very interesting catch ! it turns out dump blocks was nothing but dumping a
But the two aren't equivalent. "All rowsets" means all on-disk rowsets, which 
means all on-disk data blocks.


http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

Line 70: DEFINE_int64(rowset_index, 0, "Index of the rowset in local replica");
But rowset index 0 is a valid index. You should default to -1 here, and modify 
the help text to explain the special value.


PS9, Line 139:   fs_ptr->reset(new FsManager(Env::Default(), fs_opts));
 :   RETURN_NOT_OK((*fs_ptr)->Open());
Nit: generally, we prefer to use the calling convention where the OUT parameter 
(fs_ptr in this case) isn't modified unless the function succeeds. So what you 
should do is store the new FsManager in a local unique_ptr, then after Open() 
succeeds, swap the local unique_ptr with fs_ptr.


Line 384: Status DumpData(const RunnerContext& context) {
I took a look at how this is implemented, and I think we should remove it too. 
The problem is that it purports to dumping all of the data of the tablet, but 
it doesn't actually bootstrap the tablet, so any data in WAL segments is 
missed. After that, it's functionally equivalent to DumpRowSet (with 
rowset_index==0) in that it'll just dump all the on-disk blocks.


PS9, Line 434: bool metadata_only
Every time this function is called, FLAGS_metadata_only is fed into this 
argument, so just drop the argument and access FLAGS_metadata_only directly 
inside the function.


PS9, Line 521: cout << Indent(indent) << upd.key.ToString() << " "
 : << RowChangeList(upd.cell).ToString(schema) << endl;
The old code (before removing the std:: prefixes) took care to align the << 
that begins each line. Could you do the same?

This comment applies elsewhere in this file too.


PS9, Line 607: bool found{false};
What's this? Why not "bool found = false;"?


PS9, Line 667: tablet
Replica.

Please make the same changes fo rthe other dump Descriptions.


PS9, Line 669:   .AddOptionalParameter("rowset_index")
Resort this list of parameters alphabetically.


PS9, Line 679: tablet
Should now be 'replica'.


PS9, Line 721: tablet
Should be 'replica' now.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-11 Thread Dinesh Bhat (Code Review)
Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 8:

(35 comments)

Thank you again Adar, addressed rev comments below, please see responses too 
inline for few of the comments.

http://gerrit.cloudera.org:8080/#/c/4305/8/docs/release_notes.adoc
File docs/release_notes.adoc:

PS8, Line 71: local_repica
> local_replica
Done


http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

PS5, Line 517:   ASSERT_STR_MATCHES(stdout, "Header:");
 :   ASSERT_STR_MATCHES(stdout, "1\\.1@1");
> Hmm, not exactly what I meant. What I mean is: you're already doing a subst
I see, sorry for misunderstanding earlier comment. Given that these are common 
args for almost all commands I liked defining them in one place with an 
intuitive variable name instead of spraying "--" args everywhere. I am keeping 
them as-is where they are used multiple times but only changing them at places 
where they are used only once. Lemme know.


http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

Line 20: #include 
> Nit: should precede string.
Done


Line 215: const vector kLocalReplicaModeRegexes = {
> Shouldn't there be a dump mode in here? And something for "list all replica
Done


PS8, Line 594:   string fs_paths = "--fs_wal_dir=" + kTestDir + " "
 :   "--fs_data_dirs=" + kTestDir;
> Same comment about partial substitution here.
Done


Line 596:   LOG(INFO) < This was to help you debug, right? Can it be removed now?
thank you, good catch.


http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:

Line 19: #include "kudu/tools/tool_action_common.h"
> As before, this include doesn't belong up here.
Done


http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

Line 28: #include "kudu/common/schema.h"
> Nit: should go after row*
Done


Line 33: #include "kudu/consensus/consensus.pb.h"
> Nit: shoudl go after consensus_meta.h.
Done


Line 53: #include "kudu/tablet/rowset_metadata.h"
> Nit: should come before tablet.h.
Done


Line 55: #include "kudu/tserver/tserver.pb.h"
> Nit: should go after tablet_copy_client.h.
Done


PS8, Line 58: #include "kudu/util/logging.h"
> Nit: should go after env_util.h.
Thank you, no doubt I did a pretty bad job here :)


PS8, Line 71: information(if any)
> Nit: information (if any)
Done


PS8, Line 114: DumpOptions
> I'd drop this struct altogether, because:
Agreed, done.


PS8, Line 286: static
> Nit: don't need this; the function is already in an anonymous namespace.
Fixed.


Line 287:   const RowSetMetadata& rs_meta) {
> Nit: fix the indentation on this line.
Done


Line 292: std::cout << "Column block for column ID " << col_id;
> std::cout and std::endl are already in the 'using' blocks above, so you can
This became one  indentation adjustment fun in the entire file :)


PS8, Line 318:   const string* tablet_id = FindOrNull(context.required_args, 
"tablet_id");
 :   if (tablet_id == nullptr) {
 : LOG(INFO) << "No tablet_id specified, dumping all tablets:";
 :   }
> I understand the existing tool allowed this 'no tablet_id means dump all ta
Good catch, fixed.


PS8, Line 346: FsManager& fs_manager
> Google style frowns on passing arguments by ref. Your options are:
Cool, thanks. I changed them to pointer - pointer mainly because few following 
callsites like FsManager::OpenBlock has non-const nature and also 
TabletMetadata::Load takes a raw pointer, etc. I didn't really chase after why 
Load function is taking a raw pointer to keep the scope of this change.


Line 381:   std::cout << "\t" << tablet << std::endl;
> In this case, let's not bother with the leading tab. It'd be easier to pars
Done


PS8, Line 398: scoped_refptr(nullptr)
> I think this can just be "scoped_refptr()".
Interesting, done. I didn't really take this as an opportunity to tamper with 
the original code, although part of the exercise was that.


PS8, Line 399: nullptr
> When passing a nullptr like this, consider the following style:
Very helpful, thank you.


PS8, Line 432: FsManager& fs_manager
> Const ref here too.
Done


PS8, Line 476:   // NewDeltaIterator returns Status::OK() iff a new 
DeltaIterator is created. Thus,
 :   // it's safe to have a gscoped_ptr take possesion of 
'raw_iter' here.
 :   gscoped_ptr delta_iter(raw_iter);
> This is correct, but let's use unique_ptr now; this was written before we t
Done


PS8, Line 540: FsManager& fs_manager
> Let's change this to const ref.
Couldn't because of the reason mentioned above, eventually they call into 

[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-11 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 9:

Build Started http://104.196.14.100/job/kudu-gerrit/3362/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-11 Thread Dinesh Bhat (Code Review)
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..

tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool

This change ports fs_dump actions under 'kudu local_replica '.
Additionally this has following re-organizations:
- moved dump_cfile action under 'kudu fs dump cfile'.
- kudu-fs_list tool has been removed altogether,
  but some of the functionalities are retained under
  'local_replica' and 'fs dump' sub-actions.
- fs_tool library is stripped off, and all those
  routines are in respective action files.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_tablet.cc
11 files changed, 806 insertions(+), 817 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-11 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 8:

(38 comments)

http://gerrit.cloudera.org:8080/#/c/4305/8/docs/release_notes.adoc
File docs/release_notes.adoc:

PS8, Line 71: local_repica
local_replica


http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

PS5, Line 517:   ASSERT_STR_MATCHES(stdout, "Header:");
 :   ASSERT_STR_MATCHES(stdout, "1\\.1@1");
> Fixed, it was a blind copy-paste effect.
Hmm, not exactly what I meant. What I mean is: you're already doing a 
substitute on L521; include the fs_wal_dir and fs_data_dirs substitution in 
there rather than doing it out here.

That way the entire command line is more clear; don't need to look in two 
different places.


http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

Line 20: #include 
Nit: should precede string.


Line 215: const vector kLocalReplicaModeRegexes = {
Shouldn't there be a dump mode in here? And something for "list all replicas"?


PS8, Line 594:   string fs_paths = "--fs_wal_dir=" + kTestDir + " "
 :   "--fs_data_dirs=" + kTestDir;
Same comment about partial substitution here.


Line 596:   LOG(INFO) <

[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-11 Thread Dinesh Bhat (Code Review)
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..

tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool

This change ports fs_dump actions under 'kudu local_replica '.
Additionally this has following re-organizations:
- moved dump_cfile action under 'kudu fs dump cfile'.
- kudu-fs_list tool has been removed altogether,
  but some of the functionalities are retained under
  'local_replica' and 'fs dump' sub-actions.
- fs_tool library is stripped off, and all those
  routines are in respective action files.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_tablet.cc
11 files changed, 854 insertions(+), 805 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool

2016-09-11 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..


Patch Set 8:

Build Started http://104.196.14.100/job/kudu-gerrit/3349/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list

2016-09-11 Thread Dinesh Bhat (Code Review)
Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list
..


Patch Set 7:

(1 comment)

Thanks, please take a look at updated patch.

http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:

Line 63
> I'd prefer option #2, as it strips away more of the existing code and (hope
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list

2016-09-10 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:

Line 63: class FsTool {
> Yeah, I couldn't be more convinced here; though I have a follow up Qn about
I'd prefer option #2, as it strips away more of the existing code and 
(hopefully) exposes opportunities to remove old cruft.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list

2016-09-10 Thread Dinesh Bhat (Code Review)
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list
..

tool: port kudu-fs_dump, remove kudu-fs_list

This change also moves dump_cfile action under 'kudu fs dump cfile'.
kudu-fs_list tool has been removed altogether and
a 'kudu fs dump tree' has been added to dump the filesystem tree.
Majority of the kudu-fs_dump sub-commands which were relying on tablet
id as required parameter have been moved under 'kudu tablet'.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_tablet.cc
12 files changed, 1,141 insertions(+), 797 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list

2016-09-09 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list
..


Patch Set 5:

(22 comments)

http://gerrit.cloudera.org:8080/#/c/4305/5/docs/release_notes.adoc
File docs/release_notes.adoc:

PS5, Line 49: - The `cfile-dump` tool has been removed. The same functionality 
is now
:   implemented as `kudu fs cfile_dump`.
Update this.


PS5, Line 58: The `kudu-fs_list` tool has been removed as it was replicating 
most of the
:   functionalities already available under 'kudu fs dump'.
This should be reworded now that some functionality was preserved, right?


http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

Line 202: const vector kFsDumpModeRegexes = {
Need to add uuid to this list.


PS5, Line 501:   const string kTestTablet = "test-tablet";
 :   const string kTestTable = "test-table";
Nit: can you add "Id" as a suffix to these variable names, so it's clear that 
it's a table ID (as opposed to the table name, below)?

In the other new tests too.


PS5, Line 517:   string fs_paths = Substitute("--fs_wal_dir=$0 
--fs_data_dirs=$1",
 :kTestDir, kTestDir);
Nit: not sure what this partial substitution buys you; fs_paths is only used in 
one place here and in the next test.


Line 534:   StripWhiteSpace(_str);
What whitespace is being stripped here?


PS5, Line 568: std::ostringstream
Nit: add this to using statements at the top of the file. Should also add the 
right include too.


Line 570:   ASSERT_STR_MATCHES(stdout, tree_out.str());
We can't do ASSERT_EQ() here?


Line 582:   unique_ptr harness(new TabletHarness(kSchemaWithIds, 
opts));
Can't we just declare the TabletHarness on the stack and avoid the unique_ptr?


Line 583:   CHECK_OK(harness->Create(true));
These CHECK_OK() statements should be ASSERT_OK() instead. CHECK_OK() is only 
needed when being called on a separate thread in a unit test (here we're on the 
main thread).


PS5, Line 587:   gscoped_ptr writer(
 :   new LocalTabletWriter(harness->tablet().get(), ));
 :   gscoped_ptr row(new 
KuduPartialRow());
Likewise, can't we declare these on the stack?


PS5, Line 606: vector rows = strings::Split(stdout, "\n", 
strings::SkipEmpty());
Why not use RunActionStdoutLines to get this?


PS5, Line 621: if (stdout.find("No rowsets found") == string::npos) {
AFAICT this test is deterministic, so will we get "No rowsets found" or not? 
Given that the bulk of the interesting testing is when we don't get that error, 
we should do whatever we need to do up-front (i.e. write more data, or flush, 
or whatever) so that we can convert this into an ASSERT instead.

Below too.


http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:

Line 63: class FsTool {
My mistake, I wasn't clear when I asked you to remove fs_tool. I didn't mean 
"move FsTool from the fs_tool.* code files into tool_action.*"; I meant "remove 
the FsTool abstraction altogether and replace it with stateless functions".

Given the simplicity of what we're trying to do, I don't think the abstraction 
buys us anything, and I'd like to use this opportunity (of writing a new tool) 
to get rid of old abstractions and assumptions.


http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:

PS5, Line 138: Print
Nit: Change to "Dump" to be consistent with other dump actions.


http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_tablet.cc
File src/kudu/tools/tool_action_tablet.cc:

Line 19: #include "kudu/tools/tool_action_common.h"
This actually belongs where it was previously. The top-most include is reserved 
for the primary header of this compilation unit.

See 
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes 
for details.


PS5, Line 331: dump_wals
When you add a dump submode, move this into it too.


PS5, Line 343: list_blocks
How about "dump_block_ids"? Then you can add it to a dump submode, rename the 
associated function "DumpTabletBlockIds", and change the description to "Dump 
the IDs of all blocks belonging to a tablet".


Line 351:   ActionBuilder("list_tablets", )
Nit: let's just call this one "list", because "kudu tablet list" makes 
intuitive sense.


PS5, Line 385: tablet
Nit: "of a tablet"

Above too.


PS5, Line 385: rowset
Nit: rowset contents


PS5, Line 399:   .AddAction(std::move(list_blocks))
 :   .AddAction(std::move(list_tablets))
 :   .AddAction(std::move(dump_blocks))
 :   .AddAction(std::move(dump_data))
 :   .AddAction(std::move(dump_meta))
 :   .AddAction(std::move(dump_rowset))
How about a 'dump' mode to consolidate? Maybe 

[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list

2016-09-09 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list
..


Patch Set 5:

Build Started http://104.196.14.100/job/kudu-gerrit/3304/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)

2016-09-08 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP)
..


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/3279/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)

2016-09-08 Thread Dinesh Bhat (Code Review)
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP)
..

tool: port kudu-fs_dump, remove kudu-fs_list (WIP)

This change also moves dump_cfile action under 'kudu fs dump cfile'.
kudu-fs_list tool has been removed altogether and
a 'kudu fs dump tree' has been added to dump the filesystem tree.
Majority of the kudu-fs_dump sub-commands which were relying on tablet
id as required parameter have been moved under 'kudu tablet'.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_dump-tool.cc
D src/kudu/tools/fs_list-tool.cc
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_tablet.cc
13 files changed, 1,117 insertions(+), 1,151 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)

2016-09-06 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP)
..


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/3253/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)

2016-09-06 Thread Dinesh Bhat (Code Review)
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP)
..

tool: port kudu-fs_dump, remove kudu-fs_list (WIP)

This change also moves dump_cfile action under 'kudu fs dump cfile'.
kudu-fs_list tool has been removed altogether and
a 'kudu fs dump tree' has been added to dump the filesystem tree.
Majority of the kudu-fs_dump sub-commands which were relying on tablet
id as required parameter have been moved under 'kudu tablet'.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_dump-tool.cc
D src/kudu/tools/fs_list-tool.cc
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_tablet.cc
12 files changed, 1,087 insertions(+), 1,146 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)

2016-09-06 Thread Dinesh Bhat (Code Review)
Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP)
..


Patch Set 1:

(7 comments)

TFTR Adar, updated the patch, please re-review, also added some more tests. I 
think I can keep adding some more test exercises as you review them.

http://gerrit.cloudera.org:8080/#/c/4305/1/docs/release_notes.adoc
File docs/release_notes.adoc:

Line 58: - The `kudu-fs_list` tool has been removed as it was replicating most 
of the
> As I mentioned in HipChat the other day, I think the LIST_BLOCKS functional
Added that back, although I have kept some of the sub commands like 
tree/list_tablets as well now since it's useful to pretty print them instead of 
relying on 'find'. Also please find some routines like ListLogSegments etc 
which I think could be removed. I will do so after taking some confirmation 
from your end.


PS1, Line 59: options
> Nit: change to 'functionality'
Done


http://gerrit.cloudera.org:8080/#/c/4305/1/src/kudu/tools/CMakeLists.txt
File src/kudu/tools/CMakeLists.txt:

PS1, Line 55: add_library(fs_tool fs_tool.cc)
: target_link_libraries(fs_tool
:   gutil
:   kudu_common
:   server_common
:   consensus
:   tablet)
> Can you remove fs_tool altogether and move the needed functionality into th
I have done this, but not exactly as you described. I moved pretty much 
everything under tool_action_common(as opposed to moving just common routines), 
reason being I thought these subcommands may go through another round of 
re-shuffle after feedbacks from dev/users, we could start reorganizing them at 
that point. For now, FsTool simply moved from fs_tool library to 
tool_action_common.* and got rid of fs_tool library as such.


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

PS1, Line 187:   unique_ptr dump_fs_blocks =
 :   ActionBuilder("tablet_blocks", )
 :   .Description("Dump the data of tablet")
 :   .AddRequiredParameter({ "tablet_id", "tablet identifier" })
 :   .AddOptionalParameter("fs_wal_dir")
 :   .AddOptionalParameter("fs_data_dirs")
 :   .Build();
 : 
 :   unique_ptr dump_fs_data =
 :   ActionBuilder("tablet_data", )
 :   .Description("Dump the data of tablet")
 :   .AddRequiredParameter({ "tablet_id", "tablet identifier" })
 :   .AddOptionalParameter("fs_wal_dir")
 :   .AddOptionalParameter("fs_data_dirs")
 :   .Build();
 : 
 :   unique_ptr dump_fs_meta =
 :   ActionBuilder("tablet_meta", )
 :   .Description("Dump the metadata of tablet")
 :   .AddRequiredParameter({ "tablet_id", "tablet identifier" })
 :   .AddOptionalParameter("fs_wal_dir")
 :   .AddOptionalParameter("fs_data_dirs")
 :   .Build();
 : 
 :   unique_ptr dump_fs_rowset =
 :   ActionBuilder("rowset", )
 :   .Description("Dump the rowset of tablet")
 :   .AddRequiredParameter({ "tablet_id", "tablet identifier" })
 :   .AddRequiredParameter({ "rowset_index", "rowset index" })
 :   .AddOptionalParameter("fs_wal_dir")
 :   .AddOptionalParameter("fs_data_dirs")
 :   .Build();
> These are all scoped to a tablet, so I think they should be in the tablet m
Done, kept the dump_wals as-is since it seemed to be taking tablet_id as param.


PS1, Line 220:   unique_ptr dump_tree =
 :   ActionBuilder("tree", )
 :   .Description("Dump the tree of Kudu filesystem")
 :   .AddOptionalParameter("fs_wal_dir")
 :   .AddOptionalParameter("fs_data_dirs")
 :   .Build();
> Let's drop this one altogether since basic UNIX tools like find can do the 
Hmmm, I kept this only for pretty printing purposes, kinda looks nice with 
hierarchies enforced by indentations.


PS1, Line 237:   .AddAction(std::move(dump_fs_blocks))
 :   .AddAction(std::move(dump_fs_data))
 :   .AddAction(std::move(dump_fs_meta))
 :   .AddAction(std::move(dump_fs_rowset))
> These are all scoped to the 'fs' mode, so let's drop the fs infix.
Done


PS1, Line 242: print_uuid
> For consistency, let's change this to dump_uuid. You'll probably need to up
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master

[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)

2016-09-06 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP)
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/3249/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)

2016-09-06 Thread Dinesh Bhat (Code Review)
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP)
..

tool: port kudu-fs_dump, remove kudu-fs_list (WIP)

This change also moves dump_cfile action under 'kudu fs dump cfile'.
kudu-fs_list tool has been removed altogether and
a 'kudu fs dump tree' has been added to dump the filesystem tree.
Majority of the kudu-fs_dump sub-commands which were relying on tablet
id as required parameter have been moved under 'kudu tablet'.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/fs_dump-tool.cc
D src/kudu/tools/fs_list-tool.cc
D src/kudu/tools/fs_tool.cc
D src/kudu/tools/fs_tool.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_tablet.cc
12 files changed, 1,081 insertions(+), 1,146 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)

2016-09-02 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP)
..


Patch Set 1:

(7 comments)

Just did a quick first pass.

http://gerrit.cloudera.org:8080/#/c/4305/1/docs/release_notes.adoc
File docs/release_notes.adoc:

Line 58: - The `kudu-fs_list` tool has been removed as it was replicating most 
of the
As I mentioned in HipChat the other day, I think the LIST_BLOCKS functionality 
in kudu-fs_list should be preserved, because there isn't a 1-1 relationship 
between files and blocks, and so finding all blocks (scoped to a tablet or in 
total) isn't trivial.


PS1, Line 59: options
Nit: change to 'functionality'


http://gerrit.cloudera.org:8080/#/c/4305/1/src/kudu/tools/CMakeLists.txt
File src/kudu/tools/CMakeLists.txt:

PS1, Line 55: add_library(fs_tool fs_tool.cc)
: target_link_libraries(fs_tool
:   gutil
:   kudu_common
:   server_common
:   consensus
:   tablet)
Can you remove fs_tool altogether and move the needed functionality into the 
tool_action_* code files? Especially with the removal of fs_list-tool there's 
functionality in fs_tool that's no longer needed.


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

PS1, Line 187:   unique_ptr dump_fs_blocks =
 :   ActionBuilder("tablet_blocks", )
 :   .Description("Dump the data of tablet")
 :   .AddRequiredParameter({ "tablet_id", "tablet identifier" })
 :   .AddOptionalParameter("fs_wal_dir")
 :   .AddOptionalParameter("fs_data_dirs")
 :   .Build();
 : 
 :   unique_ptr dump_fs_data =
 :   ActionBuilder("tablet_data", )
 :   .Description("Dump the data of tablet")
 :   .AddRequiredParameter({ "tablet_id", "tablet identifier" })
 :   .AddOptionalParameter("fs_wal_dir")
 :   .AddOptionalParameter("fs_data_dirs")
 :   .Build();
 : 
 :   unique_ptr dump_fs_meta =
 :   ActionBuilder("tablet_meta", )
 :   .Description("Dump the metadata of tablet")
 :   .AddRequiredParameter({ "tablet_id", "tablet identifier" })
 :   .AddOptionalParameter("fs_wal_dir")
 :   .AddOptionalParameter("fs_data_dirs")
 :   .Build();
 : 
 :   unique_ptr dump_fs_rowset =
 :   ActionBuilder("rowset", )
 :   .Description("Dump the rowset of tablet")
 :   .AddRequiredParameter({ "tablet_id", "tablet identifier" })
 :   .AddRequiredParameter({ "rowset_index", "rowset index" })
 :   .AddOptionalParameter("fs_wal_dir")
 :   .AddOptionalParameter("fs_data_dirs")
 :   .Build();
These are all scoped to a tablet, so I think they should be in the tablet mode, 
not fs. Given that, probably makes sense to leave dump_cfile as it was in fs 
and put the dump mode in tablet. You can move 'dump_wals' already in tablet 
into the new dump mode too.


PS1, Line 220:   unique_ptr dump_tree =
 :   ActionBuilder("tree", )
 :   .Description("Dump the tree of Kudu filesystem")
 :   .AddOptionalParameter("fs_wal_dir")
 :   .AddOptionalParameter("fs_data_dirs")
 :   .Build();
Let's drop this one altogether since basic UNIX tools like find can do the same 
thing.


PS1, Line 237:   .AddAction(std::move(dump_fs_blocks))
 :   .AddAction(std::move(dump_fs_data))
 :   .AddAction(std::move(dump_fs_meta))
 :   .AddAction(std::move(dump_fs_rowset))
These are all scoped to the 'fs' mode, so let's drop the fs infix.


PS1, Line 242: print_uuid
For consistency, let's change this to dump_uuid. You'll probably need to update 
several files.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)

2016-09-02 Thread Dinesh Bhat (Code Review)
Hello Adar Dembo, Todd Lipcon,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP)
..

tool: port kudu-fs_dump, remove kudu-fs_list (WIP)

This change also moves dump_cfile action under 'kudu fs dump cfile'.
kudu-fs_list tool has been removed altogether and
a 'kudu fs dump tree' has been added to dump the filesystem tree.

Also added tests under kudu-tool-test to exercise each of these fs tools.

Change-Id: I1ec628b65613011d8c48b6239c13762276425966
---
M docs/release_notes.adoc
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
4 files changed, 223 insertions(+), 30 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)

2016-09-02 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP)
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/3213/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No