[kudu-CR](gh-pages) Update the docs webpages to reflect the master branch

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: Update the docs webpages to reflect the master branch
..


Patch Set 1:

Sounds good Todd, if the consensus is to generate the docs after the release, I 
will abandon this review, plus this diff is outdated anyways as we have gone 
through bunch of master docs changes lately.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5a760ec169880954961f2da2a288f4d963e87d84
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR](gh-pages) Update the docs webpages to reflect the master branch

[Dinesh Bhat (Code Review)]

Dinesh Bhat has abandoned this change.

Change subject: Update the docs webpages to reflect the master branch
..


Abandoned

Abandoning this patch as this is outdated now, and we are gonna publish web 
pages aligned with release for now. For future, as Todd suggested we can 
publish with an in-progress to release' label. Further I was thinking we can 
automate this docs/webpage update to be a one step process, i.e a post-hook to 
docs section could generate html pages and publish(for unreleased versions).

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I5a760ec169880954961f2da2a288f4d963e87d84
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Fix flakiness in RaftConsensusITest.TestReplaceChangeConfigOperation

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: Fix flakiness in 
RaftConsensusITest.TestReplaceChangeConfigOperation
..


Patch Set 2: -Code-Review

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3819/2//COMMIT_MSG
Commit Message:

Line 47: 
Also a nit: You may want to link this commit to KUDU-1548 I triaged where I 
have listed the error logs, or you can try to condense this to 80 columns 
somehow ? Latter would be ugly I guess.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib91b5cc974656e82f670d6a938f537b63338d036
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Fix flakiness in RaftConsensusITest.TestReplaceChangeConfigOperation

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: Fix flakiness in 
RaftConsensusITest.TestReplaceChangeConfigOperation
..


Patch Set 2: Code-Review+1

(1 comment)

Thanks for quickly patching this Mike. Not your change, but can you confirm if 
this StartElection doesn't need to wait at few other places ? 
RaftConsensusITest(line 594), RemoteBootStrapITest and DleteTabletTest as there 
are few tests which aren't waiting for leader election before pumping the 
workload.

http://gerrit.cloudera.org:8080/#/c/3819/2/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:

Line 141:   RETURN_NOT_OK(GetLastOpIdForEachReplica(tablet_id, { replica }, 
opid_type, timeout, _ids));
Was this styling issue or any impact in the way vector is passed in ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib91b5cc974656e82f670d6a938f537b63338d036
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1500: Fix the data race during RaftConsensusITest.TestCorruptReplicaMetadata

[Dinesh Bhat (Code Review)]

Dinesh Bhat has uploaded a new patch set (#2).

Change subject: KUDU-1500: Fix the data race during 
RaftConsensusITest.TestCorruptReplicaMetadata
..

KUDU-1500: Fix the data race during 
RaftConsensusITest.TestCorruptReplicaMetadata

The test intends to corrupt the metadata of one of the tserver tablets.
While the cluster is in the process of resurrecting the corrupt metadata,
the partition schema is accessed in an unguarded manner from another
thread servicing ListTablets API.

The proposed fix here accesses the metadata partition resources via
copy by value mechanism now in slow paths, where copy itself happens
under the same lock which makes the metadata resurrection operation
idempotent. Fast paths are untouched since the parition schema is
bound to be visible only after tablet metadata is resurrected.

Testing: Passed about 2000 iteration of the failing test
raft_consensus-itest.TestCorruptReplicaMetadata

Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
---
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_peer.cc
M src/kudu/tools/fs_tool.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver-path-handlers.cc
8 files changed, 25 insertions(+), 18 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Add docs for non-covering range partitioning

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: Add docs for non-covering range partitioning
..


Patch Set 1:

Also, please correct a typo in our FAQ along with this change: "partitioning is 
efficient when there are arge numbers"

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b0fd7500c5399db9dcad617ae67fea247307353
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Misty Stanley-Jones 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] fs tool: improve format for dumping a rowset

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: fs_tool: improve format for dumping a rowset
..


Patch Set 1: Code-Review+1

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

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

[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1500: Data race in 
RaftConsensusITest.TestCorruptReplicaMetadata
..


Patch Set 8:

(2 comments)

TFTR Dan, addressed both comments with responses inline...

http://gerrit.cloudera.org:8080/#/c/3823/7/src/kudu/integration-tests/tablet_copy-itest.cc
File src/kudu/integration-tests/tablet_copy-itest.cc:

Line 323:   barrier.Wait();
> Could this be done with a countdown latch or similar?  5 seconds is a very 
Just stumbled across a cute Barrier implementation in util/, I am beginning to 
appreciate the power of C++ util libraries at the moment :)


http://gerrit.cloudera.org:8080/#/c/3823/7/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:

Line 298:   LOG_WITH_PREFIX(WARNING) << "Upgrading from Kudu 0.5.0 directly 
to this"
> I think this is a little hard to follow, perhaps a better way to state it i
Done, although I wonder if there are any other situations which meet 
!superblock.has_partition() in which case this message may become  misleading.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-1500: Data race in 
RaftConsensusITest.TestCorruptReplicaMetadata
..

KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

The test intends to corrupt the metadata of one of the tserver tablets.
While the cluster is in the process of resurrecting the failed tserver,
the Partition and PartitionSchema on that tserver is accessed using
TabletPeer in an unguarded manner via ListTablets RPCs.

The proposed fix here keeps the Partition and PartitionSchema immutable
once after it is loaded during replica bootstrap. These fields are never
overwritten again during Tablet Copy or any other workflow.
Also a unit test is added to exercise this data race window. i.e,
the unit test aims to issue ListTablets RPCs during a follower's
tablet copy stage and provides an optimistic coverage for the fix.

Testing: Passed about 2000 iteration of the failing test
raft_consensus-itest.TestCorruptReplicaMetadata

Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
---
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.cc
4 files changed, 138 insertions(+), 16 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

[Dinesh Bhat (Code Review)]

Dinesh Bhat has abandoned this change.

Change subject: KUDU-1500: Data race in 
RaftConsensusITest.TestCorruptReplicaMetadata
..


Abandoned

abandoning due to duplicate links. original review is under 
http://gerrit.cloudera.org:8080/3823

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I0cfea3ea1b8e308fe05a0b3e94c93b7b1369ac24
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] [tools] Add missing help text from few tools

[Dinesh Bhat (Code Review)]

Hello Mike Percy, Adar Dembo,

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

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

to review the following change.

Change subject: [tools] Add missing help text from few tools
..

[tools] Add missing help text from few tools

Change-Id: I49cb5d34fcd5153435d6ad4c34d496a8f58cc144
---
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_remote_replica.cc
M src/kudu/tools/tool_action_test.cc
3 files changed, 3 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I49cb5d34fcd5153435d6ad4c34d496a8f58cc144
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 

[kudu-CR] tool: remove dead code

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: tool: remove dead code
..


Patch Set 1: Code-Review+1

It's good you noticed them Adar. Ship it from my side.

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

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

[kudu-CR] WIP KUDU-1330: Add a tool to unsafely recover from loss of majority replicas

[Dinesh Bhat (Code Review)]

Hello David Ribeiro Alves, Mike Percy,

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

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

to review the following change.

Change subject: WIP KUDU-1330: Add a tool to unsafely recover from loss of 
majority replicas
..

WIP KUDU-1330: Add a tool to unsafely recover from loss of majority replicas

This patch adds an API to allow unsafe config change via an external
recovery tool 'kudu remote_replica replace_config'.

This tool lets us replace a 3-node config on a tablet server with a
1-node config. This is perticularly useful when we have 2 out of 3
replicas down and we want to bring the tablet back to operational state.
We can use this tool to force a new config on the surviving node providing
all the details of the new config from the tool. As a result
of the forced config change, the automatic leader election kicks in via
raft mechanisms and the re-replication is triggered from master to bring
the replica count back upto 3-node config.
The lonely survivor talking to the tool tends to become the leader in
new config in majority of the use cases because:
a) The API/tool acts as a fake leader mimicking the actual leader the node
   had voted for and replicate the new config with a higher term and
   bumped up op_index. This ensures that other 2 nodes added later on respect
   the term emitted by this node and accept this node as leader.
a) Assumption is that, the dead nodes are not coming back with a higher term,
   hence leadership is retained.

Also the ReplaceConfig() API adds a way to abort a pending config change
because pending config comes in the way of recovery tool trying to
replicate/commit the new config on the surviving tablet server. There is only
one pending config change allowed at a time for a given tablet, hence
aborting the pending config change seems safest bet.

This patch is a first in series for unsafe config changes, and assumes that
the dead servers are not coming back while the new config change is taking
effect.

TODO:
0) Accept more replica_uuids from the command line to make support multiple
   peers to be added in the new config.
1) Add a test case when 1 leader is alive.
2) Add a test case for when the node has a pending config change,
   covering the cases when the node is a leader or a follower.
3) Test with a 5-replica config forcing the old {ABCDE} to new {AB} on A.

Change-Id: I908d8c981df74d56dbd034e72001d379fb314700
---
M src/kudu/consensus/consensus.h
M src/kudu/consensus/consensus.proto
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_remote_replica.cc
M src/kudu/tserver/tablet_service.cc
11 files changed, 310 insertions(+), 86 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I908d8c981df74d56dbd034e72001d379fb314700
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Mike Percy 

[kudu-CR] [consensus] KUDU-1613: Fix replica eviction failure for WRONG SERVER UUID

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [consensus] KUDU-1613: Fix replica eviction failure for 
WRONG_SERVER_UUID
..


Patch Set 7:

(6 comments)

TFTR Mike, I will be adding one more test which changes just the UUID keeping 
the tablets intact (which means all tablets are reported behind a new server 
UUID but behind same RPC endpoints) after I figure out a anomaly with 
non_participant I observed in the new test.

http://gerrit.cloudera.org:8080/#/c/5111/7/src/kudu/consensus/consensus_peers.cc
File src/kudu/consensus/consensus_peers.cc:

Line 305: if (response_.error().code() != 
TabletServerErrorPB::WRONG_SERVER_UUID &&
> I think we should consider WRONG_SERVER_UUID as unresponsive (ERROR) instea
Done, please note that as per our offline discussions last week, the tablet 
copy is driven due to this error code deemed as tolerable until heartbeat 
timeout kicks out the replica and re-replication is triggered at taht point.


http://gerrit.cloudera.org:8080/#/c/5111/7/src/kudu/consensus/consensus_queue-test.cc
File src/kudu/consensus/consensus_queue-test.cc:

Line 816: // Verify that tablet copy is triggered when peer responds with
> Is this test still supposed to be here?
Not after we removed the WRONG_SERVER_UUID from queue->ResponseFromPeer(). 
Updated.


http://gerrit.cloudera.org:8080/#/c/5111/7/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:

Line 597:   peer->needs_tablet_copy = true;
> Why this for WRONG_SERVER_UUID?
I suppose this isn't needed if we are driving the tablet-copy via replica 
eviction and then re-replication, right ?


http://gerrit.cloudera.org:8080/#/c/5111/7/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:

Line 2789:   MonoDelta kTimeout = MonoDelta::FromSeconds(30);
> nit: const
Done


Line 2795:   int leader_idx = -1;
> nit: This is written in a C-like way. It would be a bit more readable to de
Done


Line 2816: 
> Please add a comment in here to explain what's going on:
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0d3f84fd297a8e4760208a213c1ee393e92499a3
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [util] fixed env-test on OS X

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [util] fixed env-test on OS X
..


Patch Set 2:

(2 comments)

I am not seeing radio buttons on gerrit, perhaps some browser plugin issue I am 
facing at the moment, so consider +1 from my side.

http://gerrit.cloudera.org:8080/#/c/5738/2//COMMIT_MSG
Commit Message:

Line 14: 
Do you want to post the stacktrace of the failure ?


http://gerrit.cloudera.org:8080/#/c/5738/2/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

Line 1277:   l.rlim_max = limit;
> I'm not following: why would the rlimit struct be relevant here?  The probl
Calling setrlimit down below with astronomical value was the original cause; So 
if rlim_max was uninitialized, 'rlim_max(uint64_t) = limit(uint32_t)' here will 
end up in same issue. But ignore this, I just confirmed that structs come with 
default constructors too, so this is safe.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic355912b98b3fa592481e2457147e23de98447ea
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] WIP KUDU-1330: Add a tool to unsafely recover from loss of majority replicas

[Dinesh Bhat (Code Review)]

Hello David Ribeiro Alves, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: WIP KUDU-1330: Add a tool to unsafely recover from loss of 
majority replicas
..

WIP KUDU-1330: Add a tool to unsafely recover from loss of majority replicas

This patch adds an API to allow unsafe config change via an external
recovery tool 'kudu remote_replica replace_config'.

This tool lets us replace a 3-node config on a tablet server with a
1-node config. This is perticularly useful when we have 2 out of 3
replicas down and we want to bring the tablet back to operational state.
We can use this tool to force a new config on the surviving node providing
all the details of the new config from the tool. As a result
of the forced config change, the automatic leader election kicks in via
raft mechanisms and the re-replication is triggered from master to bring
the replica count back upto 3-node config.
The lonely survivor talking to the tool tends to become the leader in
new config in majority of the use cases because:
a) The API/tool acts as a fake leader mimicking the actual leader the node
   had voted for and replicate the new config with a higher term and
   bumped up op_index. This ensures that other 2 nodes added later on respect
   the term emitted by this node and accept this node as leader.
a) Assumption is that, the dead nodes are not coming back with a higher term,
   hence leadership is retained.

Also the ReplaceConfig() API adds a way to abort a pending config change
because pending config comes in the way of recovery tool trying to
replicate/commit the new config on the surviving tablet server. There is only
one pending config change allowed at a time for a given tablet, hence
aborting the pending config change seems safest bet.

This patch is a first in series for unsafe config changes, and assumes that
the dead servers are not coming back while the new config change is taking
effect.

TODO:
0) Accept more replica_uuids from the command line to make support multiple
   peers to be added in the new config.
1) Add a test case when 1 leader is alive.
2) Add a test case for when the node has a pending config change,
   covering the cases when the node is a leader or a follower.
3) Test with a 5-replica config forcing the old {ABCDE} to new {AB} on A.

Change-Id: I908d8c981df74d56dbd034e72001d379fb314700
---
M src/kudu/consensus/consensus.h
M src/kudu/consensus/consensus.proto
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/time_manager.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_remote_replica.cc
M src/kudu/tserver/tablet_service.cc
14 files changed, 488 insertions(+), 96 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I908d8c981df74d56dbd034e72001d379fb314700
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-456 Implement AUTO FLUSH BACKGROUND flush mode

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
..


Patch Set 19:

(17 comments)

Hi Alexey,

went through this patch more from knowledge point of view, so feel free to 
ignore any review comments you find not useful. Also I haven't completely 
digested all the routines in sesion-internal.cc, so I may as well take a second 
pass tomorrow later if time permits.

http://gerrit.cloudera.org:8080/#/c/3952/18/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:

PS18, Line 522: we 
Does it make sense to add a coment that callers are expected to serialize 
during Add ?


http://gerrit.cloudera.org:8080/#/c/3952/16/src/kudu/client/batcher.h
File src/kudu/client/batcher.h:

Line 168: 
typo: the the


PS16, Line 169: ck
s/is/are/


http://gerrit.cloudera.org:8080/#/c/3952/17/src/kudu/client/batcher.h
File src/kudu/client/batcher.h:

PS17, Line 122: int64_t
Naive doubt here: If Load() is returning a non-atomic value we seem to be 
accessing the atomic value of buffer_bytes_used_ in write path vs non-atomic in 
read path ?


http://gerrit.cloudera.org:8080/#/c/3952/18/src/kudu/client/client.cc
File src/kudu/client/client.cc:

Line 692:   : data_(new KuduSession::Data(client, client->data_->messenger_)) {
I haven't looked at how data_ being set, just wondering if data_ could be NULL 
since it seems to be a regular ptr.


PS18, Line 736: int
uint ?


PS18, Line 740: int
uint ?


http://gerrit.cloudera.org:8080/#/c/3952/16/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 1017:   /// Modes of flush operations.
Is there a default mode of op here ? if so, can we mention it in comment ?


Line 1038: /// batch is sent and the reclamed space is available for new 
operations.
typo: reclamed


Line 1040: /// @todo Provide an API for the user to specify a callback to 
do their own
If this is not specific to this mode alone, we could move this todo in the 
beginning perhaps ?


Line 1044: ///   (probably the messenger IO threads?).
I guess this todo is already addressed ?


http://gerrit.cloudera.org:8080/#/c/3952/18/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:

PS18, Line 448:   
Nice !! Liked these ascii diagrams.


http://gerrit.cloudera.org:8080/#/c/3952/19/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:

PS19, Line 233: weak_session
any significance of usage of 'weak' session ? :)


PS19, Line 263: (
Isn't this redundant ? We could as well place NextBatcher above ?
do  {
   NextBatcher();
  CanApplyWriteOp(_pre_flush);
} while (need_pre_flush)


PS19, Line 270: lock_
I thought whole routine is serialized, so why another spinlock here ?


http://gerrit.cloudera.org:8080/#/c/3952/18/src/kudu/client/session-internal.h
File src/kudu/client/session-internal.h:

PS18, Line 126: to
s/to/for ?


PS18, Line 165: cond_mutex_
Trivial nit: I suggest to rename this to flow_control_cond_mutex_ which could 
be self explanatory(and remove al the comment associated). Just a suggestion, 
feel free to ignore this.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-1500: Data race in 
RaftConsensusITest.TestCorruptReplicaMetadata
..

KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

The test intends to corrupt the metadata of one of the tserver tablets.
While the cluster is in the process of resurrecting the failed tserver,
the Partition and PartitionSchema on that tserver is accessed using
TabletPeer in an unguarded manner via ListTablets RPCs.

The proposed fix here keeps the Partition and PartitionSchema immutable
once after it is loaded during replica bootstrap. These fields are never
overwritten again during Tablet Copy or any other workflow.
Also a unit test is added to exercise this data race window. i.e,
the unit test aims to issue ListTablets RPCs during a follower's
tablet copy stage and provides an optimistic coverage for the fix.

Testing: Passed about 2000 iteration of the failing test
raft_consensus-itest.TestCorruptReplicaMetadata

Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
---
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.cc
3 files changed, 139 insertions(+), 16 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC

[Dinesh Bhat (Code Review)]

Hello Mike Percy, Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: KUDU-1534 : Added software_version to ListMasters RPC
..

KUDU-1534 : Added software_version to ListMasters RPC

This change also consolidates TSRegistrationPB and
ServerRegistrationPB and merges them into latter
as they seem to bear the same signature.

Sample output is at:
https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf

Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c
---
M src/kudu/common/wire_protocol.proto
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M src/kudu/master/ts_manager.cc
M src/kudu/master/ts_manager.h
M src/kudu/tserver/heartbeater.cc
17 files changed, 65 insertions(+), 58 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1534 : Added software_version to ListMasters RPC
..


Patch Set 4:

(14 comments)

TFTR Adar/Alexey, updated the patch after addressing rev comments.

http://gerrit.cloudera.org:8080/#/c/4099/1//COMMIT_MSG
Commit Message:

PS1, Line 9: This change also consolidates TSRegistrationPB and
> nit: I think the link to the Web server's sample output might be specified 
This was more or less to assist the review, as I doubt anyone would be 
interested in following that link via commit-log in the git history.


http://gerrit.cloudera.org:8080/#/c/4099/3//COMMIT_MSG
Commit Message:

Line 7: KUDU-1534 : Added software_version to ListMasters RPC
> Maybe add a note below about the cleanup you did?
Done


http://gerrit.cloudera.org:8080/#/c/4099/1/src/kudu/common/wire_protocol.proto
File src/kudu/common/wire_protocol.proto:

Line 82: // This entails RPC/HTTP addresses and software version on
> nit: and its version (optional).
That is correct, it looks like you accessed an older diffs Alexey, comments 
were updated in newer diffs which I think addresses this rev comment already.


http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/common/wire_protocol.proto
File src/kudu/common/wire_protocol.proto:

PS3, Line 82: // This entails RPC/HTTP addresses and software version on
: // the servers and used in the registation/heartbeat phase.
: message ServerRegistrationPB {
> The new comment describes how ServerRegistrationPB is used instead of what 
Tweaked to some extent, pls check again.


http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/integration-tests/cluster_itest_util.h
File src/kudu/integration-tests/cluster_itest_util.h:

Line 78:   gscoped_ptr tserver_proxy;
> Hmm, how does the forward declaration help with this? The layout of ServerR
Good catch, I overlooked that this was an instantiation.


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

PS1, Line 316: Registration
> I think the output does not look nice for rendering in a single table row i
Hmm, I thought about that too, but here I tried to be consistent with 
tablet-servers page output. They follow exact same format like this.


PS1, Line 333: R
> Nit: an extra space.  Is it really needed?
Fixed.


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

PS3, Line 68: tml(const ServerRegi
> Does this type even exist? Does this method still need to be templated?
Ha ! No, it was a blind text find/replace I did both in this line and above 
forward decl. Fixed both.


http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:

Line 54: class ServerRegistrationPB;
> Again, not seeing how this helps, given that to declare a ServerRegistratio
Seriously, I guess I was simply adding this to all headers just to see if 
compiles smoothly, but later forgot to clean them up. Thanks for noticing them.


PS3, Line 72: 
: // Create a client proxy to it.
: MessengerBuilder bld("Client");
> This seems like an odd place to stash a test. Why not in its own TEST_F? Al
I thought since ServerRegistrationPB was kinda stuffed in 'registration' as a 
mandatory field(though version is optional), we could let every Setup() go 
through this check. I see your point, I created a simplest test possible under 
registration-test to check version now.


http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/master/master.h
File src/kudu/master/master.h:

Line 33: #include "kudu/util/status.h"
> Nit: sort alphabetically.
Done.


Line 41: 
> I don't think this helps given L135.
Fixed.


http://gerrit.cloudera.org:8080/#/c/4099/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:

Line 234: 
> I wonder if this is backwards compatible. The message types are clearly dif
Good point, will check them out, so don't +2 on this yet pending this test 
result.


Line 555:   // Node instance information is always set.
> Nit: is this comment still relevant? Looks like we're providing a ServerReg
I removed it, don't actually know what was it conveying there.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC

[Dinesh Bhat (Code Review)]

Hello Mike Percy, Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: KUDU-1534 : Added software_version to ListMasters RPC
..

KUDU-1534 : Added software_version to ListMasters RPC

This change also consolidates TSRegistrationPB and
ServerRegistrationPB and merges them into latter
as they seem to bear the same signature.

Sample output is at:
https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf

Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c
---
M src/kudu/common/wire_protocol.proto
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M src/kudu/master/ts_manager.cc
M src/kudu/master/ts_manager.h
M src/kudu/tserver/heartbeater.cc
17 files changed, 63 insertions(+), 58 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tool: port log-dump

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: tool: port log-dump
..


Patch Set 3:

(4 comments)

Hi Adar, sorry I missed the train here, but these are more of curious questions 
than review comments as such so pls feel free to answer in your spare time.

http://gerrit.cloudera.org:8080/#/c/4167/3/docs/release_notes.adoc
File docs/release_notes.adoc:

Line 53:   implemented as `kudu wal dump` and `kudu tablet dump_wals`.
aha, nice to see this.. will add it in my change too.


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

PS3, Line 92: out
Some basic paranoia Qn unrelated to your change: The string object itself seems 
to be on stack, I haven't looked into '=' overloading part of 'string',  but I 
wonder if out happens to carry all std::cout emitted from subprocess here and 
whether we ever need to worry about ulimit -s ?


Line 369:   const Schema kSchemaWithIds(SchemaBuilder(kSchema).Build());
I am curious to know the purpose of SchemaBuilder here ? Can't we directly use 
kSchema while opening log ?


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

PS3, Line 265: AddOptionalParameter
I have been wondering about this while testing today: isn't fs_wal_dir supposed 
to be a required parameter for dump actions ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: 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 (WIP)

[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 log-dump

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: tool: port log-dump
..


Patch Set 3:

> (3 comments)

Thank you for the responses here.

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

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

[kudu-CR] tool: port kudu-admin to 'kudu cluster'

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-admin to 'kudu cluster'
..


Patch Set 4:

(6 comments)

Consider +1 from me, except one Qn about REMOVE_SERVER below.

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

Line 293:   unique_ptr add_server =
> But they are already alphabetically sorted, aren't they? Or do you mean I s
I missed that it's already sorted in BuildHelp, never mind then.


http://gerrit.cloudera.org:8080/#/c/4180/4/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

Line 91: const char* const kTabletIdArg = "tablet_id";
nice, i can consolidate in kudu-test-tool as well like this, but why not const 
string ?


PS4, Line 117: tablet_raw
Bear with a basic Qn: what prevents us from using unique_ptr directly instead 
of using raw pointer first and then re-assigning ?


PS4, Line 177: kudu
some const string again ? or GetBinName kinda routine if available ?


Line 204:   return ChangeConfig(context, consensus::REMOVE_SERVER);
I don't see you taking care of REMOVE_SERVER anywhere in ChangeConfig. In fact 
original impl also did not seem to care about it. I am missing something prolly.


PS4, Line 303: VOTER
This need not have been explicit if there are only two types - i.e, if the 
parameter did not specify VOTER, it means peer is non-voter. But again, this 
also means it is not flexible to accommodate more 'type' in future, so this is 
fine.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e96fc5b0106946f29a2faee8e2667be738b740
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tool: port kudu-admin to 'kudu cluster'

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-admin to 'kudu cluster'
..


Patch Set 4: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4180/4/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

PS4, Line 117: tablet_raw
> We can't do that because the C++ client API should not force applications t
Very cool, thank you for making that clear.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e96fc5b0106946f29a2faee8e2667be738b740
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tool: port kudu-admin to 'kudu cluster'

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-admin to 'kudu cluster'
..


Patch Set 4:

(3 comments)

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

Line 293:   unique_ptr add_server =
> There's no sorting in Action::BuildHelp() or Mode::BuildHelp(). But aren't 
Ok, although I suggested the ordering earlier, my second thoughts were like - 
really they are variable names after all, and no point in alphabetically 
sorting them in the source. Help display is the one which matters I guess.


http://gerrit.cloudera.org:8080/#/c/4180/4/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

PS4, Line 117: tablet_raw
> I'm not aware of a way to "offer" the internal pointer stored by the unique
Hmmm, I saw your other patch here is addressing the actual method 
https://gerrit.cloudera.org/#/c/4179/
My Qn was why couldn't we change the method to accept unique_ptr* 
tablet_out ? Apart from the basic ownership differences, I am very much in the 
dark what should be used where TBH.


Line 204:   return ChangeConfig(context, consensus::REMOVE_SERVER);
> Check out L186: the cc_type is stored directly in the ChangeConfigRequestPB
Got it, thanks.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e96fc5b0106946f29a2faee8e2667be738b740
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

[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)

[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] Fix kudu-ts-cli crash when there is no data in tablet

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Todd Lipcon, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: Fix kudu-ts-cli crash when there is no data in tablet
..

Fix kudu-ts-cli crash when there is no data in tablet

NULL pointer was being sent in as an argument to
RowwiseRowBlockPB::Swap routine, fixing the callsite here.

./bin/kudu-ts-cli dump_tablet f32f6e0b1f354643b6b030599a359fa6 
--server_address=127.108.26.1:33958
previously segfaulted to:

$0  0x009c9867 in std::swap (__a=@0x7ffc75c76af8, __b=@0x18) at 
/opt/rh/devtoolset-3/root/usr/include/c++/4.9.2/bits/move.h:176
$1  0x00b5bf1f in kudu::RowwiseRowBlockPB::Swap (this=0x7ffc75c76ae0, 
other=0x0) at 
/data/10/dinesh/kudu/build/debug/src/kudu/common/wire_protocol.pb.cc:1916
$2  0x0085de72 in kudu::client::KuduScanBatch::Data::Reset 
(this=0x7ffc75c76a80, controller=0x7ffc75c76a20, projection=0x7ffc75c76b40, 
client_projection=0x7ffc75c76ca0, data=...) at 
/data/10/dinesh/kudu/src/kudu/client/scanner-internal.cc:484
$3  0x0084d5d9 in kudu::tools::TsAdminClient::DumpTablet 
(this=0x7ffc75c76e20, tablet_id="1f7a6bd64c604f7fada909bd575708cd") at 
/data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:278
$4  0x0084f4bd in kudu::tools::TsCliMain (argc=3, argv=0x7ffc75c773f0) 
at /data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:455
$5  0x00850079 in main (argc=4, argv=0x7ffc75c773e8) at 
/data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:492

Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
---
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/tools/ts-cli.cc
2 files changed, 67 insertions(+), 32 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Fix kudu-ts-cli crash when there is no data in tablet

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Todd Lipcon, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: Fix kudu-ts-cli crash when there is no data in tablet
..

Fix kudu-ts-cli crash when there is no data in tablet

NULL pointer was being sent in as an argument to
RowwiseRowBlockPB::Swap routine, fixing the callsite here.

./bin/kudu-ts-cli dump_tablet f32f6e0b1f354643b6b030599a359fa6 
--server_address=127.108.26.1:33958
previously segfaulted to:

$0  0x009c9867 in std::swap (__a=@0x7ffc75c76af8, __b=@0x18) at 
/opt/rh/devtoolset-3/root/usr/include/c++/4.9.2/bits/move.h:176
$1  0x00b5bf1f in kudu::RowwiseRowBlockPB::Swap (this=0x7ffc75c76ae0, 
other=0x0) at 
/data/10/dinesh/kudu/build/debug/src/kudu/common/wire_protocol.pb.cc:1916
$2  0x0085de72 in kudu::client::KuduScanBatch::Data::Reset 
(this=0x7ffc75c76a80, controller=0x7ffc75c76a20, projection=0x7ffc75c76b40, 
client_projection=0x7ffc75c76ca0, data=...) at 
/data/10/dinesh/kudu/src/kudu/client/scanner-internal.cc:484
$3  0x0084d5d9 in kudu::tools::TsAdminClient::DumpTablet 
(this=0x7ffc75c76e20, tablet_id="1f7a6bd64c604f7fada909bd575708cd") at 
/data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:278
$4  0x0084f4bd in kudu::tools::TsCliMain (argc=3, argv=0x7ffc75c773f0) 
at /data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:455
$5  0x00850079 in main (argc=4, argv=0x7ffc75c773e8) at 
/data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:492

Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
---
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/tools/ts-cli.cc
2 files changed, 86 insertions(+), 31 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Fix kudu-ts-cli crash when there is no data in tablet

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Todd Lipcon, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: Fix kudu-ts-cli crash when there is no data in tablet
..

Fix kudu-ts-cli crash when there is no data in tablet

NULL pointer was being sent in as an argument to
RowwiseRowBlockPB::Swap routine, fixing the callsite here.

./bin/kudu-ts-cli dump_tablet f32f6e0b1f354643b6b030599a359fa6 
--server_address=127.108.26.1:33958
previously segfaulted to:

$0  0x009c9867 in std::swap (__a=@0x7ffc75c76af8, __b=@0x18) at 
/opt/rh/devtoolset-3/root/usr/include/c++/4.9.2/bits/move.h:176
$1  0x00b5bf1f in kudu::RowwiseRowBlockPB::Swap (this=0x7ffc75c76ae0, 
other=0x0) at 
/data/10/dinesh/kudu/build/debug/src/kudu/common/wire_protocol.pb.cc:1916
$2  0x0085de72 in kudu::client::KuduScanBatch::Data::Reset 
(this=0x7ffc75c76a80, controller=0x7ffc75c76a20, projection=0x7ffc75c76b40, 
client_projection=0x7ffc75c76ca0, data=...) at 
/data/10/dinesh/kudu/src/kudu/client/scanner-internal.cc:484
$3  0x0084d5d9 in kudu::tools::TsAdminClient::DumpTablet 
(this=0x7ffc75c76e20, tablet_id="1f7a6bd64c604f7fada909bd575708cd") at 
/data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:278
$4  0x0084f4bd in kudu::tools::TsCliMain (argc=3, argv=0x7ffc75c773f0) 
at /data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:455
$5  0x00850079 in main (argc=4, argv=0x7ffc75c773e8) at 
/data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:492

Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
---
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/tools/ts-cli.cc
2 files changed, 92 insertions(+), 31 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Fix kudu-ts-cli crash when there is no data in tablet

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: Fix kudu-ts-cli crash when there is no data in tablet
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4134/10/src/kudu/tools/kudu-ts-cli-test.cc
File src/kudu/tools/kudu-ts-cli-test.cc:

PS10, Line 146:   vector rows = strings::Split(out, "\n", 
strings::SkipEmpty());
  :   for (const auto& row : rows) {
> Nit: combine onto one line.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
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 (WIP)

[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] Fix kudu-ts-cli crash when there is no data in tablet

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Todd Lipcon, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: Fix kudu-ts-cli crash when there is no data in tablet
..

Fix kudu-ts-cli crash when there is no data in tablet

NULL pointer was being sent in as an argument to
RowwiseRowBlockPB::Swap routine, fixing the callsite here.

./bin/kudu-ts-cli dump_tablet f32f6e0b1f354643b6b030599a359fa6 
--server_address=127.108.26.1:33958
previously segfaulted to:

$0  0x009c9867 in std::swap (__a=@0x7ffc75c76af8, __b=@0x18) at 
/opt/rh/devtoolset-3/root/usr/include/c++/4.9.2/bits/move.h:176
$1  0x00b5bf1f in kudu::RowwiseRowBlockPB::Swap (this=0x7ffc75c76ae0, 
other=0x0) at 
/data/10/dinesh/kudu/build/debug/src/kudu/common/wire_protocol.pb.cc:1916
$2  0x0085de72 in kudu::client::KuduScanBatch::Data::Reset 
(this=0x7ffc75c76a80, controller=0x7ffc75c76a20, projection=0x7ffc75c76b40, 
client_projection=0x7ffc75c76ca0, data=...) at 
/data/10/dinesh/kudu/src/kudu/client/scanner-internal.cc:484
$3  0x0084d5d9 in kudu::tools::TsAdminClient::DumpTablet 
(this=0x7ffc75c76e20, tablet_id="1f7a6bd64c604f7fada909bd575708cd") at 
/data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:278
$4  0x0084f4bd in kudu::tools::TsCliMain (argc=3, argv=0x7ffc75c773f0) 
at /data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:455
$5  0x00850079 in main (argc=4, argv=0x7ffc75c773e8) at 
/data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:492

Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
---
M src/kudu/tools/ts-cli.cc
1 file changed, 17 insertions(+), 16 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Fix kudu-ts-cli crash when there is no data in tablet

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Todd Lipcon, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: Fix kudu-ts-cli crash when there is no data in tablet
..

Fix kudu-ts-cli crash when there is no data in tablet

NULL pointer was being sent in as an argument to
RowwiseRowBlockPB::Swap routine, fixing the callsite here.

./bin/kudu-ts-cli dump_tablet f32f6e0b1f354643b6b030599a359fa6 
--server_address=127.108.26.1:33958
previously segfaulted to:

$0  0x009c9867 in std::swap (__a=@0x7ffc75c76af8, __b=@0x18) at 
/opt/rh/devtoolset-3/root/usr/include/c++/4.9.2/bits/move.h:176
$1  0x00b5bf1f in kudu::RowwiseRowBlockPB::Swap (this=0x7ffc75c76ae0, 
other=0x0) at 
/data/10/dinesh/kudu/build/debug/src/kudu/common/wire_protocol.pb.cc:1916
$2  0x0085de72 in kudu::client::KuduScanBatch::Data::Reset 
(this=0x7ffc75c76a80, controller=0x7ffc75c76a20, projection=0x7ffc75c76b40, 
client_projection=0x7ffc75c76ca0, data=...) at 
/data/10/dinesh/kudu/src/kudu/client/scanner-internal.cc:484
$3  0x0084d5d9 in kudu::tools::TsAdminClient::DumpTablet 
(this=0x7ffc75c76e20, tablet_id="1f7a6bd64c604f7fada909bd575708cd") at 
/data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:278
$4  0x0084f4bd in kudu::tools::TsCliMain (argc=3, argv=0x7ffc75c773f0) 
at /data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:455
$5  0x00850079 in main (argc=4, argv=0x7ffc75c773e8) at 
/data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:492

Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
---
M src/kudu/tools/ts-cli.cc
1 file changed, 8 insertions(+), 7 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Fix kudu-ts-cli crash when there is no data in tablet

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: Fix kudu-ts-cli crash when there is no data in tablet
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4134/2/src/kudu/tools/ts-cli.cc
File src/kudu/tools/ts-cli.cc:

Line 278:   req.set_scanner_id(resp.scanner_id());
> yea, even without filters, you could have a bunch of deleted rows, and it's
Got it, thanks, also updated the scan to 'continue' instead of break. I will  
pump some small data into tablet to check this patch doesn't break the regular 
usage of dump_tablet.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Mike Percy, Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-1500: Data race in 
RaftConsensusITest.TestCorruptReplicaMetadata
..

KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

The test intends to corrupt the metadata of one of the tserver tablets.
While the cluster is in the process of resurrecting the failed tserver,
the Partition and PartitionSchema on that tserver is accessed using
TabletPeer in an unguarded manner via ListTablets RPCs.

The proposed fix here keeps the Partition and PartitionSchema immutable
after it is loaded for the very first time. These fields are never
overwritten again during tablet-copy or any other workflow.
Also a unit test is added to exercise this data race window. i.e,
the unit test aims to issue ListTablets RPCs during a follower's
tablet copy stage and provides an optimistic coverage for the fix.
DCHECKs are added to make sure the oncoming protobuf fields
match the immutable fields not updated during tablet-copy.

Testing: Spun a test TabletCopyITest.TestDeleteTabletDuringTabletCopy
which exercises this data race window, also ran original test which
caught the race in raft_consensus-itest.TestCorruptReplicaMetadata.

Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
---
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.cc
4 files changed, 129 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/3823/17
-- 
To view, visit http://gerrit.cloudera.org:8080/3823
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1500: Data race in 
RaftConsensusITest.TestCorruptReplicaMetadata
..


Patch Set 17:

(2 comments)

TFTRs Mike/Dan/Adar, updated the patch to reflect the discussions we have had 
on this. Please take a look.

http://gerrit.cloudera.org:8080/#/c/3823/15/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:

PS15, Line 313:   CHECK_EQ(table_id_, superblock.table_id());
  :   PartitionSchema partition_schema;
  :   
RETURN_NOT_OK(PartitionSchema::FromPB(superblock.partition_schema(),
  : *schema_, 
_schema));
  :   CHECK(partition_schema_.Equals(partition_schema));
  : 
  :   Partition partition;
  :   Partition::FromPB(superblock.partition(), );
  :   CHECK(partition_.Equals(partition));
> Providing equals on partition should be easy, it just needs to check that t
also checked for hash_buckets equality Dan, assuming that's immutable as well.


PS15, Line 313:   CHECK_EQ(table_id_, superblock.table_id());
  :   PartitionSchema partition_schema;
  :   
RETURN_NOT_OK(PartitionSchema::FromPB(superblock.partition_schema(),
  : *schema_, 
_schema));
  :   CHECK(partition_schema_.Equals(partition_schema));
  : 
  :   Partition partition;
  :   Partition::FromPB(superblock.partition(), );
  :   CHECK(partition_.Equals(partition));
> I suppose we could change this to CHECK and just rely on upgrade / downgrad
Cool, thanks Mike, updated the code with Equals() for both partition and 
partition_schema, and moving to CHECK to keep the code same for all build 
types. I agree with catching the compat issues with white/black box automated 
tests for upgrade/downgrade.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Fix kudu-ts-cli crash when there is no data in tablet

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Todd Lipcon, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: Fix kudu-ts-cli crash when there is no data in tablet
..

Fix kudu-ts-cli crash when there is no data in tablet

NULL pointer was being sent in as an argument to
RowwiseRowBlockPB::Swap routine, fixing the callsite here.

./bin/kudu-ts-cli dump_tablet f32f6e0b1f354643b6b030599a359fa6 
--server_address=127.108.26.1:33958
previously segfaulted to:

$0  0x009c9867 in std::swap (__a=@0x7ffc75c76af8, __b=@0x18) at 
/opt/rh/devtoolset-3/root/usr/include/c++/4.9.2/bits/move.h:176
$1  0x00b5bf1f in kudu::RowwiseRowBlockPB::Swap (this=0x7ffc75c76ae0, 
other=0x0) at 
/data/10/dinesh/kudu/build/debug/src/kudu/common/wire_protocol.pb.cc:1916
$2  0x0085de72 in kudu::client::KuduScanBatch::Data::Reset 
(this=0x7ffc75c76a80, controller=0x7ffc75c76a20, projection=0x7ffc75c76b40, 
client_projection=0x7ffc75c76ca0, data=...) at 
/data/10/dinesh/kudu/src/kudu/client/scanner-internal.cc:484
$3  0x0084d5d9 in kudu::tools::TsAdminClient::DumpTablet 
(this=0x7ffc75c76e20, tablet_id="1f7a6bd64c604f7fada909bd575708cd") at 
/data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:278
$4  0x0084f4bd in kudu::tools::TsCliMain (argc=3, argv=0x7ffc75c773f0) 
at /data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:455
$5  0x00850079 in main (argc=4, argv=0x7ffc75c773e8) at 
/data/10/dinesh/kudu/src/kudu/tools/ts-cli.cc:492

Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
---
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/tools/ts-cli.cc
2 files changed, 48 insertions(+), 20 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Fix kudu-ts-cli crash when there is no data in tablet

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: Fix kudu-ts-cli crash when there is no data in tablet
..


Patch Set 6:

> Code change looks good, but how about a test in kudu-ts-cli-test?

Yess, thought about same after updating diffs; small piece of code lets me test 
this path, however I am wondering how can we output a small dump_tablet output 
to the test logfile(instead of usual stdout):

  string exe_path = GetTsCliToolPath();
  vector argv;
  argv.push_back(exe_path);
  argv.push_back("--server_address");
  argv.push_back(cluster_->tablet_server(0)->bound_rpc_addr().ToString());
  argv.push_back("dump_tablet");
  argv.push_back(tablet_id);
  ASSERT_OK(Subprocess::Call(argv));

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1500: Data race in 
RaftConsensusITest.TestCorruptReplicaMetadata
..


Patch Set 17:

> Do you guys think we should pull this in for 1.0 even though we
 > don't have an upgrade test yet? Maybe it's worth filing a JIRA and
 > committing this, since I think Adar saw it in a test environment.

I think so, given that this change doesn't break forward compat(i.e, 0.10 
should be able to accept remote-copy from 1.0). If we intro partition changes 
in 1.0, we anyways need to handle backward-compat in the future code. I had 
filed JIRA for upgrade/downgrade test.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

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

[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, 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 

[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

[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

[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, 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

[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)]

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

[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

[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 ts-cli

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: tool: port ts-cli
..


Patch Set 3:

(3 comments)

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

Line 265:   builder.default_rpc_timeout(GetDefaultTimeout());
is this worth mentioning in commit-message or just a cleanup ?


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

PS3, Line 69: Change a gflag value on a Kudu Tablet Server
Extending this with an example flag could be useful.


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

Line 117: .AddMode(BuildMasterMode())
We could have made 'tablet' as a sub-mode of this actually given that the 
consensus config change takes master_addresses. Something like 'kudu master 
change_config '. I think we can tackle this later, but I am somewhat 
not so fan of intro'ing many top level modes.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb5a59fd690c2dd09e4e76858469d81f9d501371
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
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 (#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-admin to 'kudu table' and 'kudu tablet'

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-admin to 'kudu table' and 'kudu tablet'
..


Patch Set 6: Code-Review+1

(3 comments)

LGTM, few nits and Qs as usual from my side.

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

Line 240: "peers", "List of peers where each peer is of form 
'uuid:hostname:port'" })
at this point, user may want to know where can he grab this uuid from ?  this 
was the reason i retained 'list_tablets' kinda functionality from fs_list 
because it means user doesn't have to rely on any other output outside 
/bin/kudu. well in this case, i think he can grab this uuid from kudu ksck ?


http://gerrit.cloudera.org:8080/#/c/4180/6/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:

Line 101:   return ModeBuilder("table")
Come to think of it, I guess these are nothing but operations on the 
cluster(although table may not span an entire cluster), but I was thinking if 
it makes sense to keep less number of Modes under bin/kudu and more subModes 
under cluster. Just some thoughts, we can do this for post 1.0 if we have 
second thoughts later.


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

Line 185:   .Description("Add a new replica to a tablet's Raft 
configuration")
I am not sure if we want to stay away from using 'Raft config' here, we could 
just stick to 'add a new replica to tablet'. Nothing wrong in it, but just 
feels like internal detail.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e96fc5b0106946f29a2faee8e2667be738b740
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
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 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

[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] [tools] Implement a manual leader step down for a tablet

[Dinesh Bhat (Code Review)]

Hello David Ribeiro Alves, Todd Lipcon,

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

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

to review the following change.

Change subject: [tools] Implement a manual leader_step_down for a tablet
..

[tools] Implement a manual leader_step_down for a tablet

This change introduces a leader_step_down functionality
under 'kudu tablet'. This tool may be handy to recover from
situations when a single tablet server is overloaded and we want
to kick off  a new election to balance the load across the clusters.
Although it is not guaranteed that a different replica will be elected
as the leader, this is an optimistic effort to elect a new tablet
server as the leader for the given tablet in the cluster.

Also snuck in a small change to display host:port details with
'kudu tablet list  --list_tablets' command.

Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
3 files changed, 185 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above

[Dinesh Bhat (Code Review)]

Dinesh Bhat has abandoned this change.

Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
..


Abandoned

Since we decided to keep the INFO level logs with these tools, this patch is 
not relevant anymore. Idea is to be as much informative in the output as 
possible when using these tools.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [tools] Implement a manual leader step down for a tablet

[Dinesh Bhat (Code Review)]

Hello David Ribeiro Alves, Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: [tools] Implement a manual leader_step_down for a tablet
..

[tools] Implement a manual leader_step_down for a tablet

This change introduces a leader_step_down functionality
under 'kudu tablet'. This tool may be handy to recover from
situations when a single tablet server is overloaded and we want
to kick off a new election to balance the load across the clusters.
Although it is not guaranteed that a different replica will be elected
as the leader, this is an optimistic effort to elect a new tablet
server as the leader for the given tablet in the cluster.

Also snuck in a small change to display host:port details with
'kudu table list  --list_tablets' command.

Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
5 files changed, 187 insertions(+), 44 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [tools] Implement a manual leader step down for a tablet

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [tools] Implement a manual leader_step_down for a tablet
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4533/1/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

Line 218:   tablet_id_
> But why not get the leader info from the consensus info too? Why go to two 
One reason I was hesitant to use the same routine for leadership info was 
because GetConsensus collects the details from all the tservers, and I wasn't 
sure whether one of them could end up returning a stale info depending on when 
the RPC hits them. In the code below at L234, probability of that RPC hitting 
when the leader failover is in transition is very likely. If anything, this was 
mainly due to not knowing the codebase well and staying on safer side(go to 
master for leader info).


PS1, Line 230: "leader_failure_max_missed_heartbeat_periods",
 : _missed_hb_periods_str));
> Now I see what you mean; the number of attempts in AssertEventually is neve
Well we don't want to include the  under AssertEventually, 
because it's perfectly possible that even after specified timeout, we never 
elected a leader which is different than current_leader. ASSERT_NE(new_leader, 
current_leader) could yield us incorrect results. I will still debug what I 
observed is just conincidence or there is more to it than just meet the eye. 
However, the ++attempts kinda ensures that eventually new_leader emerges on the 
other side.


http://gerrit.cloudera.org:8080/#/c/4533/3/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

PS3, Line 221: // Grab the default settings for heartbeat timeouts.
 : string hb_timeout_str;
 : 
ASSERT_TRUE(google::GetCommandLineOption("raft_heartbeat_interval_ms",
 :  _timeout_str));
 : int32 hb_timeout;
 : ASSERT_TRUE(safe_strto32(hb_timeout_str, _timeout));
 : string max_missed_hb_periods_str;
 : ASSERT_TRUE(
 : google::GetCommandLineOption(
 : "leader_failure_max_missed_heartbeat_periods",
 : _missed_hb_periods_str));
 : double max_missed_hb_periods;
 : ASSERT_TRUE(safe_strtod(max_missed_hb_periods_str,
 : _missed_hb_periods));
> You don't need to go through all this trouble (and you certainly don't need
Good point, forgot about DECLARE, done.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4551/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

PS2, Line 499: en to by another lbm, we must reinspect th
> Compaction writes new blocks to the container, and old ones are hole punche
Great, thanks.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [util] Fix a minor bug in AssertEventually()

[Dinesh Bhat (Code Review)]

Hello Mike Percy, Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: [util] Fix a minor bug in AssertEventually()
..

[util] Fix a minor bug in AssertEventually()

We want to be sleeping with a monotonically increasing timeout
value in this routine. After 10 attempts, we sleep
for 1000 ms(constant value). But this routine was never
increasing the 'attempts' thus falling back to sleep always at 1ms
intervals making it inefficient.

Also snuck in a change to remove a warning stemming from fresh builds.

Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
---
M src/kudu/common/column_predicate.cc
M src/kudu/util/test_util.cc
2 files changed, 3 insertions(+), 2 deletions(-)


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

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

[kudu-CR] KUDU-1674: Fix SubProcess:Call SEGV when trying to capture stderr alone

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1674: Fix SubProcess:Call SEGV when trying to capture 
stderr alone
..


Patch Set 3:

(5 comments)

TFTR Alexey,

http://gerrit.cloudera.org:8080/#/c/4594/3/src/kudu/util/subprocess-test.cc
File src/kudu/util/subprocess-test.cc:

Line 27: #include "kudu/gutil/strings/numbers.h"
> Is this necessary for the updated version?
Done


PS3, Line 138: string
> Nit: consider 'const string str = ...' or 'static const string str = ...' j
Done


PS3, Line 140:   Status s = Subprocess::Call({"/bin/sh", "-c", cmd_str}, 
nullptr, );
 :   ASSERT_TRUE(s.ok());
> Nit: since 's' is used only for ASSERT_TRUE(s.ok()) check, consider replaci
Done


PS3, Line 146:   s = Subprocess::Call({"/bin/ls", "/dev/null"}, , 
nullptr);
 :   ASSERT_TRUE(s.ok());
> Ditto for merging these two strings into ASSERT_OK(...)
Done


PS3, Line 150:   s = Subprocess::Call({"/bin/ls", "/dev/zero"}, nullptr, 
nullptr);
 :   ASSERT_TRUE(s.ok());
> Ditto for merging these two strings into ASSERT_OK(...)
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I67bd462098526a9ba032669b621b8139b56ee5b8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1674: Fix SubProcess:Call SEGV when trying to capture stderr alone

[Dinesh Bhat (Code Review)]

Hello Adar Dembo, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: KUDU-1674: Fix SubProcess:Call SEGV when trying to capture 
stderr alone
..

KUDU-1674: Fix SubProcess:Call SEGV when trying to capture stderr alone

ReadFdsFully() captures either stdout or stderr or both depending on how
caller has registered. Once the helpers are done with capturing, it needs
take into account that there can be less than 2 fds registered from the
caller and hence can not do std::move(vector[1]) operation on it.

Also added a test code to exercise combination of:
SubProcess::Call({argv}, nullptr, );
SubProcess::Call({argv}, , nullptr);
SubProcess::Call({argv}, nullptr, nullptr);

Change-Id: I67bd462098526a9ba032669b621b8139b56ee5b8
---
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
2 files changed, 28 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I67bd462098526a9ba032669b621b8139b56ee5b8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1674: Fix SubProcess:Call SEGV when trying to capture stderr alone

[Dinesh Bhat (Code Review)]

Hello Adar Dembo, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: KUDU-1674: Fix SubProcess:Call SEGV when trying to capture 
stderr alone
..

KUDU-1674: Fix SubProcess:Call SEGV when trying to capture stderr alone

ReadFdsFully() captures either stdout or stderr or both depending on how
caller has registered. Once the helpers are done with capturing, it needs
take into account that there can be less than 2 fds registered from the
caller and hence can not do std::move(vector[1]) operation on it.

Also added a test code to exercise combination of:
SubProcess::Call(argv[0], nullptr, );
SubProcess::Call(argv[0], , nullptr);
SubProcess::Call(argv[0], nullptr, nullptr);

Change-Id: I67bd462098526a9ba032669b621b8139b56ee5b8
---
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
2 files changed, 32 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I67bd462098526a9ba032669b621b8139b56ee5b8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1674: Fix SubProcess:Call SEGV when trying to capture stderr alone

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1674: Fix SubProcess:Call SEGV when trying to capture 
stderr alone
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4594/2/src/kudu/util/subprocess-test.cc
File src/kudu/util/subprocess-test.cc:

PS2, Line 139: SimpleItoa(std::rand())
> I agree with Alexey (and I wrote the same on PS1): generating a random stri
Thanks Adar, agreed on rand() part. I think Alexey & I were discussing a 
slightly different topic earlier; i.e whether or not we want to be checking 
stderr content at all. My argument was we want to because we may have collected 
a garbage in stderr if the program didn't SIGSEGV. I also didn't want to rely 
on an error code which came out as EACCESS via SubProcess::Call("/").  After 
some offline discussions, we updated the stderr to be more deterministic. In 
this case we know certainly:
1) Program has executed via execve(unlike EACCESS case)
2) It has also captured a well-defined string in stderr


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I67bd462098526a9ba032669b621b8139b56ee5b8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [tools] Implement a manual leader step down for a tablet

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [tools] Implement a manual leader_step_down for a tablet
..


Patch Set 13:

(1 comment)

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

PS12, Line 245: to step down
> What if that '(if present)' is dropped?  If it do not provide essential inf
It kinda tells this command is effective only with leader present.  However 
initial inclination was not to compromise on the actual help description to 
satisfy a test case. But command returns gracefully with a string if leader not 
present, so I guess we can drop it.. Updated.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [tools] Implement a manual leader step down for a tablet

[Dinesh Bhat (Code Review)]

Hello Adar Dembo, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: [tools] Implement a manual leader_step_down for a tablet
..

[tools] Implement a manual leader_step_down for a tablet

This change introduces a leader_step_down functionality
under 'kudu tablet'. This tool may be handy to recover from
situations when a single tablet server is overloaded and we want
to kick off a new election to balance the load across the clusters.
Although it is not guaranteed that a different replica will be elected
as the leader, this is an optimistic effort to elect a new tablet
server as the leader for the given tablet in the cluster.

Test: Ran 8000 iterations of leader_step_down test on dist_test.

Also snuck in a small change to display host:port details with
'kudu table list  --list_tablets' command.

Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_remote_replica.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tools/tool_action_test.cc
10 files changed, 190 insertions(+), 66 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [tools] Implement a manual leader step down for a tablet

[Dinesh Bhat (Code Review)]

Hello Adar Dembo, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: [tools] Implement a manual leader_step_down for a tablet
..

[tools] Implement a manual leader_step_down for a tablet

This change introduces a leader_step_down functionality
under 'kudu tablet'. This tool may be handy to recover from
situations when a single tablet server is overloaded and we want
to kick off a new election to balance the load across the clusters.
Although it is not guaranteed that a different replica will be elected
as the leader, this is an optimistic effort to elect a new tablet
server as the leader for the given tablet in the cluster.

Test: Ran 8000 iterations of leader_step_down test on dist_test.

Also snuck in a small change to display host:port details with
'kudu table list  --list_tablets' command.

Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_remote_replica.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tools/tool_action_test.cc
10 files changed, 191 insertions(+), 66 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] delete table-test: fix flakiness with table creation timeout

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: delete_table-test: fix flakiness with table creation timeout
..


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61ad384e1b247f83bfc709528c4c7bda586c9d2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] KUDU-1674: Fix SubProcess:Call SEGV when trying to capture stderr alone

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1674: Fix SubProcess:Call SEGV when trying to capture 
stderr alone
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4594/2/src/kudu/util/subprocess-test.cc
File src/kudu/util/subprocess-test.cc:

PS2, Line 139: SimpleItoa(std::rand())
> I don't think it's worth analyzing the stderr in the context of this test: 
We are better off checking little more than no SIGSEGV here, to make sure we 
don't have garbage stashed in stderr. Previously I had run 50k iterations of 
original patch without hitting anything, but one of the jenkins run had caught 
sigsegv.

I am sure you would get a different error if you are running on a VM with root 
privileges for "/" command. In any case, I can include a simpler version than 
'bash -c' if you feel this is too fancy, but checking for sanity output of 
stderr is important IMHO.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I67bd462098526a9ba032669b621b8139b56ee5b8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
..


Patch Set 2:

(2 comments)

> (2 comments)
 > 
 > Would be nice to get a little coverage of the new path, especially
 > since there was that bug in the first patch which didn't list all
 > the tables. Could you add something to TestListTables in
 > kudu-admin-test? We should test listing of multiple tables, and
 > verify the new output if possible.

Thanks, yeah that alarmed a test for sure. I will upload a new patch with test. 

Also on an ortho topic of making the output to machine-readable by keeping 
loglevels to only WARNING and above: I realized couple of ways to do that is to 
use big hammer FLAG_minloglevel = 2 or 
google::SetCommandLineOption(minloglevel, 2) from code , but that also means we 
are losing the access to all logs below WARNING. Does that mean it's a no-go ? 
Come to think of it, any script wants to consume this output, it could use 
'kudu ... 2>/dev/null' which isn't a bad assumption.

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

Line 641:   .AddOptionalParameter("dump_data")
> Nit: maintain alphabetical order.
Done


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

PS2, Line 34: "Print list of tablets for the table along with replica uuids"
> Nit: how about "Include tablet and replica UUIDs in the output"?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
..


Patch Set 2:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/4440/1//COMMIT_MSG
Commit Message:

PS1, Line 10: correlat
> correlate
Done


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

PS1, Line 67: false,
> Nit: don't need this breadcrumb. I only added it for timeout_ms because tha
Done


Line 178:   unique_ptr cmeta;
> I'd rather we didn't do this; it makes the output less machine-readable. Or
Alrighty, as discussed offline, removed them.


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

PS1, Line 81: cout << tname << endl;
: if (!FLAGS_list_tablets) {
:   continue;
: }
: client::sp::shared_ptr client_table;
: RETURN_NOT_OK(client->OpenTable(tname,
> All this should happen after the verbose check, since its output isn't need
Done


PS1, Line 89: KuduScanTokenBuilder builder(client_table.get());
: RETURN_NOT_OK(builder.Build());
: 
> Won't this cause just the first table to be printed?
Good catch, looks like I was testing with one table too :)


PS1, Line 95: cout << "P" << (replica->is_leader() ? "(L) " : " ")
:  << replica->ts().uuid() << " ";
> To avoid leaking too many Raft implementation details, let's  annotate the 
Done


Line 124:   .AddOptionalParameter("list_tablets")
> Printing extra information during "kudu table list" is useful, but I'd pref
done, i pinged in hipchat before seeing this comment of yours, happy that we 
arrived at same conclusion. i replaced '--list-tablets' or suggest with any 
other choices.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
..

cli tool: List all tablets/replica_uuids with 'kudu table list'

I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to correlate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.

Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 88 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4440/4/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

PS4, Line 251:   ASSERT_STR_CONTAINS(stdout, kTableId);
 :   ASSERT_STR_CONTAINS(stdout, kAnotherT
> Is there a guarantee that the two tables will be printed in this order?
Hmmm, I looked at my test output and another cluster output where I had 4 
tables, and they seem to be ordered alphabetically. However, after looking at 
ListTables/CreateTable service handlers in catalog_manager they are using 
std::unordered_map which doesn't guarantee anything about ordering of the keys 
in the map. For safety purposes, I am removing this assumption from code. 
Thanks for catching that !!


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
..

cli tool: List all tablets/replica_uuids with 'kudu table list'

I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to correlate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.

Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 66 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [tools]: Keep the verbosity of CLI at WARNING and above

[Dinesh Bhat (Code Review)]

Hello David Ribeiro Alves, Adar Dembo, Alexey Serbin,

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

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

to review the following change.

Change subject: [tools]: Keep the verbosity of CLI at WARNING and above
..

[tools]: Keep the verbosity of CLI at WARNING and above

This keeps the verbosity of the 'kudu' commands at WARNING
and above if the user had not specified 'minloglevel' or
'--v' on the command line options. Both stdout and stderr
will suppress INFO level output without having to explicitly
append '2>/dev/null' to cli commands.

Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
---
M src/kudu/tools/tool_main.cc
1 file changed, 9 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 

[kudu-CR] [tools]: Keep the verbosity of CLI at WARNING and above

[Dinesh Bhat (Code Review)]

Hello David Ribeiro Alves, Adar Dembo, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: [tools]: Keep the verbosity of CLI at WARNING and above
..

[tools]: Keep the verbosity of CLI at WARNING and above

This keeps the verbosity of the 'kudu' commands at WARNING
and above if the user had not specified 'minloglevel' or
'--v' on the command line options. Both stdout and stderr
will suppress INFO level output without having to explicitly
append '2>/dev/null' to cli commands.

Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
---
M src/kudu/tools/tool_main.cc
1 file changed, 9 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [misc] : Remove few more warnings

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [misc] : Remove few more warnings
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4518/1/src/kudu/benchmarks/rle.cc
File src/kudu/benchmarks/rle.cc:

PS1, Line 94: r (int
> Ah, I see. We typically use ignore_result() (see gutil/basictypes.h) for th
yep, that works, thanks, updated the patch.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I89b96d52dfed6b38f17cf8cdebeed840fb32f98d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
..


Patch Set 4:

(1 comment)

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

PS1, Line 215: FLAGS_minloglevel = google::GLOG_FATAL;
 :   }
 :   return show_help;
 : }
 : 
 : int main(int argc, char** argv) {
 :   bool show_help = ParseCommandLineFlags(, );
 :   FLAGS_logtostderr = true;
 :   k
> Also, --vmodule affects only the verbose logging stuff, like VLOG(), right 
That's correct. --v controls VLOG(LOG at lowest level, which is INFO), and 
minloglevel controls LOG levels and typically you would want use one of these. 
My understanding is that, consumers of these additional gflags are either 
ops/support or developers. So for these audience, keeping the knobs at more 
granularity(file level) might help in the long run than keeping the loglevel 
knobs at library. My initial intention of this change was to get rid of INFO 
level messages on CLI like "Opened FS with uuid: ", and I am glad it led to 
some interesting discussions on this topic. 

Folks, are we concluding that we should keep the loglevels for CLI at WARNINGS 
and above then ? If the user explicitly specifies a loglevel via --minloglevel 
or --v, then we can honor that. Also, all this impacts to 'kudu' toolset alone. 
We also seem to be converging that we don't want to be under a contract about 
machine-readability of the 'kudu' output to keep the CLI more flexible.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++compilation] fixed 'unused' warnings

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [c++compilation] fixed 'unused' warnings
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4503/1/src/kudu/codegen/row_projector.cc
File src/kudu/codegen/row_projector.cc:

PS1, Line 408: ProjectionsCompatible
Sorry looked at this little late; Looks like this is a non-existant routine 
name now, I will clean this up in my patch if it's okay.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If0a59ef51e5c5ea8be89109a48a57dc5abfde646
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
..


Patch Set 4:

(1 comment)

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

PS1, Line 215: FLAGS_minloglevel = google::GLOG_FATAL;
 :   }
 :   return show_help;
 : }
 : 
 : int main(int argc, char** argv) {
 :   bool show_help = ParseCommandLineFlags(, );
 :   FLAGS_logtostderr = true;
 :   k
> What it we could set up different level of logging for different components
@alexey: currently one of the gflags '--vmodule' lets us achieve what you are 
describing above. http://rpg.ifi.uzh.ch/docs/glog.html#verbose


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
..

cli tool: List all tablets/replica_uuids with 'kudu table list'

I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to correlate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.

Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 89 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Adar Dembo,

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

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

to review the following change.

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
..

cli tool: List all tablets/replica_uuids with 'kudu table list'

I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to correlate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.

This lists tablet/replica uuids irrespective of their states.

Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7

ver 2

Change-Id: Ic2207c5984beaeda6d1dcef700e20f61d1f26e77
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 89 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic2207c5984beaeda6d1dcef700e20f61d1f26e77
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
..

cli tool: List all tablets/replica_uuids with 'kudu table list'

I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to correlate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.

This lists tablet/replica uuids irrespective of their states.

Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 89 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'

[Dinesh Bhat (Code Review)]

Dinesh Bhat has abandoned this change.

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
..


Abandoned

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ic2207c5984beaeda6d1dcef700e20f61d1f26e77
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4440/3/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

Line 192: TEST_F(AdminCliTest, TestListTables) {
> No change here for multiple tables? Or in the new test?
Yeah, I want to make below test populated with mulitple tables. Do you have any 
examples/inputs how to create multiple tables without re-inventing wheel here ? 
The way I see current structure of TabletIntegrationTestBase, it seems to be 
written for single cluster/table scenario. Perhaps I can add another routine to 
that CreateTable(string table-name) ? It prolly also means that we also need 
additional instances of KuduTable and tablet_id_ to track.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above

[Dinesh Bhat (Code Review)]

Hello David Ribeiro Alves, Adar Dembo, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
..

[tools]: Keep the verbosity of CLI at FATAL and above

This keeps the verbosity of the 'kudu' commands at FATAL
if the user had not specified '--minloglevel' or '--v' on the
command line options. Both stdout and stderr will suppress
INFO level output without having to explicitly append '2>/dev/null'
to cli commands.

Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
---
M src/kudu/tools/tool_main.cc
1 file changed, 8 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
..


Patch Set 3:

(1 comment)

Thanks, updated.

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

PS3, Line 215: google::SetCommandLineOption("minloglevel",
 :  
SimpleItoa(google::GLOG_FATAL).c_str());
> Can this just be:
Done, declaration is already available under logging.h


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4440/3/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

Line 192: TEST_F(AdminCliTest, TestListTables) {
> Yeah, I want to make below test populated with mulitple tables. Do you have
Never mind, I was able to create another table using same client handle and 
schema, I will try to wrap this up with mutliple table output.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [tools]: Keep the verbosity of CLI at WARNING and above

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [tools]: Keep the verbosity of CLI at WARNING and above
..


Patch Set 1:

(1 comment)

TFTR Adar, please see below.

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

PS1, Line 215:   // Set the verbosity of the commands to WARNING(2) and above.
 :   // If the user had explicitly specified verbosity, then user's
 :   // verbosity level is honored. Since '--v' depends on 
minloglevel
 :   // specifying either of them on CLI will override this setting.
 :   if 
(google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
 :   google::GetCommandLineFlagInfoOrDie("v").is_default) {
 : google::SetCommandLineOption("minloglevel",
 :  
SimpleItoa(google::GLOG_WARNING).c_str());
 :   }
> Couple of comments:
1. Done.
2. I was wondering if we want to keep the errors visible - for eg, if we run 
cluster ksck on a cluster which is partitioned, some RPC failures 
indicate(although not user-friendly output) that those hosts are not reachable. 
But this makes the output slightly less machine-readable. So keeping the level 
to FATAL for now, and let's decipher the status from the output itself rather 
than from error/warning logs.
3. When you say 'parameters', I am thinking about optional mode params(which 
could be gflags too in our case). But inheriting them means exposing all the 
ugly output of gflags help again, isn't it ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above

[Dinesh Bhat (Code Review)]

Hello David Ribeiro Alves, Adar Dembo, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
..

[tools]: Keep the verbosity of CLI at FATAL and above

This keeps the verbosity of the 'kudu' commands at FATAL
if the user had not specified '--minloglevel' or '--v' on the
command line options. Both stdout and stderr will suppress
INFO level output without having to explicitly append '2>/dev/null'
to cli commands.

Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
---
M src/kudu/tools/tool_main.cc
1 file changed, 9 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] cli tool: add -v to 'kudu table list'

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Adar Dembo,

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

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

to review the following change.

Change subject: cli tool: add -v to 'kudu table list'
..

cli tool: add -v to 'kudu table list'

I noticed that given a tablet or replica_uuid we need to execute
multiple nested commands and also need to corelate tablets and
their replica uuids and their relation to tables. Added a verbose
flag to 'kudu table list' to make this simpler.

This also incorporates few explicit object identifiers on local_replica
outputs, a feedback given by few support folks during training.

Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
---
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 33 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
..


Patch Set 4:

(1 comment)

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

PS1, Line 215: FLAGS_minloglevel = google::GLOG_FATAL;
 :   }
 :   return show_help;
 : }
 : 
 : int main(int argc, char** argv) {
 :   bool show_help = ParseCommandLineFlags(, );
 :   FLAGS_logtostderr = true;
 :   k
> I agree it's a little unusual looking, but given that many of the usages of
One of the motivations of being less verbose here was to keep the output more 
machine-readable(I should update that in commit-message). Also binaries 
retrying on those RPC errors are difficult to apprehend for folks other than 
developers. Given that this tool may be used by support folks as well, we were 
inclining to go with default=quiet as opposed to default=verbose mode.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] cli tool: List all tablets/replica uuids with 'kudu table list'

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
..


Patch Set 4:

(1 comment)

TFTR Adar, updated the patch with new test added.

http://gerrit.cloudera.org:8080/#/c/4440/3/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

Line 192: }
> Sure, you could add another CreateTable() variant. Alternatively, you could
Thanks, since the tablet/tserver data were available in place, I tested their 
output too.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Create base class for MiniCluster and ExternalMiniCluster

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: Create base class for MiniCluster and ExternalMiniCluster
..


Patch Set 5:

(1 comment)

LGTM, one nit below which you can ignore.

http://gerrit.cloudera.org:8080/#/c/3974/5/src/kudu/integration-tests/mini_cluster.h
File src/kudu/integration-tests/mini_cluster.h:

Line 91:   void ShutdownMasters();
Not exactly related to this, but while we are here, perhaps we could just flag 
a bool to Shutdown(bool masters_only) instead of another routine 
ShutDownMasters here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I62256168a9245c845e99f7253f07e69a225954bf
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-1500: Data race in 
RaftConsensusITest.TestCorruptReplicaMetadata
..

KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

The test intends to corrupt the metadata of one of the tserver tablets.
While the cluster is in the process of resurrecting the failed tserver,
the Partition and PartitionSchema on that tserver is accessed using
TabletPeer in an unguarded manner via ListTablets RPCs.

The proposed fix here keeps the Partition and PartitionSchema immutable
once after it is loaded during replica bootstrap. These fields are never
overwritten again during tablet-copy or any other workflow.
Also a unit test is added to exercise this data race window. i.e,
the unit test aims to issue ListTablets RPCs during a follower's
tablet copy stage and provides an optimistic coverage for the fix.
DCHECKs are added to make sure the oncoming protobuf fields
match the immutable fields not updated during tablet-copy.

Testing: Spun a test TabletCopyITest.TestDeleteTabletDuringTabletCopy
which exercises this data race window, also ran original test which
caught the race in raft_consensus-itest.TestCorruptReplicaMetadata.

Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.cc
2 files changed, 131 insertions(+), 16 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1500: Data race in 
RaftConsensusITest.TestCorruptReplicaMetadata
..


Patch Set 12:

(1 comment)

Thanks again, updated along with std:: prefixes to keep jenkins happy.

http://gerrit.cloudera.org:8080/#/c/3823/11/src/kudu/integration-tests/tablet_copy-itest.cc
File src/kudu/integration-tests/tablet_copy-itest.cc:

Line 268: 
> Can get rid of this
Nnice ! yeah that was a useless return, updated all of above.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

[Dinesh Bhat (Code Review)]

Hello Dan Burkert, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-1500: Data race in 
RaftConsensusITest.TestCorruptReplicaMetadata
..

KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata

The test intends to corrupt the metadata of one of the tserver tablets.
While the cluster is in the process of resurrecting the failed tserver,
the Partition and PartitionSchema on that tserver is accessed using
TabletPeer in an unguarded manner via ListTablets RPCs.

The proposed fix here keeps the Partition and PartitionSchema immutable
once after it is loaded during replica bootstrap. These fields are never
overwritten again during tablet-copy or any other workflow.
Also a unit test is added to exercise this data race window. i.e,
the unit test aims to issue ListTablets RPCs during a follower's
tablet copy stage and provides an optimistic coverage for the fix.
DCHECKs are added to make sure the oncoming protobuf fields
match the immutable fields not updated during tablet-copy.

Testing: Spun a test TabletCopyITest.TestDeleteTabletDuringTabletCopy
which exercises this data race window, also ran original test which
caught the race in raft_consensus-itest.TestCorruptReplicaMetadata.

Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.cc
2 files changed, 132 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/3823/15
-- 
To view, visit http://gerrit.cloudera.org:8080/3823
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon