date:20171023

[kudu-CR] consensus: support changing between VOTER and NON VOTER

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8297 )

Change subject: consensus: support changing between VOTER and NON_VOTER
..


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8297/4/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:

http://gerrit.cloudera.org:8080/#/c/8297/4/src/kudu/consensus/raft_consensus.h@569
PS4, Line 569:  boost::optional delta = 
boost::none);
can you explain what 'delta' is here? I know it's just passing through  to the 
above Enable case but it's worth a comment like 'See EnableFailureDetector()'


http://gerrit.cloudera.org:8080/#/c/8297/4/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

http://gerrit.cloudera.org:8080/#/c/8297/4/src/kudu/consensus/raft_consensus.cc@2424
PS4, Line 2424:   ToggleFailureDetector(cmeta_->CommittedConfig());
can't you remove the config param from  the ToggleFailureDetector function and  
just always use  ActiveConfig? Here we just cleared the pending config, so we 
know t hat the active config is the committed config.


http://gerrit.cloudera.org:8080/#/c/8297/4/src/kudu/consensus/raft_consensus.cc@2622
PS4, Line 2622:   ToggleFailureDetector(new_config);
same here, we just set the pending config, so if the function just uses 
ActiveConfig  it'll do the right thing


http://gerrit.cloudera.org:8080/#/c/8297/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8297/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@103
PS4, Line 103:   // Promote non-voter replica to voter one.
 :   Status PromoteReplica(const string& tablet_id,
 : const TServerDetails* replica,
 : const MonoDelta& timeout) {
 : return ChangeReplicaMembership(tablet_id, replica, 
RaftPeerPB::VOTER, timeout);
 :   }
 :
 :   // Demote voter replica to non-voter one.
 :   Status DemoteReplica(const string& tablet_id,
 :const TServerDetails* replica,
 :const MonoDelta& timeout) {
 : return ChangeReplicaMembership(tablet_id, replica, 
RaftPeerPB::NON_VOTER, timeout);
 :   }
I'd go  one further and just inline these ChangeReplicaMembership calls down 
below - it'll net save 10 lines or so and one layer of indirection



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978cbed683b0e95a903f2bd2b57496aee71cb33a
Gerrit-Change-Number: 8297
Gerrit-PatchSet: 4
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 24 Oct 2017 03:34:48 +
Gerrit-HasComments: Yes

[kudu-CR] tool: new actions for adding and removing data directories

2017-10-23 Thread Andrew Wong (Code Review)

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8352 )

Change subject: tool: new actions for adding and removing data directories
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/kudu-tool-test.cc@2330
PS2, Line 2330:   NO_FATALS(RunActionStdoutNone(Substitute(
> Ah, good example. Yes, the assertion implies that it's past checking data d
Interesting problem, hadn't thought of that..

To give a bit more context, TSTabletManager::Init() will load all of the tablet 
metadata from disk first, and once all are loaded, it will open the blocks. My 
guess is this is to prevent some disruptive IO patterns since this stresses the 
metadata directory. The loading is encapsulated by TabletMetadata::Load(), 
which verifies schemas, partitions, corruption, directory groups, etc. Once the 
metadata are successfully loaded, CreateAndRegisterTabletReplica() is called on 
each metadata object to create a replica. This call is used in a few places: 
creating a brand new replica, during tablet copies, when bootstrapping the 
server, and also when starting up and the tablet data is messed up, in which 
case a tombstoned replica is created (we may be able to do something similar! 
See HandleNonReadyTabletOnStartup() in ts_tablet_manager.cc).

At first I was thinking we could somehow use the superblock to do some sort of 
group-size comparison, but it's tricky because TabletMetadata::Load abstracts 
away access to it. I suppose if we did want to defer resolution, one path 
forward would be to store the superblock in the tablet metadata when loading it 
from disk.

Alternatively we could maybe push parts or all of OpenTabletMeta into 
CreateAndRegisterTabletReplica, although this would require a bit of 
refactoring in a few places since the calls aren't always paired together (e.g. 
in the case of tablet copies).

We could also special-case this like we do for HandleNonReadyTabletOnStartup() 
and create a dummy replica that gets failed immediately.

I think a relatively clean solution would be to wrap the metadata in a 
struct/tuple like TabletMetadataAndStatus that could be passed to 
CreateAndRegisterTabletReplica, which would then check the status if it's a 
"missing directory" error, and successfully create a failed replica. This would 
have the benefit of being usable in the case of disk failure as well, although 
at this point I think we should only permit the "missing directory" case.
The tricky thing here is that TabletMetadata::Load() may return different 
versions of IOError, NotFound, etc., that we may not necessarily want to create 
a failed replica for (although maybe we should?).

Based on description-length, I'm leaning towards the last solution, although I 
think it'd be a small amount of ugly plumbing. Trying to think of more, but 
from these, wdyt?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
Gerrit-Change-Number: 8352
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 24 Oct 2017 01:41:17 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM

2017-10-23 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8162 )

Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
..


Patch Set 10:

(5 comments)

Looks good to me, my only real feedback is to add a comment to the LBM deletion 
transaction dtor, as I verbosely explained in the comment.

http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:

http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager-test.cc@260
PS10, Line 260: FindOrNull
I think it'd be better to do a FindOrDie here to be more explicit about what 
happens when the metric isn't found.  Dereferencing null should be equivalent, 
but I'd expect the error message from FindOrDie to be easier to debug.


http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager-test.cc@266
PS10, Line 266: entity->FindOrNull(*prototype).get())->value());
likewise


http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager-test.cc@1069
PS10, Line 1069:   {
Are these explicit blocks necessary for correctness, or is it just to add a 
little bit of scoping?


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

http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager.cc@231
PS10, Line 231: destructed
should this be committed?


http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager.cc@1244
PS10, Line 1244: LogBlockDeletionTransaction::~LogBlockDeletionTransaction() {
What is the reason to do this work in the destructor instead of in 
CommitDeletedBlocks?  I'm a bit surprised we're doing work in the destructor, 
since the FBM equivalent doesn't do anything in the destructor, and nothing in 
the BlockDeletionTransaction says anything about the dtor.I asked in the 
test file about the braces being added, and now I see this is probably the 
reason.  Could we do all of this in CommitDeletedBlocks instead?  Perhaps the 
dtor could be empty except for a DCHECK that the transaction has previously 
been committed.

Edit: OK I think I've figured this out.  We're using the 
LogBlockDeletionTransaction's shared ownership to keep it alive until the very 
last block is destructed, at which point the actual hole punching takes place.  
I think this is sufficiently tricky that you should document this lifetime 
semantics on this dtor method.

It also changes the semantics so that no hole punching is attempted until the 
last block of the transaction is dropped, but I think that's probably fine, I 
can't see a reason why it would cause problems.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 24 Oct 2017 00:53:17 +
Gerrit-HasComments: Yes

[kudu-CR] consensus: support changing between VOTER and NON VOTER

2017-10-23 Thread Alexey Serbin (Code Review)

Alexey Serbin has uploaded a new patch set (#4) to the change originally 
created by Mike Percy. ( http://gerrit.cloudera.org:8080/8297 )

Change subject: consensus: support changing between VOTER and NON_VOTER
..

consensus: support changing between VOTER and NON_VOTER

This patch implements changing from VOTER to NON_VOTER and vice-versa.

Added an integration test scenario for NON_VOTER --> VOTER,
VOTER --> NON_VOTER changes and other use cases.

Change-Id: I978cbed683b0e95a903f2bd2b57496aee71cb33a
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
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_nonvoter-itest.cc
8 files changed, 345 insertions(+), 14 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978cbed683b0e95a903f2bd2b57496aee71cb33a
Gerrit-Change-Number: 8297
Gerrit-PatchSet: 4
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] consensus: support changing between VOTER and NON VOTER

2017-10-23 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8297 )

Change subject: consensus: support changing between VOTER and NON_VOTER
..


Patch Set 3:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/consensus/raft_consensus.cc@1674
PS3, Line 1674: type
> can you use ChangeConfigType_Name here to get it stringified?
Done


http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/consensus/raft_consensus.cc@2428
PS3, Line 2428:   if (cmeta_->IsVoterInConfig(peer_uuid(), 
COMMITTED_CONFIG)) {
  : EnableFailureDetector(boost::none);
  :   } else {
  : DisableFailureDetector();
  :   }
> we have this same bit of code in a bunch of places, maybe we can extract a
Done: added ToggleFailureDetector().  Though I'm not sure it has safe enough 
signature for all cases.  Raft consensus state looks tricky.


http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@157
PS3, Line 157: Status RaftConsensusNonVoterITest::PromoteReplica(
> this function seems identical to DemoteReplica except for the constant VOTE
Done


http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@595
PS3, Line 595: 64
> why the power of two here? seems a bit unnecessarily magic
changed to 50


http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@723
PS3, Line 723: while (workload.rows_inserted() < 1000) {
> I think it would be better to capture the number of rows inserted right aft
Good idea.  Done.


http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@751
PS3, Line 751: approrpiate
> typo
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978cbed683b0e95a903f2bd2b57496aee71cb33a
Gerrit-Change-Number: 8297
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 24 Oct 2017 00:11:44 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

2017-10-23 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6755 )

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6755/8/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/6755/8/src/kudu/server/webserver.cc@382
PS8, Line 382:
> Nit: revert this empty line addition.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 24 Oct 2017 00:10:35 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

2017-10-23 Thread Hao Hao (Code Review)

Hello Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon,

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

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

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

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
..

KUDU-1957: Clarify web UI redaction in --redact flag help

This patch refines the definition of --redact flag to control the
context of redaction. The available options now are 'all', 'log'
and 'none'. If 'all' is specified, sensitive data (sensitive
configuration flags and row data) will be redacted from the
web UI as well as glog and error messages. If 'log' is specified,
sensitive data will only be redacted from glog and error messages.
And the option 'flag' is removed from --redact flag to let this
flag only control redaction context.

Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
---
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/test_util.cc
12 files changed, 51 insertions(+), 41 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster

2017-10-23 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8304 )

Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8304/6/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:

http://gerrit.cloudera.org:8080/#/c/8304/6/src/kudu/mini-cluster/external_mini_cluster.h@138
PS6, Line 138:   // If true, set up a Hive Metastore as part of this 
ExternalMiniCluster.
> include a line Default: false/true
It's not so simple due to cases like data_root, block_manager_type, or 
daemon_bin_path. I mean, you could inline the "" default, but you'd still need 
some description of what "" means.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
Gerrit-Change-Number: 8304
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 24 Oct 2017 00:07:37 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8304 )

Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8304/6/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:

http://gerrit.cloudera.org:8080/#/c/8304/6/src/kudu/mini-cluster/external_mini_cluster.h@138
PS6, Line 138:   // If true, set up a Hive Metastore as part of this 
ExternalMiniCluster.
include a line Default: false/true

(maybe we should move to using inline defaults in the class definition, 
C++11-style, so we don't have to duplicate the default in the comment and the 
code)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
Gerrit-Change-Number: 8304
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 24 Oct 2017 00:03:31 +
Gerrit-HasComments: Yes

[kudu-CR] tool: new actions for adding and removing data directories

2017-10-23 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8352 )

Change subject: tool: new actions for adding and removing data directories
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/kudu-tool-test.cc@2330
PS2, Line 2330:   NO_FATALS(RunActionStdoutNone(Substitute(
> Imagine you have two tablets A and B:
Ah, good example. Yes, the assertion implies that it's past checking data dirs.

Andrew, I'm not as familiar with the data dir group resolution as you are. What 
do you think I should do? Is it possible to defer resolution of data dir groups 
to bootstrap time, so that a failure will fail the tablet instead of the entire 
tserver?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
Gerrit-Change-Number: 8352
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 24 Oct 2017 00:01:57 +
Gerrit-HasComments: Yes

[kudu-CR] tool: new actions for adding and removing data directories

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8352 )

Change subject: tool: new actions for adding and removing data directories
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/kudu-tool-test.cc@2330
PS2, Line 2330:   NO_FATALS(RunActionStdoutNone(Substitute(
> We do data dir group resolution while loading the tablet's superblock, and
Imagine you have two tablets A and B:

>> TS starts
- create tablet A
- write block 1 on drive /data/1, flush tablet A's metadata, pointing to it
- create tablet B
- write block 2 on drive /data/2, flush tablet B's metadata, pointing to it

>>shutdown tserver
>> run tool to remove /data/2
>> start tserver
- LBM decides that max_block_id is '1'
- tablet A bootstraps first
-- immediately when done bootstrapping, it causes an MRS flush, and writes a 
new block id 2 (re-used) to /data/1
- tablet B bootstraps
-- it thinks it has block '2' but in fact it's pointing to the new block 2

I would have expected tablet B in this case to see "ah, I have a data dir that 
has been removed" and mark itself as failed. But your assertion above is having 
it say "can't find block" which implies that it is getting past the point of 
checking data dir existence, right?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
Gerrit-Change-Number: 8352
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 23:57:02 +
Gerrit-HasComments: Yes

[kudu-CR] [webui] Add templates for tserver webui

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8307 )

Change subject: [webui] Add templates for tserver webui
..


Patch Set 6:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc
File src/kudu/tserver/tserver_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@a156
PS6, Line 156:
so did we lose the ?raw ability here? I actually use this pretty often with 
'curl' to get output in a readable format over an ssh connection. Not sure if 
JSON would be as readable.


http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@169
PS6, Line 169: transaction_json["trace"] = inflight_tx.trace_buffer();
Perhaps we should be std::move()ing the large strings out of the PB into the 
JSON to avoid copy?


http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@210
PS6, Line 210: Subst
std::to_string


http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@220
PS6, Line 220: percentage
do you want to be using StringPrintf here to get a nice printout? it seems like 
in the template you're just including this directly rather than something like 
%%%.1f or whatever


http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@225
PS6, Line 225: kArray
I'm surprised this isn't kObject.. how does this work?


http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@235
PS6, Line 235:  if (replica->tablet() != nullptr) {
 :   tablet_detail_json["target"] = Substitute("/tablet?id=$0", 
status.tablet_id());
 :   tablet_detail_json["mem_bytes"] = 
HumanReadableNumBytes::ToString(
 :   replica->tablet()
isn't there a race here where tablet() can become null in between, if it's 
shutting down?


http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@413
PS6, Line 413: Substitute("$0",
std::to_string() here and a few other spots

(though I thought easyjson could hold ints too)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99e08be9aa8abddd51ada61683b6f75190a00b5c
Gerrit-Change-Number: 8307
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 23:53:13 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

2017-10-23 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6755 )

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
..


Patch Set 8: Code-Review+1

(1 comment)

Would like Dan or Todd to also review.

http://gerrit.cloudera.org:8080/#/c/6755/8/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/6755/8/src/kudu/server/webserver.cc@382
PS8, Line 382:
Nit: revert this empty line addition.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 23:53:10 +
Gerrit-HasComments: Yes

[kudu-CR] tool: new actions for adding and removing data directories

2017-10-23 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8352 )

Change subject: tool: new actions for adding and removing data directories
..


Patch Set 2:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/kudu-tool-test.cc@2330
PS2, Line 2330:   ASSERT_STR_CONTAINS(s.ToString(), "Can't find block");
> shouldn't the tablet fail because it sees that one of its disks is now miss
We do data dir group resolution while loading the tablet's superblock, and hard 
failing there means the tserver will fail to start. I tried this at first (by 
modifying DataDirGroup::FromPB to return a bad status when we can't find a data 
dir) and that's what I got.

Separately, remind me what's the problem with block ID reuse? I know we have a 
problem in tests where we lack the ability to clear the block cache, but what's 
wrong with reuse in production, across distinct tserver process instances?


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

http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/tool_action_fs.cc@348
PS2, Line 348: Status AddDataDir(const RunnerContext& context) {
> when adding a data dir, should we have some option to add it to existing ta
That's a good idea, but it'd mean rewriting all the superblocks, so it's not 
trivial to do when you factor in the cleanup work in the event of an error.

Can I punt on it for now?


http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/tool_action_fs.cc@554
PS2, Line 554: Any tablets with data on the removed data directory "
 : "will fail the next time the server is 
started
> Sort of had a similar thought: perhaps a mandatory unsafe "force" flag if t
I'd rather not introduce interactivity to this particular action, but I'm OK 
with Andrew's idea to require users to pass an "optional" --force parameter. I 
can check for the presence of containers as an approximation for "is there live 
data?", and require users to use --force if there is.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
Gerrit-Change-Number: 8352
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 23:46:08 +
Gerrit-HasComments: Yes

[kudu-CR] tool: new actions for adding and removing data directories

2017-10-23 Thread Adar Dembo (Code Review)

Hello Andrew Wong, Todd Lipcon,

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

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

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

Change subject: tool: new actions for adding and removing data directories
..

tool: new actions for adding and removing data directories

To add a data directory, a new FS root is created and within it, a new data
directory. Then, the set of all_uuids is updated to include the uuid of the
new data directory, and all of the on-disk instance files are updated with
the new value of all_uuids.

To remove a data directory, some sanity checks are performed, all_uuids is
updated to exclude the uuid of the data directory being removed, and all of
the on-disk instance files are updated with the new value of all_uuids.
Needless to say, removing a data directory will cause any tablet with data
in it to fail at the next startup.

In terms of the overall approach, I waffled between fully encapsulating the
new logic in FsManager/DataDirManager, and separating it out entirely.
Eventually I settled on this hybrid model where existing code in the
FsManager and DataDirManager created the roots, directories, and instance
files, while tool-specific code was responsible for updating existing
instances, fsyncing directories, and stringing it all together.

Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
---
M src/kudu/fs/block_manager_util.h
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action_fs.cc
9 files changed, 791 insertions(+), 110 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
Gerrit-Change-Number: 8352
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] tool: remove dependency on libcurl

2017-10-23 Thread Adar Dembo (Code Review)

Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8364 )

Change subject: tool: remove dependency on libcurl
..

tool: remove dependency on libcurl

This dependency was introduced in commit 761ce10, when I added support for
spawning mini clusters through the CLI tool. We could treat it as a valid
new dependency and doc it, but it only exists because of one ExternalDaemon
test function. So instead, let's move that function from ExternalDaemon to
the group of free functions in cluster_itest_util.

I verified that with this patch the CLI no longer depends on kudu_curl_util
or on libcurl, and that the build still passes with NO_TESTS=1.

Change-Id: Ifcf0b4eeed9f8af38cc471d81eb59a0473f470c3
Reviewed-on: http://gerrit.cloudera.org:8080/8364
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/integration-tests/client-stress-test.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/create-table-itest.cc
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/dense_node-itest.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/raft_consensus-itest-base.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/util/CMakeLists.txt
19 files changed, 205 insertions(+), 188 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifcf0b4eeed9f8af38cc471d81eb59a0473f470c3
Gerrit-Change-Number: 8364
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

2017-10-23 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6755 )

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
..


Patch Set 8:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/server/webserver.cc@385
PS7, Line 385: shared_lock l(lock_);
 : PathHandlerMap::const_iterator it = path_handlers
> This seems a little strange; how about just heap allocating the ScopedDisab
Moved the disable logic to L468 as you suggested.


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/server/webserver.cc@468
PS7, Line 468: ScopedDisableRedaction s;
> Would it be sufficient to disable redaction around L468?
Done


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flag_tags.h
File src/kudu/util/flag_tags.h:

http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flag_tags.h@84
PS7, Line 84: The values of these flags are considered sensitive and will be 
redacted
: // if redaction is enabled.
: //
> Let's be a little bit vague about this, to allow the redaction implementati
Done


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.h
File src/kudu/util/flags.h:

http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.h@68
PS7, Line 68: // Get all the flags different from their defaults. The output is 
a nicely
: // formatted string with --flag=value pairs per line. Redact any 
flags that
: // are tagged as sensitive, if redaction is enabled.
> Update.
Done


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.cc@545
PS7, Line 545: args << "--" << flag.name << '=' << flag_value;
 :   }
> Seems like this call-site ought to check g_should_redact_log and the one in
On a second thought, I am not sure if it would be better to leave it more 
vague.  As now there is not much differentiation in GetNonDefaultFlags() and 
CommandlineFlagsIntoString(). The enabling/disabling of web UI redaction logic 
is mainly controlled by ScopedDisableRedaction.


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h
File src/kudu/util/logging.h:

http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@63
PS7, Line 63: #define KUDU_SHOULD_REDACT() ((kudu::g_should_redact == 
kudu::RedactContext::ALL ||\
> Shouldn't this check g_should_redact_log || g_should_redact_web?
Updated.


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@91
PS7, Line 91: enum class RedactContext {
> It might be clearer if the tri-state --redact values were mapped to a tri-s
Updated to tri-state.


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@93
PS7, Line 93:
> Nit: don't need the
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 23:39:19 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

2017-10-23 Thread Hao Hao (Code Review)

Hello Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon,

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

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

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

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
..

KUDU-1957: Clarify web UI redaction in --redact flag help

This patch refines the definition of --redact flag to control the
context of redaction. The available options now are 'all', 'log'
and 'none'. If 'all' is specified, sensitive data (sensitive
configuration flags and row data) will be redacted from the
web UI as well as glog and error messages. If 'log' is specified,
sensitive data will only be redacted from glog and error messages.
And the option 'flag' is removed from --redact flag to let this
flag only control redaction context.

Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
---
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/test_util.cc
12 files changed, 52 insertions(+), 41 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] tool: remove dependency on libcurl

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8364 )

Change subject: tool: remove dependency on libcurl
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcf0b4eeed9f8af38cc471d81eb59a0473f470c3
Gerrit-Change-Number: 8364
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Mon, 23 Oct 2017 23:36:58 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7053 )

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 30:

(15 comments)

http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py
File build-support/run_dist_test.py:

http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py@128
PS30, Line 128:   env['HIVE_HOME'] = glob.glob(os.path.join(ROOT, 
"thirdparty/src/apache-hive-*-bin"))[0]
  :   env['HADOOP_HOME'] = glob.glob(os.path.join(ROOT, 
"thirdparty/src/hadoop-*"))[0]
when we end up revving these dependencies it seems likely we'll end up with 
multiple ones in src/. Could we do use the installed/opt/ symlink that you make 
elsewhere for this?


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/CMakeLists.txt@40
PS30, Line 40: execute_process(COMMAND ln -nsf
 : 
"${CMAKE_SOURCE_DIR}/thirdparty/installed/common/opt/hive"
 : "${EXECUTABLE_OUTPUT_PATH}/hive-home")
 : execute_process(COMMAND ln -nsf
 : 
"${CMAKE_SOURCE_DIR}/thirdparty/installed/common/opt/hadoop"
 : "${EXECUTABLE_OUTPUT_PATH}/hadoop-home")
 : execute_process(COMMAND ln -nsf
 : "${JAVA_HOME}"
 : "${EXECUTABLE_OUTPUT_PATH}/java-home")
does it make more sense to do these in build-thirdparty? even though it's not a 
"build" step it seems to fit a little closer there (at least for the 
hive/hadoop ones, not necessarily the java one)


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hive_metastore.thrift
File src/kudu/hms/hive_metastore.thrift:

http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hive_metastore.thrift@21
PS30, Line 21: # DO NOT MODIFY! Copied from
should we be setting any C++ specific options like 'moveable types'?


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:

http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client-test.cc@103
PS30, Line 103:   ASSERT_TRUE(CreateTable(, database_name, table_name, 
"").IsRuntimeError());
i think it's worth ASSERT_STR_MATCHES the error string here since RuntimeError 
is so generic


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:

http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@60
PS30, Line 60:   // can not be reached, an error is returned.
is there some default timeout, etc?


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@73
PS30, Line 73:   // returned.
it's probably worth a class-wide comment on what the errors returned will be 
for other types of issues, such as if the RPC times out or the HMS has 
disconnected, etc.


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@85
PS30, Line 85: bool cascade = false,
 :   bool delete_data = true)
would this be better off as a bitset of flags?


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@121
PS30, Line 121:int32_t max_events,
do we have any idea of the expected size of these events? wondering if we need 
to be thinking about memory consumption of this component carefully or not


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@124
PS30, Line 124:   // Deserializes a JSON encoded table.
can you add a little more context as to where one would see the JSON-encoded 
table format?


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:

http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc@78
PS30, Line 78: IOError
would RemoteError be better here?

Is there any more specific error to distinguish network issues and timeouts vs 
exceptions thrown on the remote end?


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc@94
PS30, Line 94: HmsClient::HmsClient(const HostPort& hms_address)
is there a world in which people run multiple HMS in an HA configuration? If so 
is that typically done by configuring clients or putting the HMSs behind a 
proxy or vip?

Do we need to support auto-reconnect after failure? eg if someone just bounces 
the HMS how does the Kudu master know to reconnect/retry? Is that assumed to be 
at a higher layer? Might be worth documenting this at the class-doc level.


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc@96
PS30, Line 96:   auto socket = make_shared(hms_address.host(), 
hms_address.port());
no need to set timeouts on the TSocket?


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc@99
PS30, Line 99:   client_ =

[kudu-CR] tool: remove dependency on libcurl

2017-10-23 Thread Adar Dembo (Code Review)

Hello Will Berkeley, Alexey Serbin, Dan Burkert,

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

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

to review the following change.


Change subject: tool: remove dependency on libcurl
..

tool: remove dependency on libcurl

This dependency was introduced in commit 761ce10, when I added support for
spawning mini clusters through the CLI tool. We could treat it as a valid
new dependency and doc it, but it only exists because of one ExternalDaemon
test function. So instead, let's move that function from ExternalDaemon to
the group of free functions in cluster_itest_util.

I verified that with this patch the CLI no longer depends on kudu_curl_util
or on libcurl, and that the build still passes with NO_TESTS=1.

Change-Id: Ifcf0b4eeed9f8af38cc471d81eb59a0473f470c3
---
M src/kudu/integration-tests/client-stress-test.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/create-table-itest.cc
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/dense_node-itest.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/raft_consensus-itest-base.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/util/CMakeLists.txt
19 files changed, 205 insertions(+), 188 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifcf0b4eeed9f8af38cc471d81eb59a0473f470c3
Gerrit-Change-Number: 8364
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Will Berkeley

[kudu-CR] kudu-tool-test: adjust minicluster functions slightly

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8351 )

Change subject: kudu-tool-test: adjust minicluster functions slightly
..

kudu-tool-test: adjust minicluster functions slightly

I didn't like that cluster_opts_ was a member of the test fixture. This
patch changes the use of cluster options to be more conventional.

Change-Id: I0f7e613432e165d449e7901be03880c55e8770ab
Reviewed-on: http://gerrit.cloudera.org:8080/8351
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong 
Reviewed-by: Todd Lipcon 
---
M src/kudu/tools/kudu-tool-test.cc
1 file changed, 20 insertions(+), 26 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Andrew Wong: Looks good to me, but someone else must approve
  Todd Lipcon: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0f7e613432e165d449e7901be03880c55e8770ab
Gerrit-Change-Number: 8351
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] kudu-tool-test: adjust minicluster functions slightly

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8351 )

Change subject: kudu-tool-test: adjust minicluster functions slightly
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f7e613432e165d449e7901be03880c55e8770ab
Gerrit-Change-Number: 8351
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 22:23:20 +
Gerrit-HasComments: No

[kudu-CR](branch-1.4.x) KUDU-2170 Masters can start with duplicates specified

2017-10-23 Thread Will Berkeley (Code Review)

Will Berkeley has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8360 )

Change subject: KUDU-2170 Masters can start with duplicates specified
..

KUDU-2170 Masters can start with duplicates specified

Users can specify duplicate masters in --master_addresses, where a
duplicate includes duplicated names and the same server known by
different names. This doesn't cause problems as far as I know, but it
looks scary because, for example, the web ui shows extra masters listed in
/masters. This is because /masters shows one master per result from
ListMasters. This patch deduplicates the output of ListMasters by UUID.

It also warns on start if there are exact string duplicates in
--master_addresses, at the time the master compares on-disk and
configured master addresses, but before the masters are ready to
resolve UUIDs.

Also, the error message when on-disk and configured masters don't match
is confusing. This patch makes it easy to understand.

Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Reviewed-on: http://gerrit.cloudera.org:8080/8341
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong 
Reviewed-on: http://gerrit.cloudera.org:8080/8360
---
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
2 files changed, 33 insertions(+), 3 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Adar Dembo: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: branch-1.4.x
Gerrit-MessageType: merged
Gerrit-Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Gerrit-Change-Number: 8360
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley

[kudu-CR](branch-1.3.x) KUDU-2170 Masters can start with duplicates specified

2017-10-23 Thread Will Berkeley (Code Review)

Will Berkeley has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8361 )

Change subject: KUDU-2170 Masters can start with duplicates specified
..

KUDU-2170 Masters can start with duplicates specified

Users can specify duplicate masters in --master_addresses, where a
duplicate includes duplicated names and the same server known by
different names. This doesn't cause problems as far as I know, but it
looks scary because, for example, the web ui shows extra masters listed in
/masters. This is because /masters shows one master per result from
ListMasters. This patch deduplicates the output of ListMasters by UUID.

It also warns on start if there are exact string duplicates in
--master_addresses, at the time the master compares on-disk and
configured master addresses, but before the masters are ready to
resolve UUIDs.

Also, the error message when on-disk and configured masters don't match
is confusing. This patch makes it easy to understand.

Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Reviewed-on: http://gerrit.cloudera.org:8080/8341
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong 
Reviewed-on: http://gerrit.cloudera.org:8080/8361
---
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
2 files changed, 33 insertions(+), 3 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Adar Dembo: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-MessageType: merged
Gerrit-Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Gerrit-Change-Number: 8361
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley

[kudu-CR](branch-1.5.x) KUDU-2170 Masters can start with duplicates specified

2017-10-23 Thread Will Berkeley (Code Review)

Will Berkeley has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8359 )

Change subject: KUDU-2170 Masters can start with duplicates specified
..

KUDU-2170 Masters can start with duplicates specified

Users can specify duplicate masters in --master_addresses, where a
duplicate includes duplicated names and the same server known by
different names. This doesn't cause problems as far as I know, but it
looks scary because, for example, the web ui shows extra masters listed in
/masters. This is because /masters shows one master per result from
ListMasters. This patch deduplicates the output of ListMasters by UUID.

It also warns on start if there are exact string duplicates in
--master_addresses, at the time the master compares on-disk and
configured master addresses, but before the masters are ready to
resolve UUIDs.

Also, the error message when on-disk and configured masters don't match
is confusing. This patch makes it easy to understand.

Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Reviewed-on: http://gerrit.cloudera.org:8080/8341
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong 
Reviewed-on: http://gerrit.cloudera.org:8080/8359
---
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
2 files changed, 24 insertions(+), 3 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Adar Dembo: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: branch-1.5.x
Gerrit-MessageType: merged
Gerrit-Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Gerrit-Change-Number: 8359
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley

[kudu-CR] tool: new actions for adding and removing data directories

2017-10-23 Thread Andrew Wong (Code Review)

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8352 )

Change subject: tool: new actions for adding and removing data directories
..


Patch Set 2:

(1 comment)

Still reviewing, but thought I'd comment in response to one of Todd's points.

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

http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/tool_action_fs.cc@554
PS2, Line 554: Any tablets with data on the removed data directory "
 : "will fail the next time the server is 
started
> Is there any way we can force an interactive user to confirm a 'YES' to thi
Sort of had a similar thought: perhaps a mandatory unsafe "force" flag if the 
directory isn't empty? In practice it might be just as easy to run, but at 
least users would need to acknowledge the fact that data is being deleted. Not 
sure how easy the interactive bit would be to implement given the current Kudu 
tooling code.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
Gerrit-Change-Number: 8352
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 22:04:26 +
Gerrit-HasComments: Yes

[kudu-CR] tool: new actions for adding and removing data directories

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8352 )

Change subject: tool: new actions for adding and removing data directories
..


Patch Set 2:

(3 comments)

just skimmed this and left some high-level comments

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

http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/kudu-tool-test.cc@2330
PS2, Line 2330:   ASSERT_STR_CONTAINS(s.ToString(), "Can't find block");
shouldn't the tablet fail because it sees that one of its disks is now missing? 
relying on 'can't find block' is a little scary, because if the block is not 
found, we might accidentally end up re-using this blockid for a new allocation, 
since we start allocating new blocks at max(block-id) + 1, right?


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

http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/tool_action_fs.cc@348
PS2, Line 348: Status AddDataDir(const RunnerContext& context) {
when adding a data dir, should we have some option to add it to existing tablet 
pathsets, so the new disk is used for new blocks? This seems to make sense in 
the world where the prior config was to spread all tablets across all disks.


http://gerrit.cloudera.org:8080/#/c/8352/2/src/kudu/tools/tool_action_fs.cc@554
PS2, Line 554: Any tablets with data on the removed data directory "
 : "will fail the next time the server is 
started
Is there any way we can force an interactive user to confirm a 'YES' to this? 
Otherwise I'm afraid people may not truly understand this, run the script 
across a bunch of servers, and then lose data.

Maybe we can have it look and see if there are any blocks at all in the removed 
dir, and if not, not confirm/complain? I think a relatively common use case 
here is that someone starts Kudu for the first time with the wrong set of dirs, 
doesn't create any data at all, and then restarts with a different set.

This could also later expand to an option to migrate blocks off of the removed 
disk (though I guess that will be trickier because tablet metadata also needs 
to be updated with the appropriate datadir group)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
Gerrit-Change-Number: 8352
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 21:57:28 +
Gerrit-HasComments: Yes

[kudu-CR](gh-pages) Add slides from my Strata 2017 talk

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8362 )

Change subject: Add slides from my Strata 2017 talk
..

Add slides from my Strata 2017 talk

Change-Id: I60389b5a0e356f271160a6db9cea524d158a6a15
Reviewed-on: http://gerrit.cloudera.org:8080/8362
Reviewed-by: Jean-Daniel Cryans 
Tested-by: Todd Lipcon 
---
A _posts/2017-10-23-nosql-kudu-spanner-slides.md
1 file changed, 48 insertions(+), 0 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: merged
Gerrit-Change-Id: I60389b5a0e356f271160a6db9cea524d158a6a15
Gerrit-Change-Number: 8362
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR](gh-pages) Add slides from my Strata 2017 talk

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8362 )

Change subject: Add slides from my Strata 2017 talk
..


Patch Set 2: Verified+1


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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: comment
Gerrit-Change-Id: I60389b5a0e356f271160a6db9cea524d158a6a15
Gerrit-Change-Number: 8362
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 21:42:24 +
Gerrit-HasComments: No

[kudu-CR] consensus: support changing between VOTER and NON VOTER

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8297 )

Change subject: consensus: support changing between VOTER and NON_VOTER
..


Patch Set 3:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/consensus/raft_consensus.cc@1674
PS3, Line 1674: type
can you use ChangeConfigType_Name here to get it stringified?


http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/consensus/raft_consensus.cc@2428
PS3, Line 2428:   if (cmeta_->IsVoterInConfig(peer_uuid(), 
COMMITTED_CONFIG)) {
  : EnableFailureDetector(boost::none);
  :   } else {
  : DisableFailureDetector();
  :   }
we have this same bit of code in a bunch of places, maybe we can extract a 
function like 'EnableOrDisableFailureDetector()'?


http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@157
PS3, Line 157: Status RaftConsensusNonVoterITest::PromoteReplica(
this function seems identical to DemoteReplica except for the constant VOTER vs 
NON_VOTER. How about taking that as a parameter and just making this function 
be PromoteOrDemoteVoter or ChangeVoterType or something?


http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@595
PS3, Line 595: 64
why the power of two here? seems a bit unnecessarily magic


http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@723
PS3, Line 723: while (workload.rows_inserted() < 1000) {
I think it would be better to capture the number of rows inserted right after 
the 'Demote' call, and then wait for it to continue to increase by another 
1000. Otherwise it's quite likely that it already reached 1000 prior to Demote


http://gerrit.cloudera.org:8080/#/c/8297/3/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@751
PS3, Line 751: approrpiate
typo



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978cbed683b0e95a903f2bd2b57496aee71cb33a
Gerrit-Change-Number: 8297
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 21:42:10 +
Gerrit-HasComments: Yes

[kudu-CR](branch-1.5.x) KUDU-2170 Masters can start with duplicates specified

2017-10-23 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8359 )

Change subject: KUDU-2170 Masters can start with duplicates specified
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: branch-1.5.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Gerrit-Change-Number: 8359
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 23 Oct 2017 21:34:53 +
Gerrit-HasComments: No

[kudu-CR](branch-1.3.x) KUDU-2170 Masters can start with duplicates specified

2017-10-23 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8361 )

Change subject: KUDU-2170 Masters can start with duplicates specified
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Gerrit-Change-Number: 8361
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 23 Oct 2017 21:34:54 +
Gerrit-HasComments: No

[kudu-CR](gh-pages) Add slides from my Strata 2017 talk

2017-10-23 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8362 )

Change subject: Add slides from my Strata 2017 talk
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: comment
Gerrit-Change-Id: I60389b5a0e356f271160a6db9cea524d158a6a15
Gerrit-Change-Number: 8362
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Comment-Date: Mon, 23 Oct 2017 21:12:42 +
Gerrit-HasComments: No

[kudu-CR](gh-pages) Add slides from my Strata 2017 talk

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/8362 )

Change subject: Add slides from my Strata 2017 talk
..

Add slides from my Strata 2017 talk

Change-Id: I60389b5a0e356f271160a6db9cea524d158a6a15
---
A _posts/2017-10-23-nosql-kudu-spanner-slides.md
1 file changed, 48 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60389b5a0e356f271160a6db9cea524d158a6a15
Gerrit-Change-Number: 8362
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon

[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster

2017-10-23 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8304 )

Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster
..


Patch Set 6:

(1 comment)

I don't think ignoring IWYU by giving yourself a +1 verified is appropriate, 
because it means whoever modifies these files next has to deal with the exact 
same IWYU failures.

If you want to ignore a particular recommendation, please add the appropriate 
pragma to do so.

http://gerrit.cloudera.org:8080/#/c/8304/6/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:

http://gerrit.cloudera.org:8080/#/c/8304/6/src/kudu/mini-cluster/external_mini_cluster.h@38
PS6, Line 38: #include "kudu/hms/mini_hms.h"
The IWYU recommendation to forward declare MiniHms is legit; see our discussion 
in part 2 about forward declaring classes stored in unique_ptrs.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
Gerrit-Change-Number: 8304
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 23 Oct 2017 19:51:36 +
Gerrit-HasComments: Yes

[kudu-CR](gh-pages) Add slides from my Strata 2017 talk

2017-10-23 Thread Todd Lipcon (Code Review)

Todd Lipcon has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8362


Change subject: Add slides from my Strata 2017 talk
..

Add slides from my Strata 2017 talk

Change-Id: I60389b5a0e356f271160a6db9cea524d158a6a15
---
A _posts/2017-10-23-nosql-kudu-spanner-slides.md
1 file changed, 48 insertions(+), 0 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: newchange
Gerrit-Change-Id: I60389b5a0e356f271160a6db9cea524d158a6a15
Gerrit-Change-Number: 8362
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

2017-10-23 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7053 )

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 30:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h@30
PS29, Line 30: #include "kudu/util/subprocess.h"
> unique_ptr doesn't work with forward declared classes.
You sure about that? I'm looking at 
https://stackoverflow.com/questions/6012157/is-stdunique-ptrt-required-to-know-the-full-definition-of-t
 and it suggests that, for most operations incomplete knowledge of the class is 
OK.

I also wrote a small test that's similar to mini_hms to prove this to myself:

  --bar.cc--
  #include "bar.h"
  #include "foo.h"

  Bar::~Bar() {}
  void Bar::CreateFoo() {
f.reset(new Foo());
f->a = "hello world";
  }

  --test.cc--
  #include "bar.h"
  #include "foo.h"

  int main(void) {
Bar b;
b.CreateFoo();
return 0;
  }

  --bar.h--
  #include 

  struct Foo;
  struct Bar {
Bar() = default;
~Bar();
void CreateFoo();
   private:
std::unique_ptr f;
  };

  --foo.h--
  #include 

  struct Foo {
std::string a;
  };

This test compiles and links. As you can see, bar.h has only partial knowledge 
of Foo, but full knowledge of it where needed (bar.cc and test.cc).

Also, if I follow Alexey's advice and move Bar's noarg constructor from bar.h 
to bar.cc, I can remove the inclusion of foo.h from test.cc.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 19:47:45 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

2017-10-23 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7053 )

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 29:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client-test.cc@25
PS29, Line 25: #include 
 : #include 
> stl_logging.h is necessary for the stream formatting on line 194.  I'll rem
It seems stl_loggin.h is a frequent point of confusion for IWYU.  I'll take a 
look whether it's possible to handle it with a mapping.

Meanwhile, could you add IWYU pragma, please:

#include   // IWYU pragma: keep


http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h@22
PS29, Line 22: #include 
> perhaps, but I don't think I should remove it.
Could you at least add the IWYU pragma, please?  It should be something like:

#include  // IWYU pragma: keep



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 19:41:36 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

2017-10-23 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7053 )

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 29:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h@30
PS29, Line 30: #include "kudu/util/subprocess.h"
> unique_ptr doesn't work with forward declared classes.
It depends on what 'work' is required :)

The funny fact here is that if you move the definition of the MiniHms 
constructor into the .cc file, it will work with forward-declared Subprocess 
class.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 19:31:18 +
Gerrit-HasComments: Yes

[kudu-CR] java: improve javadoc for setRangePartitionColumns

2017-10-23 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8314 )

Change subject: java: improve javadoc for setRangePartitionColumns
..


Patch Set 3:

Thanks!


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icfa41aa26bf1961f27a08a42a8353098d04305b6
Gerrit-Change-Number: 8314
Gerrit-PatchSet: 3
Gerrit-Owner: Hector Camarena 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 19:31:12 +
Gerrit-HasComments: No

[kudu-CR] java: improve javadoc for setRangePartitionColumns

2017-10-23 Thread Dan Burkert (Code Review)

Dan Burkert has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8314 )

Change subject: java: improve javadoc for setRangePartitionColumns
..

java: improve javadoc for setRangePartitionColumns

Adding clarification to setRangePartitionColumns documentation, stating
that if not set, by default the range will be partitioned by the primary
key columns, along with a single unbounded partition. Only when set to
an empty vector, will the table be created without range partition.

Change-Id: Icfa41aa26bf1961f27a08a42a8353098d04305b6
Reviewed-on: http://gerrit.cloudera.org:8080/8314
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
1 file changed, 4 insertions(+), 3 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Dan Burkert: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Icfa41aa26bf1961f27a08a42a8353098d04305b6
Gerrit-Change-Number: 8314
Gerrit-PatchSet: 4
Gerrit-Owner: Hector Camarena 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] java: improve javadoc for setRangePartitionColumns

2017-10-23 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8314 )

Change subject: java: improve javadoc for setRangePartitionColumns
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icfa41aa26bf1961f27a08a42a8353098d04305b6
Gerrit-Change-Number: 8314
Gerrit-PatchSet: 3
Gerrit-Owner: Hector Camarena 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 19:31:06 +
Gerrit-HasComments: No

[kudu-CR](branch-1.3.x) KUDU-2170 Masters can start with duplicates specified

2017-10-23 Thread Will Berkeley (Code Review)

Hello Kudu Jenkins, Andrew Wong, Adar Dembo,

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

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

to review the following change.


Change subject: KUDU-2170 Masters can start with duplicates specified
..

KUDU-2170 Masters can start with duplicates specified

Users can specify duplicate masters in --master_addresses, where a
duplicate includes duplicated names and the same server known by
different names. This doesn't cause problems as far as I know, but it
looks scary because, for example, the web ui shows extra masters listed in
/masters. This is because /masters shows one master per result from
ListMasters. This patch deduplicates the output of ListMasters by UUID.

It also warns on start if there are exact string duplicates in
--master_addresses, at the time the master compares on-disk and
configured master addresses, but before the masters are ready to
resolve UUIDs.

Also, the error message when on-disk and configured masters don't match
is confusing. This patch makes it easy to understand.

Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Reviewed-on: http://gerrit.cloudera.org:8080/8341
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong 
---
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
2 files changed, 33 insertions(+), 3 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Gerrit-Change-Number: 8361
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR](branch-1.4.x) KUDU-2170 Masters can start with duplicates specified

2017-10-23 Thread Will Berkeley (Code Review)

Hello Kudu Jenkins, Andrew Wong, Adar Dembo,

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

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

to review the following change.


Change subject: KUDU-2170 Masters can start with duplicates specified
..

KUDU-2170 Masters can start with duplicates specified

Users can specify duplicate masters in --master_addresses, where a
duplicate includes duplicated names and the same server known by
different names. This doesn't cause problems as far as I know, but it
looks scary because, for example, the web ui shows extra masters listed in
/masters. This is because /masters shows one master per result from
ListMasters. This patch deduplicates the output of ListMasters by UUID.

It also warns on start if there are exact string duplicates in
--master_addresses, at the time the master compares on-disk and
configured master addresses, but before the masters are ready to
resolve UUIDs.

Also, the error message when on-disk and configured masters don't match
is confusing. This patch makes it easy to understand.

Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Reviewed-on: http://gerrit.cloudera.org:8080/8341
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong 
---
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
2 files changed, 33 insertions(+), 3 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: branch-1.4.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Gerrit-Change-Number: 8360
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR](branch-1.5.x) KUDU-2170 Masters can start with duplicates specified

2017-10-23 Thread Will Berkeley (Code Review)

Hello Kudu Jenkins, Andrew Wong, Adar Dembo,

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

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

to review the following change.


Change subject: KUDU-2170 Masters can start with duplicates specified
..

KUDU-2170 Masters can start with duplicates specified

Users can specify duplicate masters in --master_addresses, where a
duplicate includes duplicated names and the same server known by
different names. This doesn't cause problems as far as I know, but it
looks scary because, for example, the web ui shows extra masters listed in
/masters. This is because /masters shows one master per result from
ListMasters. This patch deduplicates the output of ListMasters by UUID.

It also warns on start if there are exact string duplicates in
--master_addresses, at the time the master compares on-disk and
configured master addresses, but before the masters are ready to
resolve UUIDs.

Also, the error message when on-disk and configured masters don't match
is confusing. This patch makes it easy to understand.

Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Reviewed-on: http://gerrit.cloudera.org:8080/8341
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong 
---
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
2 files changed, 24 insertions(+), 3 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: branch-1.5.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5047ccf92dde5377da18a1a84e105a951fa5b48c
Gerrit-Change-Number: 8359
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] consensus: support changing between VOTER and NON VOTER

2017-10-23 Thread Alexey Serbin (Code Review)

Alexey Serbin has uploaded a new patch set (#3) to the change originally 
created by Mike Percy. ( http://gerrit.cloudera.org:8080/8297 )

Change subject: consensus: support changing between VOTER and NON_VOTER
..

consensus: support changing between VOTER and NON_VOTER

This patch implements changing from VOTER to NON_VOTER and vice-versa.

Added an integration test scenario for NON_VOTER --> VOTER,
VOTER --> NON_VOTER changes and other use cases.

Change-Id: I978cbed683b0e95a903f2bd2b57496aee71cb33a
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.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_nonvoter-itest.cc
7 files changed, 335 insertions(+), 3 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978cbed683b0e95a903f2bd2b57496aee71cb33a
Gerrit-Change-Number: 8297
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] java: improve javadoc for setRangePartitionColumns

2017-10-23 Thread Hector Camarena (Code Review)

Hello Dan Burkert, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: java: improve javadoc for setRangePartitionColumns
..

java: improve javadoc for setRangePartitionColumns

Adding clarification to setRangePartitionColumns documentation, stating
that if not set, by default the range will be partitioned by the primary
key columns, along with a single unbounded partition. Only when set to
an empty vector, will the table be created without range partition.

Change-Id: Icfa41aa26bf1961f27a08a42a8353098d04305b6
---
M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
1 file changed, 4 insertions(+), 3 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icfa41aa26bf1961f27a08a42a8353098d04305b6
Gerrit-Change-Number: 8314
Gerrit-PatchSet: 3
Gerrit-Owner: Hector Camarena 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] kudu-tool-test: adjust minicluster functions slightly

2017-10-23 Thread Andrew Wong (Code Review)

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8351 )

Change subject: kudu-tool-test: adjust minicluster functions slightly
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f7e613432e165d449e7901be03880c55e8770ab
Gerrit-Change-Number: 8351
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 18:15:10 +
Gerrit-HasComments: No

[kudu-CR] [tests] clean-up on cluster itest util

2017-10-23 Thread Alexey Serbin (Code Review)

Alexey Serbin has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8318 )

Change subject: [tests] clean-up on cluster_itest_util
..

[tests] clean-up on cluster_itest_util

Updated signature of the AddServer(), RemoveServer(), and DeleteTablet()
methods to accommodate the majority of their use cases.

Also, changed the signature of the TSTabletManager::DeleteTablet() to be
more lightweight.

This changelist does not contain any functional modifications.

Change-Id: Icc5c85fb58750dec286a8ae546db955e3bd4073c
Reviewed-on: http://gerrit.cloudera.org:8080/8318
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/delete_tablet-itest.cc
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_election-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tablet_copy_client_session-itest.cc
M src/kudu/integration-tests/tablet_replacement-itest.cc
M src/kudu/integration-tests/tombstoned_voting-imc-itest.cc
M src/kudu/integration-tests/tombstoned_voting-stress-test.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
18 files changed, 227 insertions(+), 260 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Dan Burkert: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Icc5c85fb58750dec286a8ae546db955e3bd4073c
Gerrit-Change-Number: 8318
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster

2017-10-23 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8304 )

Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster
..


Patch Set 6: Verified+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8304/2/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/8304/2/src/kudu/mini-cluster/external_mini_cluster.cc@189
PS2, Line 189: RETURN_NOT_OK_PREPEND(hms_->Start(),
> RETURN_ NOT_OK_PREPEND and add a useful string?
Done


http://gerrit.cloudera.org:8080/#/c/8304/2/src/kudu/mini-cluster/external_mini_cluster.cc@331
PS2, Line 331:   scoped_refptr master = new 
ExternalMaster(opts);
> I don't think the master knows what this flag is yet?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
Gerrit-Change-Number: 8304
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 23 Oct 2017 14:46:23 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster

2017-10-23 Thread Dan Burkert (Code Review)

Dan Burkert has removed a vote on this change.

Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster
..


Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8304
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
Gerrit-Change-Number: 8304
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

2017-10-23 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7053 )

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 30: Verified+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 14:45:40 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

2017-10-23 Thread Dan Burkert (Code Review)

Dan Burkert has removed a vote on this change.

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [tests] clean-up on cluster itest util

2017-10-23 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8318 )

Change subject: [tests] clean-up on cluster_itest_util
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc5c85fb58750dec286a8ae546db955e3bd4073c
Gerrit-Change-Number: 8318
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 14:13:59 +
Gerrit-HasComments: No

[kudu-CR] java: improve javadoc for setRangePartitionColumns

2017-10-23 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8314 )

Change subject: java: improve javadoc for setRangePartitionColumns
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8314/2/java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
File 
java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java:

http://gerrit.cloudera.org:8080/#/c/8314/2/java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java@90
PS2, Line 90: by
:* default a table will be created with a range partitioned by 
the columns
:* set in the hash partition, along with partition unbounded
It will default to the PK columns, not the hash columns.  I think this will 
read better as:

'If not set, the table is range partitioned by the primary key columns with a 
single unbounded partition.'



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icfa41aa26bf1961f27a08a42a8353098d04305b6
Gerrit-Change-Number: 8314
Gerrit-PatchSet: 2
Gerrit-Owner: Hector Camarena 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 14:13:12 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

2017-10-23 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7053 )

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 30:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client-test.cc@25
PS29, Line 25: #include 
 : #include 
> I think IWYU is right about removing these; I don't see any LOG/VLOG usage
stl_logging.h is necessary for the stream formatting on line 194.  I'll remove 
glog/logging.h.


http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h@22
PS29, Line 22: #include 
> Maybe IWYU wants to remove this because:
perhaps, but I don't think I should remove it.


http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.cc@26
PS29, Line 26:
> Indeed, it seems IWYU is confused, and I cannot see why this happens.
Done


http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h@30
PS29, Line 30: #include "kudu/util/subprocess.h"
> IWYU's recommendation to remove this makes sense: you can forward declare S
unique_ptr doesn't work with forward declared classes.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 23 Oct 2017 14:05:41 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

2017-10-23 Thread Dan Burkert (Code Review)

Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 
Todd Lipcon,

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

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

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

Change subject: KUDU-2191 (2/n): Hive Metastore client
..

KUDU-2191 (2/n): Hive Metastore client

This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.

The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.

- Thrift has been added as a dependency, and a mechanism for performing
  Thrift codegen at compile time has been added (see FindThrift.cmake,
  based on FindProtobuf.cmake)

- Bison has been added as a build-time dependency, because the system
  bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.

- Hive and Hadoop have been added to thirdparty as test-only dependencies.

- A Hive MetaStore external mini server is included for testing. See
  mini_hms.[h|cc].

- The Kudu Metastore plugin is compiled from CMake as a standalone jar
  for loading into the HMS mini server.

Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,911 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/30
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

58 matches

Mail list logo