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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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