[kudu-CR] tool: add cluster shell action

[Alexey Serbin (Code Review)]

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

Change subject: tool: add cluster shell action
..


Patch Set 8:

(11 comments)

Just skimmed through.  Will take a deeper look tomorrow.

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

http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/kudu-tool-test.cc@1843
PS8, Line 1843: ASSERT_FALSE(s.ok());
  : ASSERT_TRUE(s.IsInvalidArgument());
here and below: why is it necessary to verify for ASSERT_FALSE(s.ok()); if next 
line is about ASSERT_TRUE(s.IsInvlidArgument())?


http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:

http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@60
PS8, Line 60: The generated ID of the cluster, unique to the control shell.
maybe just: 'ID of the cluster to destroy.' ?


http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@91
PS8, Line 91: required HostPortPB process_address
How does it work for multiple addresses (corresponding to 
'--rpc_bind_addresses' flag)?  Or we never want to have that test cluster bound 
to multiple addresses?


http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@104
PS8, Line 104: HostPortPB
Same question if multiple addresses are allowed: which address should be 
specified here -- the first one or any of the bound addresses?


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

http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@20
PS8, Line 20: 
nit: please use  instead.


http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@64
PS8, Line 64: ;
nit: remove extra semicolon


http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@102
PS8, Line 102: InvalidArgument
nit: maybe, NotFound is the better choice here?


http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@133
PS8, Line 133: InvalidArgument
ditto regarding NotFound


http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@150
PS8, Line 150: FLAGS_serialization == "json"
As I see, case-insensitive comparison (i.e. boost::iequals(v, "json")) is used 
in the validator.  Maybe, it makes sense to compare case-insensitive comparison 
here as well?


http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@153
PS8, Line 153: "pb", FLAGS_serialization
ditto for case-insensitive comparison.


http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@216
PS8, Line 216:   opts.master_rpc_ports = { 11030, 11031, 11032 };
nit: add DCHECK_EQ(3, opts.num_masters) ?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6
Gerrit-Change-Number: 7853
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 03 Oct 2017 04:30:17 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1125: issue one catalog write per tablet report

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8090 )

Change subject: KUDU-1125: issue one catalog write per tablet report
..


Patch Set 7:

(9 comments)

This is a big improvement, Adar.

http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3106
PS6, Line 3106:"requestor", rpc->requestor_string(),
> would be nice to also use TRACE_COUNTER_INCREMENT("reported_tablets", updat
Todd, I'm not sure how your example can help measure latency. Did you mean to 
suggest using something like TRACE_COUNTER_SCOPE_LATENCY_US() ?


http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3113
PS6, Line 3113:
TODO(todd) per git blame


http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3152
PS7, Line 3152: acqurie
acquire


http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3173
PS7, Line 3173: // reasonable to ignore it and wait for an operator fix 
the situation.
The end of this comment only makes sense after going to the flag definition and 
seeing that it defaults to false. I wonder, if we are going to default it to 
false, if we shouldn't just remove it and this by-default-disabled logic 
altogether and simply emit the warning on L3181. When was the last time we 
enabled this? I actually thought the default behavior of the master was to 
delete unknown tablets.


http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3236
PS7, Line 3236: report.has_consensus_state()
I think this tertiary if condition needs to be: (report.has_consensus_state() 
&& report.consensus_state().committed_config().has_opid_index()) because of the 
issue brought up in the comment on L3269.


http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3291
PS7, Line 3291: Should it?
Interesting question. I think it technically should, because the purpose of 
disregarding the non-member leaders is to wait until a leader is part of the 
committed config before publishing its existence to clients. OTOH I can't 
easily come up with a series of events where this would be a problem because to 
change the config there has to be an initial leader that coordinates the config 
change, and this logic is about waiting for that first leader when creating a 
new tablet from scratch for the first time.


http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3319
PS7, Line 3319: config
cstate; a config doesn't really have a term


http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3320
PS7, Line 3320: config
cstate


http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3452
PS7, Line 3452: as
in



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6f5cf0e4b1cd1160b3b310d89c6dbf3dd62e43b
Gerrit-Change-Number: 8090
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 03 Oct 2017 02:55:41 +
Gerrit-HasComments: Yes

[kudu-CR] WIP [consensus] adding/removing NON VOTER members

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: WIP [consensus] adding/removing NON_VOTER members
..

WIP [consensus] adding/removing NON_VOTER members

Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly.  Also, added new integration test:
  * RaftConsensusITest.AddNonVoterReplica
  * RaftConsensusITest.AddThenRemoveNonVoterReplica
  * RaftConsensusITest.NonVoterReplicasDoNotVote

WIP: Perhaps, I should continue gathering feedback on this patch
 and adding more tests.

Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/metadata.proto
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
9 files changed, 488 insertions(+), 58 deletions(-)


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

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

[kudu-CR] [tablet copy] end session before bootstrapping tablet

[Alexey Serbin (Code Review)]

Alexey Serbin has abandoned this change. ( http://gerrit.cloudera.org:8080/8133 
)

Change subject: [tablet copy] end session before bootstrapping tablet
..


Abandoned

Abandoned in favor of http://gerrit.cloudera.org:8080/8197
--
To view, visit http://gerrit.cloudera.org:8080/8133
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I20592007a40113d8409f121b226ba14e8300
Gerrit-Change-Number: 8133
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] [tablet copy] comment on TabletCopyClient lifecycle

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8197


Change subject: [tablet copy] comment on TabletCopyClient lifecycle
..

[tablet copy] comment on TabletCopyClient lifecycle

Added an explanatory comment on the TabletCopyClient instance lifecycle.
That was the result of an attempt to clean-up corresponding code:
  https://gerrit.cloudera.org/#/c/8133/

Change-Id: I7ae41f12fe75f05c84518c26b6875f34efae0172
---
M src/kudu/tserver/ts_tablet_manager.cc
1 file changed, 32 insertions(+), 0 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7ae41f12fe75f05c84518c26b6875f34efae0172
Gerrit-Change-Number: 8197
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 

[kudu-CR] KUDU-1125: issue one catalog write per tablet report

[Adar Dembo (Code Review)]

Hello Mike Percy, Dan Burkert, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-1125: issue one catalog write per tablet report
..

KUDU-1125: issue one catalog write per tablet report

This commit addresses a long-standing issue in the catalog manager where an
independent catalog write is performed for each reported tablet. When the
master is configured to fsync WAL writes, this can add a lot of load during
election storms and when the master is restarted.

To tackle this, I fully rewrote ProcessTabletReport and friends. I had
higher hopes for the final product, but all of the dependent control flow
complicates decomposition. Still, I managed to make some improvements. For
example, all RPCs are now sent at the end in one go rather than piecemeal. I
also rewrote all of the comments so that they can serve as a map to the
function and to emphasize the actions performed by the corresponding code.

Here are the actual semantic changes being made:
- Table and tablet locks are now acquired en masse. For tablets, this is
  required for correctness; I've documented why I did the same for tables.
- We no longer check for non-running tables. AFAICT this was a useless check
  because a non-running table must be a deleted table, and there's a check
  for that just before.
- We no longer wake the BgTasks thread upon completion. This is because:
  1. WakeIfHasPendingUpdates() was semi-broken; pending_updates_ is never
 set in ProcessTabletReport.
  2. Regardless, there's no work for the BgTasks thread to do.

Change-Id: Ie6f5cf0e4b1cd1160b3b310d89c6dbf3dd62e43b
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
2 files changed, 387 insertions(+), 402 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie6f5cf0e4b1cd1160b3b310d89c6dbf3dd62e43b
Gerrit-Change-Number: 8090
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1125: issue one catalog write per tablet report

[Adar Dembo (Code Review)]

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

Change subject: KUDU-1125: issue one catalog write per tablet report
..


Patch Set 6:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8090/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8090/6//COMMIT_MSG@10
PS6, Line 10: When the
: master is configured to fsync WAL writes, this can add a lot of 
load during
: election storms and when the master is restarted.
> any chance you did a before/after test here? eg start a small cluster with
Haven't done any perf testing yet. I was planning to do it post-merge, but I'll 
tackle it now instead.

I'll also try to experiment to see whether your estimate for max tablets is 
correct. It's tough because although it's pretty easy to trigger a full report 
(restart the master or a tserver), the underlying write performed by that 
report is likely to be sparse because 3b6ab64 will filter out all tablets that 
haven't actually changed. Meanwhile, events that do yield real changes (such as 
an election storm or the creation of a table) tend to be staggered over time. 
In short, it's hard to create a full report with nothing but real changes.


http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3106
PS6, Line 3106:"num_tablets", 
full_report.updated_tablets_size());
> would be nice to also use TRACE_COUNTER_INCREMENT("reported_tablets", updat
Done


http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3122
PS6, Line 3122:   unordered_map updates;
> worth adding to the comment explaining ownership of these raw pointers
Done


http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3151
PS6, Line 3151: ReportedTabletUpdatesPB* update = 
full_report_update->add_tablets();
> to cut down on small allocations here, consider doing full_report_update->m
Done


http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3157
PS6, Line 3157:   shared_lock l(lock_);
> I think this would likely perform better if you can acquire this once for t
I think I can do that without much refactoring, just need a new scope here.


http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3227
PS6, Line 3227: bootstrapping
> should this now say "copying"?
Yeah. The original wording was "This prevents us from spuriously deleting 
replicas that have just been added to a pending config and are in the process 
of catching up to the log entry where they were added to the config."

Dan asked me whether it should say "just been added to the committed config and 
are in the process of bootstrapping" and I misinterpreted his suggestion as 
"change the wording to this", though that wasn't his intent.


http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3249
PS6, Line 3249: with an error
> nit: rephrase as "non-deleted tablet which is reporting an error" or somesu
Done


http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3268
PS6, Line 3268:   if (!cstate.committed_config().has_opid_index()) {
> nit: would it be possible to move this if statement up a few lines before c
Yeah, I think that'd be safe.


http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3285
PS6, Line 3285:   // TODO(adar): ShouldTransitionTabletToRunning doesn't 
take step 7b into
  :   // account. Should it?
> not sure I understand this question -- isn't the condition from ShouldTrans
This TODO is trying to draw attention to the fact that 
ShouldTransitionTabletToRunning when examining the replica's ConsensusStatePB, 
goes straight to report rather than looking at the local 'cstate' (which may 
have had its leader_uuid() cleared).

I am comfortable changing it to use 'cstate', but I wasn't sure whether that'd 
be correct.


http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3344
PS6, Line 3344: ConsensusStatePB prev_cstate = 
tablet->metadata().state().pb.consensus_state();
> this is a little confusing - this is shadowing a variable with the same nam
Ugh, yes. Given the COW nature of the persistent data, it should be safe to 
reuse the existing prev_cstate even though we're mutating the cstate.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6f5cf0e4b1cd1160b3b310d89c6dbf3dd62e43b
Gerrit-Change-Number: 8090
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] KUDU-2055 [part 5]: Incorporate BlockDeletionTransaction in block deletions

[Adar Dembo (Code Review)]

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

Change subject: KUDU-2055 [part 5]: Incorporate BlockDeletionTransaction in 
block deletions
..


Patch Set 1:

(1 comment)

Anything worth testing here?

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

http://gerrit.cloudera.org:8080/#/c/8163/1//COMMIT_MSG@10
PS1, Line 10: to improve overall performance of
: batched block deletions, such as delete a tablet.
to improve overall performance of tablet deletion by batching up the individual 
blocks being deleted.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1bfac61a70b45d9f27d807f22bc21e582da2dd97
Gerrit-Change-Number: 8163
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 02 Oct 2017 22:33:03 +
Gerrit-HasComments: Yes

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

[Adar Dembo (Code Review)]

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

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


Patch Set 3:

(18 comments)

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

http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@13
PS3, Line 13: It also adds a new metric 'holes_punched' in log block manager to 
track
Let's also add a new counter for 'blocks_deleted', which will track the total 
number of deletions since startup. By comparing it with holes_punched, we can 
see how effective the coalescing behavior is.

Actually, let's add 'blocks_deleted' and 'blocks_created' as generic block 
manager metrics (see block_manager_metrics.{cc,h}), since they're not specific 
to the LBM.


http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@14
PS3, Line 14: operation
operations


http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@17
PS3, Line 17: vaule
value


http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@17
PS3, Line 17: coalescing hole punch works as expected by checking the vaule of
punching


http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/block_manager.h@323
PS3, Line 323: class BlockDeletionTransaction {
In part 3 I left a comment about how it'd be nice for these transaction classes 
to be purely virtual. If you agree, I think this bit of refactoring should be 
moved to part 3.


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

http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager-test.cc@347
PS3, Line 347: std::shared_ptr 
deletion_transaction =
Add a using:: for this.


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

http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@128
PS3, Line 128: METRIC_DEFINE_gauge_uint64(server, 
log_block_manager_holes_punched,
Because the number of holes punched only ever increases, this should be a 
counter not a gauge.

See "Gauge vs. Counter" in metrics.h for more information.


http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@129
PS3, Line 129:"Blocks Under Management",
Update.


http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@130
PS3, Line 130:kudu::MetricUnit::kBlocks,
Update.


http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@131
PS3, Line 131:"Number of data blocks currently 
under management");
Update.


http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@182
PS3, Line 182:   scoped_refptr holes_punched;
Please separate from the previous two metrics with a blank line, since it's not 
an "under management" metric.


http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@229
PS3, Line 229: to
with


http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@230
PS3, Line 230: gets
is


http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@246
PS3, Line 246: that this block has been registered to
with which this block has been registered.


http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1208
PS3, Line 1208: if (container->block_manager()->metrics()) {
  :   
container->block_manager()->metrics()->holes_punched->IncrementBy(
  :   entry.second.size());
  : }
For this to be more accurate:
1. Defer it to ContainerDeletionAsync, so that the change in metric values 
reflect the time that it actually takes to process the hole punches.
2. Only do it if hole punching actually succeeds.


http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1224
PS3, Line 1224: Status s = lbm_->RemoveLogBlock(block, shared_from_this());
Would be better if:
1. shared_from_this() were called once; no need to call it for every iteration 
in the loop.
2. RemoveLogBlock() was actually RemoveLogBlocks() to avoid a spinlock 
acquisition/release with each loop iteration.


http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1286
PS3, Line 1286:   transaction_->AddBlock(this);
The call sequence for deleting blocks is long and involves a lot of class 
traversal:
1. LogBlockManager::DeletionTransaction() to get a new transaction. Also calls 
BlockDeletionTransaction and LogBlockDeletionTransaction constructors.
2. BlockDeletionTransaction::AddDeletedBlock to mark a block as to-be-deleted.
3. LogBlockDeletionTransaction::CommitDeletedBlocks to effect the deletions 
in-memory and to set them up to be deleted on disk. This calls 

[kudu-CR] [tablet copy] end session before bootstrapping tablet

[Alexey Serbin (Code Review)]

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

Change subject: [tablet copy] end session before bootstrapping tablet
..


Patch Set 1:

> > Should this patch be abandoned?
 >
 > I think I need to add the corresponding comment and then abandon
 > the patch.  I'll do that today.

I meant I'll post a new patch to add comments on the necessity to keep the 
target session alive until the tablet successfully starts up.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20592007a40113d8409f121b226ba14e8300
Gerrit-Change-Number: 8133
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Mon, 02 Oct 2017 22:26:51 +
Gerrit-HasComments: No

[kudu-CR] [tablet copy] end session before bootstrapping tablet

[Alexey Serbin (Code Review)]

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

Change subject: [tablet copy] end session before bootstrapping tablet
..


Patch Set 1:

> Should this patch be abandoned?

I think I need to add the corresponding comment and then abandon the patch.  
I'll do that today.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20592007a40113d8409f121b226ba14e8300
Gerrit-Change-Number: 8133
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Mon, 02 Oct 2017 22:24:08 +
Gerrit-HasComments: No

[kudu-CR] [tests] fix flakes in delete table-itest

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7972 )

Change subject: [tests] fix flakes in delete_table-itest
..


Patch Set 3:

Maybe we should just merge this patch the way it is to reduce the flakiness 
issues... thoughts?


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dda40ae1054becaf25963a6d301ecaed5a926f9
Gerrit-Change-Number: 7972
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 02 Oct 2017 21:53:32 +
Gerrit-HasComments: No

[kudu-CR] KUDU-1788. Increase Raft RPC timeout to 30sec to avoid fruitless retries.

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8037 )

Change subject: KUDU-1788. Increase Raft RPC timeout to 30sec to avoid 
fruitless retries.
..


Patch Set 1:

Are we planning on merging this patch?


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f47dc006dc3dfb1659a224172e1905b6bf3d2a4
Gerrit-Change-Number: 8037
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 02 Oct 2017 21:52:39 +
Gerrit-HasComments: No

[kudu-CR] WIP [raft consensus-itest] fix flake in TestSlowLeader

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7887 )

Change subject: WIP [raft_consensus-itest] fix flake in TestSlowLeader
..


Patch Set 2:

I think this change makes sense but I am wondering what the purpose of the read 
thread is in the test, anyway. The original test doesn't do a good job of 
justifying its existence or implementation in my opinion. I am honestly 
surprised it is not more flaky than reported here.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5ee6c5400c947f87b1da2e76d24dd837b1270ca
Gerrit-Change-Number: 7887
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 02 Oct 2017 21:51:57 +
Gerrit-HasComments: No

[kudu-CR] [tablet copy] end session before bootstrapping tablet

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8133 )

Change subject: [tablet copy] end session before bootstrapping tablet
..


Patch Set 1:

Should this patch be abandoned?


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20592007a40113d8409f121b226ba14e8300
Gerrit-Change-Number: 8133
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Mon, 02 Oct 2017 21:52:19 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction

[Adar Dembo (Code Review)]

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

Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and 
BlockDeletionTransaction
..


Patch Set 5:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/8144/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8144/4//COMMIT_MSG@10
PS4, Line 10: so that in
: future they can be extended for specific implementations.
Is one of the motivations also to force block creation and deletion to take 
place via these transactions rather than today's  CreateBlock() and 
DeleteBlock()? If so, it would be good to understand how we'll get there, since 
that requires some additional refactoring.


http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/bloomfile.cc
File src/kudu/cfile/bloomfile.cc:

http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/bloomfile.cc@116
PS4, Line 116: fs::BlockManager
We having "using fs::..." stuff above; you can do the same for BlockManager.


http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:

http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile-test.cc@937
PS4, Line 937:   fs::BlockManager* bm = fs_manager_->block_manager();
We having "using fs::..." stuff above; you can do the same for BlockManager.


http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile_writer.cc
File src/kudu/cfile/cfile_writer.cc:

http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile_writer.cc@206
PS4, Line 206:   fs::BlockManager* bm = block_->block_manager();
We having "using fs::..." stuff above; you can do the same for BlockManager.


http://gerrit.cloudera.org:8080/#/c/8144/5/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

http://gerrit.cloudera.org:8080/#/c/8144/5/src/kudu/fs/block_manager.h@258
PS5, Line 258: CreationTransaction
Nit: how about NewCreationTransaction()? And NewDeletionTransaction?


http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h@283
PS4, Line 283: class BlockCreationTransaction {
I think this and BlockDeletionTransaction should be pure interfaces. It may 
result in some duplicated code (not much, I think), but I believe it'll let you 
eliminate BlockManager::CloseBlocks, which AFAICT only exists because 
BlockCreationTransaction isn't a member of LogBlockManager.

The other problem with the transaction classes being concrete classes is that 
there's nothing stopping me from declaring and using them directly, when the 
intent of this patch is (I believe) to force people to use 
CreationTransaction() and DeletionTransaction(). Is this going to be addressed 
in a follow-on patch?


http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h@325
PS4, Line 325:   explicit BlockDeletionTransaction(BlockManager* bm)
> I read it at: http://en.cppreference.com/w/cpp/language/rule_of_three that
The rule of three explanation makes sense, but it might be nice to define a 
virtual dtor just so this is more consistent with BlockCreationTransaction. 
It's easy to look at the two classes, see that one doesn't define a virtual 
dtor, and assume it's a mistake.


http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/file_block_manager.cc
File src/kudu/fs/file_block_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/file_block_manager.cc@801
PS4, Line 801:   unique_ptr transaction(
 :   new internal::FileBlockCreationTransaction(this));
 :   return std::move(transaction);
Nit: combine


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

http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/log_block_manager.cc@1832
PS4, Line 1832:   unique_ptr transaction(
  :   new internal::LogBlockCreationTransaction(this));
  :   return std::move(transaction);
Nit: combine


http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/deltafile.cc
File src/kudu/tablet/deltafile.cc:

http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/deltafile.cc@116
PS4, Line 116:   fs::BlockManager* bm = writer_->block()->block_manager();
We having "using fs::..." stuff above; you can do the same for BlockManager.


http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:

http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/diskrowset.cc@231
PS4, Line 231:   fs::BlockManager* bm = 
rowset_metadata_->fs_manager()->block_manager();
We having "using fs::..." stuff above; you can do the same for BlockManager.


http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/multi_column_writer.cc
File src/kudu/tablet/multi_column_writer.cc:

[kudu-CR] [catalog manager] introduce replica selector

[Adar Dembo (Code Review)]

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

Change subject: [catalog manager] introduce replica selector
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8161/3/src/kudu/master/master.proto@378
PS3, Line 378: ReplicaSelector
Bike-shedding, but "ReplicaSelector" (especially when seen in C++) suggests a 
stateful class that uses some sort of heuristic or algorithm to make a 
selection. The reality is that it's just an enum, so perhaps 
ReplicaSelectionMode would be a clearer name?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I303a6d158184575a9a105c2d2bf26961ae8b3e93
Gerrit-Change-Number: 8161
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 02 Oct 2017 21:36:53 +
Gerrit-HasComments: Yes

[kudu-CR] [webui] Allow custom response codes and headers

[Adar Dembo (Code Review)]

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

Change subject: [webui] Allow custom response codes and headers
..


Patch Set 6:

(9 comments)

I'm not an HTTP expert so it'd be good to find a reviewer who is (maybe Todd?).

I know Impala also uses Squeasel; perhaps they even use this Webserver code. 
Could you check in with an Impala developer and see whether their Webserver 
already does this, or whether they'd be interested in this functionality?

http://gerrit.cloudera.org:8080/#/c/8141/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8141/6//COMMIT_MSG@14
PS6, Line 14: has
have ("WebResponse and PrerenderedWebResponse" are plural)


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

http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/master/master-path-handlers.cc@242
PS6, Line 242: response
respond


http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.h
File src/kudu/server/webserver.h:

http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.h@66
PS6, Line 66:   // Register a route 'path' to be rendered via template.
Since you're already updating these docs, could you explain what 'alias', 
'is_styled' and 'is_on_nav_bar' do?


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

http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@88
PS6, Line 88: switch (code) {
Add a default case that crashes or something? Would be good to know if we've 
forgotten to update this switch when adding a new code.


http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@452
PS6, Line 452: HttpResponseHeaders{}
Just {} doesn't work?


http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@469
PS6, Line 469:   for (const auto& entry : resp.response_headers) {
Should we enforce that response_headers doesn't include Content-Type, 
Content-Length, or X-Frame-Options? I understand it's sort of moot since no 
callback responds with headers right now, but what happens if a duplicate 
header is in the resposne?


http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@486
PS6, Line 486: HttpResponseHeaders{}
Just {} doesn't work?


http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/util/web_callback_registry.h
File src/kudu/util/web_callback_registry.h:

http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/util/web_callback_registry.h@66
PS6, Line 66:   typedef std::map HttpResponseHeaders;
Is it important that the headers be sorted by key? Or can this be an 
unordered_map instead?


http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/util/web_callback_registry.h@69
PS6, Line 69:   // - 'status_code' determines the status code of the HTTP 
response.
:   // - 'response_headers' are additional headers added to the 
HTTP response.
:   // - 'output' is a JSON object to be rendered in a mustache 
template.
Nit: move each of these so that it's just above the declaration of its field.

Same for 'output' in PrerenderedWebResponse.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ff890785eeb2df3eed9e7c54d0daf760c8b3924
Gerrit-Change-Number: 8141
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Mon, 02 Oct 2017 21:33:14 +
Gerrit-HasComments: Yes

[kudu-CR] java: replace bespoke minicluster implementation with control shell

[Adar Dembo (Code Review)]

Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/8194 )

Change subject: java: replace bespoke minicluster implementation with control 
shell
..


Removed reviewer Kudu Jenkins with the following votes:

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ia6340dadd6ff0397fffa23388a8cbbca3e26a618
Gerrit-Change-Number: 8194
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 

[kudu-CR] java: replace bespoke minicluster implementation with control shell

[Adar Dembo (Code Review)]

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

Change subject: java: replace bespoke minicluster implementation with control 
shell
..


Patch Set 1: Verified+1

Overriding Jenkins, the one test failure was due to an unsynchronized clock.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6340dadd6ff0397fffa23388a8cbbca3e26a618
Gerrit-Change-Number: 8194
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 02 Oct 2017 20:55:11 +
Gerrit-HasComments: No

[kudu-CR] tool: add cluster shell action

[Adar Dembo (Code Review)]

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

Change subject: tool: add cluster shell action
..


Patch Set 8: Verified+1

Overriding Jenkins, the 10 test failures were all due to an unsynchronized 
clock on one dist-test slave.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6
Gerrit-Change-Number: 7853
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo 
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, 02 Oct 2017 20:50:57 +
Gerrit-HasComments: No

[kudu-CR] tool: add cluster shell action

[Adar Dembo (Code Review)]

Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/7853 )

Change subject: tool: add cluster shell action
..


Removed reviewer Kudu Jenkins with the following votes:

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6
Gerrit-Change-Number: 7853
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7981 )

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..


Patch Set 6:

(6 comments)

LGTM, just a couple nits

http://gerrit.cloudera.org:8080/#/c/7981/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/7981/6//COMMIT_MSG@20
PS6, Line 20: tombstoned's tablets
tombstoned tablet's


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

http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/integration-tests/tablet_copy-itest.cc@1726
PS6, Line 1726:   // Find the leader so we can tombstone a follower, which 
makes the test faster and more focused.
  :   int leader_index;
  :   int follower_index;
  :   TServerDetails* leader_ts;
  :   TServerDetails* follower_ts;
  :   ASSERT_OK(FindTabletLeader(ts_map_, tablet_id, kTimeout, 
_ts));
  :   leader_index = 
cluster_->tablet_server_index_by_uuid(leader_ts->uuid());
  :   follower_index = (leader_index + 1) % 
cluster_->num_tablet_servers();
  :   follower_ts = 
ts_map_[cluster_->tablet_server(follower_index)->uuid()];
  :
  :   // Make sure the metrics reflect that some rows have been 
inserted.
  :   
ASSERT_GT(CountRowsInserted(cluster_->tablet_server(follower_index)), 0);
  :
  :   // Tombstone the tablet on the follower.
  :   ASSERT_OK(itest::DeleteTablet(follower_ts, tablet_id,
  : TABLET_DATA_TOMBSTONED,
  : boost::none, kTimeout));
This bit should be wrapped in ASSERT_EVENTUALLY() { ... } in order to  ensure 
that the test doesn't get flaky due to typical latency volatility on the test 
machines.


http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/tserver/tablet_server-test.cc@2583
PS6, Line 2583: It takes three calls
Thanks for the explanation. Can you please add: "at the time of writing, based 
on the policy in ClassFoo" or "foo.cc", or something like that?


http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/tserver/tablet_server-test.cc@2591
PS6, Line 2591: mini
nit: indent this another 4 spaces to line up with the parameter on the above 
line


http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/tserver/tablet_server-test.cc@2592
PS6, Line 2592: 
nit: the indentation of this line seems random


http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:

http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/util/metrics.h@544
PS6, Line 544:   bool unpublished_;
Sometimes thinking in negatives can be confusing. How about flipping this and 
the corresponding getter method to "published" ?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
Gerrit-Change-Number: 7981
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Mon, 02 Oct 2017 19:56:33 +
Gerrit-HasComments: Yes

[kudu-CR] java: replace bespoke minicluster implementation with control shell

[Adar Dembo (Code Review)]

Hello Dan Burkert, Jean-Daniel Cryans,

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

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

to review the following change.


Change subject: java: replace bespoke minicluster implementation with control 
shell
..

java: replace bespoke minicluster implementation with control shell

This patch replaces the Java client's bespoke MiniKuduCluster and MiniKdc
implementations with appropriate calls to the new CLI control shell. The
shell can communicate either in protobuf or JSON; I've opted to use the
former here since the Java client already understands protobuf, and doing
so yields more type safety.

Change-Id: Ia6340dadd6ff0397fffa23388a8cbbca3e26a618
---
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
D java/kudu-client/src/test/java/org/apache/kudu/client/MiniKdc.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestClientFailoverSupport.java
D java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKdc.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestMultipleLeaderFailover.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
D java/kudu-client/src/test/resources/flags
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
10 files changed, 409 insertions(+), 1,233 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia6340dadd6ff0397fffa23388a8cbbca3e26a618
Gerrit-Change-Number: 8194
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 

[kudu-CR] tool: add cluster shell action

[Adar Dembo (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Jean-Daniel Cryans, Kudu Jenkins, 
Todd Lipcon,

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

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

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

Change subject: tool: add cluster shell action
..

tool: add cluster shell action

Maintaining Kudu clients across various languages has been an ongoing
maintenance burden. Even when the client is just a thin wrapper around
another client (e.g. Kudu Python bindings), a great deal of work goes into
client testability. In practice, this has meant a bespoke mini cluster
implementation for each language. On the surface this doesn't seem that bad;
we just need to spawn some masters and tservers, right? Well, the work
quickly adds up:

o While the C++ mini cluster is heavily used and has seen many improvements,
  the Java mini cluster has not received the same kind of love, and is less
  robust as a result. KUDU-1976 is a great example of this deficiency.
o With the inclusion of authn came the addition of a "mini KDC", a special
  daemon for Kerberized mini clusters. It was originally implemented in C++
  and ported to Java, but has yet to be ported to the Python client; this is
  one of the obstacles towards porting full authn support to Python.
o Dan has been prototyping Hive Metastore and Sentry integration for Kudu,
  the testing of which will require "mini HMS" and possibly "mini Sentry"
  testing implementations in C++, Java, and eventually, Python.

In sum, good support for non-C++ mini clusters is an ongoing commitment and
requires a great deal of work. This work hasn't always been forthcoming, and
the non-C++ clusters are deficient as a result. But it doesn't have to be
this way! Here's a thought: what if we reused the C++ mini cluster for tests
written in these other languages? We could write a "proxy" application whose
job it is to manage the C++ mini cluster and expose a rudimentary API that's
easily programmable from Java and Python.

This patch attempts to do just that. It adds a "shell" action to the Kudu
CLI which provides a rudimentary control shell that can be used to spin up
an ExternalMiniCluster. The shell is controlled via a wire protocol over
stdin/stdout. The protocol is protobuf-based with optional JSON encoding.

I should add that I like the idea of shipping "shell" into production as
part of the CLI, as it helps realize the vision of a single Kudu artifact
that can provide Kudu testability for any integrating product.

Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6
---
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
A src/kudu/tools/tool.proto
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
A src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
9 files changed, 1,129 insertions(+), 14 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6
Gerrit-Change-Number: 7853
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo 
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] subprocess: some cosmetic changes

[Adar Dembo (Code Review)]

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

Change subject: subprocess: some cosmetic changes
..

subprocess: some cosmetic changes

Change-Id: I9d81dee93817c560c1237114fb515facbdb06b1d
Reviewed-on: http://gerrit.cloudera.org:8080/8157
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
2 files changed, 33 insertions(+), 19 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9d81dee93817c560c1237114fb515facbdb06b1d
Gerrit-Change-Number: 8157
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] mini-cluster: new module for the mini cluster implementations

[Adar Dembo (Code Review)]

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

Change subject: mini-cluster: new module for the mini cluster implementations
..

mini-cluster: new module for the mini cluster implementations

For the upcoming "control shell" patch, the Kudu CLI will need to link
ExternalMiniCluster and friends. Today that means depending on the
integration-tests target, which doesn't feel quite right since that's
mostly, well, integration tests. More practically, the integration-tests
cmake target is conditioned on !NO_TESTS but the Kudu CLI is not.

So this patch moves the mini cluster code into a standalone module. I also
took the opportunity to place the code in a new "cluster" namespace.

Change-Id: I0437e281da5874016d9c1f1404a6de043bfb4088
Reviewed-on: http://gerrit.cloudera.org:8080/8173
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M CMakeLists.txt
M src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc
M src/kudu/benchmarks/tpch/tpch1.cc
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client-test.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/integration-tests/catalog_manager_tsk-itest.cc
M src/kudu/integration-tests/client-negotiation-failover-itest.cc
M src/kudu/integration-tests/client-stress-test.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/cluster_verifier.cc
M src/kudu/integration-tests/cluster_verifier.h
M src/kudu/integration-tests/consistency-itest.cc
M src/kudu/integration-tests/create-table-itest.cc
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/delete_tablet-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/exactly_once_writes-itest.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.cc
M src/kudu/integration-tests/external_mini_cluster-itest-base.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.h
M src/kudu/integration-tests/flex_partitioning-itest.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/integration-tests/internal_mini_cluster-itest-base.cc
M src/kudu/integration-tests/internal_mini_cluster-itest-base.h
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/log-rolling-itest.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/log_verifier.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/integration-tests/multidir_cluster-itest.cc
M src/kudu/integration-tests/open-readonly-fs-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/registration-test.cc
M src/kudu/integration-tests/security-faults-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/integration-tests/security-unknown-tsk-itest.cc
M src/kudu/integration-tests/table_locations-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_history_gc-itest.cc
M src/kudu/integration-tests/tablet_replacement-itest.cc
M src/kudu/integration-tests/test_workload.cc
M src/kudu/integration-tests/test_workload.h
M src/kudu/integration-tests/token_signer-itest.cc
M src/kudu/integration-tests/tombstoned_voting-imc-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/integration-tests/tombstoned_voting-stress-test.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/integration-tests/update_scan_delta_compact-test.cc
M src/kudu/integration-tests/version_migration-test.cc
M src/kudu/integration-tests/webserver-stress-itest.cc
A src/kudu/mini-cluster/CMakeLists.txt
R src/kudu/mini-cluster/external_mini_cluster-test.cc
R src/kudu/mini-cluster/external_mini_cluster.cc
R 

[kudu-CR] KUDU-2132 Error dropping columns renamed to old key column names

[Will Berkeley (Code Review)]

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

Change subject: KUDU-2132 Error dropping columns renamed to old key column names
..

KUDU-2132 Error dropping columns renamed to old key column names

Suppose a table has two columns 'a' and 'b', and 'a' is the primary
key. Consider the following sequence of alter table steps, done as
part of one alter table request:
1. Rename the key column 'a' to 'c'.
2. Rename 'b' to 'a', the previous name of the key column.
3. Drop 'a'.
This is a valid sequence, but previously we were rejecting it,
saying that a primary key column was being dropped, even though
that isn't the case.

The problem is that CatalogManager::ApplyAlterSchemaSteps
checked whether a column was a key column by checking by
name against the table's schema before any alter steps were
applied. The fix is to check against the schema with the
previous alter steps applied.

Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2
Reviewed-on: http://gerrit.cloudera.org:8080/8188
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
Reviewed-by: Dan Burkert 
---
M src/kudu/common/schema.h
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
4 files changed, 21 insertions(+), 1 deletion(-)

Approvals:
  Kudu Jenkins: Verified
  Adar Dembo: Looks good to me, but someone else must approve
  Dan Burkert: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2
Gerrit-Change-Number: 8188
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2132 Error dropping columns renamed to old key column names

[Dan Burkert (Code Review)]

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

Change subject: KUDU-2132 Error dropping columns renamed to old key column names
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2
Gerrit-Change-Number: 8188
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Mon, 02 Oct 2017 18:53:58 +
Gerrit-HasComments: No

[kudu-CR] tool: add cluster shell action

[Adar Dembo (Code Review)]

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

Change subject: tool: add cluster shell action
..


Patch Set 7:

(8 comments)

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

http://gerrit.cloudera.org:8080/#/c/7853/7/src/kudu/tools/kudu-tool-test.cc@89
PS7, Line 89: #include "kudu/tools/tool_action_common.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


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

http://gerrit.cloudera.org:8080/#/c/7853/7/src/kudu/tools/tool_action_common.h@39
PS7, Line 39: class faststring;
> warning: invalid case style for class 'faststring' [readability-identifier-
It's a forward declaration of an existing class...


http://gerrit.cloudera.org:8080/#/c/7853/7/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:

http://gerrit.cloudera.org:8080/#/c/7853/7/src/kudu/tools/tool_action_common.cc@38
PS7, Line 38: #include "kudu/client/client.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


http://gerrit.cloudera.org:8080/#/c/7853/7/src/kudu/tools/tool_action_common.cc@148
PS7, Line 148: using tserver::TabletServerAdminServiceProxy;
> warning: using decl 'TabletServerAdminServiceProxy' is unused [misc-unused-
This is used in template specializations in this file. If I remove it, the 
build fails.


http://gerrit.cloudera.org:8080/#/c/7853/7/src/kudu/tools/tool_action_common.cc@149
PS7, Line 149: using tserver::TabletServerServiceProxy;
> warning: using decl 'TabletServerServiceProxy' is unused [misc-unused-using
This is used in template specializations in this file. If I remove it, the 
build fails.


http://gerrit.cloudera.org:8080/#/c/7853/7/src/kudu/tools/tool_action_test.cc
File src/kudu/tools/tool_action_test.cc:

http://gerrit.cloudera.org:8080/#/c/7853/7/src/kudu/tools/tool_action_test.cc@76
PS7, Line 76: using std::cin;
> warning: using decl 'cin' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/7853/7/src/kudu/tools/tool_action_test.cc@77
PS7, Line 77: using std::cout;
> warning: using decl 'cout' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/7853/7/src/kudu/tools/tool_action_test.cc@78
PS7, Line 78: using std::endl;
> warning: using decl 'endl' is unused [misc-unused-using-decls]
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6
Gerrit-Change-Number: 7853
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 02 Oct 2017 18:49:46 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2132 Error dropping columns renamed to old key column names

[Adar Dembo (Code Review)]

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

Change subject: KUDU-2132 Error dropping columns renamed to old key column names
..


Patch Set 2: Code-Review+1

Will defer to Dan.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2
Gerrit-Change-Number: 8188
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Mon, 02 Oct 2017 18:47:34 +
Gerrit-HasComments: No

[kudu-CR] WIP [consensus] adding/removing NON VOTER members

[Alexey Serbin (Code Review)]

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

Change subject: WIP [consensus] adding/removing NON_VOTER members
..


Patch Set 2:

(2 comments)

Thank you for the review!

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

http://gerrit.cloudera.org:8080/#/c/8138/2/src/kudu/integration-tests/raft_consensus-itest.cc@416
PS2, Line 416: // Get number of source tablet copy sessions at the specified 
server.
> nit: leave one blank line above this, for consistency.
Done


http://gerrit.cloudera.org:8080/#/c/8138/2/src/kudu/integration-tests/raft_consensus-itest.cc@3242
PS2, Line 3242: TEST_F(RaftConsensusITest, AddNonVoterReplica) {
> I think this is a good addition, but that, ultimately we also need to add n
Yep, that's a good idea -- I'm working on it.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 02 Oct 2017 18:35:13 +
Gerrit-HasComments: Yes

[kudu-CR] WIP [consensus] adding/removing NON VOTER members

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: WIP [consensus] adding/removing NON_VOTER members
..

WIP [consensus] adding/removing NON_VOTER members

Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly.  Also, added new integration test:
  * RaftConsensusITest.AddNonVoterReplica
  * RaftConsensusITest.AddThenRemoveNonVoterReplica

WIP: Perhaps, I should continue gathering feedback on this patch
 and adding more tests.

Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/metadata.proto
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
9 files changed, 431 insertions(+), 58 deletions(-)


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

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

[kudu-CR] WIP [consensus] adding/removing NON VOTER members

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: WIP [consensus] adding/removing NON_VOTER members
..

WIP [consensus] adding/removing NON_VOTER members

Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly.  Also, added new integration test:
  * RaftConsensusITest.AddNonVoterReplica
  * RaftConsensusITest.AddThenRemoveNonVoterReplica

WIP: Perhaps, I should continue gathering feedback on this patch
 and adding more tests.

Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/metadata.proto
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
9 files changed, 430 insertions(+), 58 deletions(-)


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

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

[kudu-CR](branch-1.3.x) KUDU-2032 (part 2): propagate master hostnames into client

[Will Berkeley (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2032 (part 2): propagate master hostnames into client
..

KUDU-2032 (part 2): propagate master hostnames into client

This changes the client code to remember the user-specified master
addresses and propagate them into the creation of master proxies.

It's not possible to reproduce the necessary DNS configurations in a
minicluster test, but with this patch I am now able to use 'kudu perf
loadgen' against a Kerberized cluster even when my local krb5.conf has
rdns=false.

Change-Id: I0c3e6c5f6543a86173e242b04d3515f5ec69200d
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/integration-tests/external_mini_cluster.cc
5 files changed, 68 insertions(+), 45 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0c3e6c5f6543a86173e242b04d3515f5ec69200d
Gerrit-Change-Number: 8183
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR](branch-1.4.x) KUDU-2032 (part 2): propagate master hostnames into client

[Will Berkeley (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2032 (part 2): propagate master hostnames into client
..

KUDU-2032 (part 2): propagate master hostnames into client

This changes the client code to remember the user-specified master
addresses and propagate them into the creation of master proxies.

It's not possible to reproduce the necessary DNS configurations in a
minicluster test, but with this patch I am now able to use 'kudu perf
loadgen' against a Kerberized cluster even when my local krb5.conf has
rdns=false.

Change-Id: I4bd299f72907a0d9442dd3eb45fcbaa2de4f73ef
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/integration-tests/external_mini_cluster.cc
5 files changed, 58 insertions(+), 39 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: branch-1.4.x
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4bd299f72907a0d9442dd3eb45fcbaa2de4f73ef
Gerrit-Change-Number: 8186
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR](branch-1.3.x) KUDU-2032 (part 2): propagate master hostnames into client

[Will Berkeley (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2032 (part 2): propagate master hostnames into client
..

KUDU-2032 (part 2): propagate master hostnames into client

This changes the client code to remember the user-specified master
addresses and propagate them into the creation of master proxies.

It's not possible to reproduce the necessary DNS configurations in a
minicluster test, but with this patch I am now able to use 'kudu perf
loadgen' against a Kerberized cluster even when my local krb5.conf has
rdns=false.

Change-Id: I0c3e6c5f6543a86173e242b04d3515f5ec69200d
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/integration-tests/external_mini_cluster.cc
5 files changed, 68 insertions(+), 43 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0c3e6c5f6543a86173e242b04d3515f5ec69200d
Gerrit-Change-Number: 8183
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2032 (part 2): propagate master hostnames into client

[Will Berkeley (Code Review)]

Will Berkeley has abandoned this change. ( http://gerrit.cloudera.org:8080/8193 
)

Change subject: KUDU-2032 (part 2): propagate master hostnames into client
..


Abandoned

Whoops. Wrong branch.
--
To view, visit http://gerrit.cloudera.org:8080/8193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I4bd299f72907a0d9442dd3eb45fcbaa2de4f73ef
Gerrit-Change-Number: 8193
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2032 (part 2): propagate master hostnames into client

[Will Berkeley (Code Review)]

Will Berkeley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8193


Change subject: KUDU-2032 (part 2): propagate master hostnames into client
..

KUDU-2032 (part 2): propagate master hostnames into client

This changes the client code to remember the user-specified master
addresses and propagate them into the creation of master proxies.

It's not possible to reproduce the necessary DNS configurations in a
minicluster test, but with this patch I am now able to use 'kudu perf
loadgen' against a Kerberized cluster even when my local krb5.conf has
rdns=false.

Change-Id: I4bd299f72907a0d9442dd3eb45fcbaa2de4f73ef
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/integration-tests/external_mini_cluster.cc
5 files changed, 58 insertions(+), 39 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4bd299f72907a0d9442dd3eb45fcbaa2de4f73ef
Gerrit-Change-Number: 8193
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR](branch-1.4.x) KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies

[Will Berkeley (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC 
proxies
..

KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies

This modifies the constructor of RPC proxies (generated and otherwise)
to take the remote hostname in addition to the existing resolved
Sockaddr parameter. The hostname is then passed into the ConnectionId
object, and plumbed through to the SASL client in place of the IP
address that was used previously.

The patch changes all of the construction sites of Proxy to fit the new
interface. In most of the test cases, we don't have real hostnames, so
we just use the dotted-decimal string form of the remote Sockaddr, which
matches the existing behavior.

In the real call sites, we have actual host names typically specified by
the user, and in those cases we'll need to pass those into the proxy. In
a few cases, they were conveniently available in the same function that
creates the proxy. In others, they are relatively far away, so this
patch just uses the dotted-decimal string and leaves TODOs.

In the case that Kerberos is not configured, this change should have no
effect since the hostname is ignored by SASL "plain". In the case that
Kerberos is configured with 'rdns=true', they also have no effect,
because the krb5 library will resolve and reverse the hostname from the
IP as it did before. In the case that 'rdns=false', this moves us one
step closer to fixing KUDU-2032 by getting a hostname into the SASL
library.

I verified that, if I set 'rdns = false' on a Kerberized client, I'm now
able to run  'kudu master status ' successfully where it would not
before. This tool uses a direct proxy instantiation where the hostname
was easy to plumb in. 'kudu table list ' still does not work because
it uses the client, which wasn't convenient to plumb quite yet.

Given that this makes incremental improvement towards fixing the issue
without any regression, and is already a fairly wide patch, I hope to
commit this and then address the remaining plumbing in a separate patch.

Change-Id: I6d58abbe6a5d9fc524e54e4182402c5326a60d82
---
M src/kudu/client/client-internal.cc
M src/kudu/client/master_rpc.cc
M src/kudu/client/meta_cache.cc
M src/kudu/consensus/consensus_peers.cc
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/mini_cluster.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M src/kudu/rpc/connection.h
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/protoc-gen-krpc.cc
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-bench.cc
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_server_test_util.cc
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
42 files changed, 217 insertions(+), 164 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: branch-1.4.x
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6d58abbe6a5d9fc524e54e4182402c5326a60d82
Gerrit-Change-Number: 8185
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2132 Error dropping columns renamed to old key column names

[Will Berkeley (Code Review)]

Hello Kudu Jenkins, Adar Dembo,

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

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

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

Change subject: KUDU-2132 Error dropping columns renamed to old key column names
..

KUDU-2132 Error dropping columns renamed to old key column names

Suppose a table has two columns 'a' and 'b', and 'a' is the primary
key. Consider the following sequence of alter table steps, done as
part of one alter table request:
1. Rename the key column 'a' to 'c'.
2. Rename 'b' to 'a', the previous name of the key column.
3. Drop 'a'.
This is a valid sequence, but previously we were rejecting it,
saying that a primary key column was being dropped, even though
that isn't the case.

The problem is that CatalogManager::ApplyAlterSchemaSteps
checked whether a column was a key column by checking by
name against the table's schema before any alter steps were
applied. The fix is to check against the schema with the
previous alter steps applied.

Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2
---
M src/kudu/common/schema.h
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
4 files changed, 21 insertions(+), 1 deletion(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2
Gerrit-Change-Number: 8188
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2132 Error dropping columns renamed to old key column names

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8188 )

Change subject: KUDU-2132 Error dropping columns renamed to old key column names
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8188/1/src/kudu/common/schema.h
File src/kudu/common/schema.h:

http://gerrit.cloudera.org:8080/#/c/8188/1/src/kudu/common/schema.h@903
PS1, Line 903: return col - cols_.begin() < num_key_columns_;
> This seems a little magical to me, though maybe it's because I'm easily con
Done


http://gerrit.cloudera.org:8080/#/c/8188/1/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:

http://gerrit.cloudera.org:8080/#/c/8188/1/src/kudu/integration-tests/alter_table-test.cc@2107
PS1, Line 2107:   gscoped_ptr 
table_alterer(client_->NewTableAlterer(kTableName));
> Nit: unique_ptr for new code?
Done


http://gerrit.cloudera.org:8080/#/c/8188/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8188/1/src/kudu/master/catalog_manager.cc@1761
PS1, Line 1761: if (builder.is_key_column(step.drop_column().name())) {
> Is my understanding correct that L1726 initializes the builder with the cur
That's right.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie27f7aa936e6dfcf6c8b0b7a701784b0683680f2
Gerrit-Change-Number: 8188
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Mon, 02 Oct 2017 16:47:50 +
Gerrit-HasComments: Yes

[kudu-CR] [webui] Allow custom response codes and headers

[Will Berkeley (Code Review)]

Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,

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

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

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

Change subject: [webui] Allow custom response codes and headers
..

[webui] Allow custom response codes and headers

Previously, the path handlers used to implement pages in the web ui
could only return 200 OK and could not set any headers, as these two
aspects of the HTTP response were handled in the underlying webserver
code. This patch introduces WebResponse and PrerenderedWebResponse
structs that wrap and replace the 'output' EasyJson and ostringstream
pointers, respectively, used before, and which has fields for the
response code and additional headers.

The ability to add headers isn't currently used, but it's nice to have.
The response codes are adjusted where necessary to match what one
would expect, e.g. navigating to /tablet?id=foo returns
404.

Change-Id: I9ff890785eeb2df3eed9e7c54d0daf760c8b3924
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/default-path-handlers.cc
M src/kudu/server/pprof-path-handlers.cc
M src/kudu/server/rpcz-path-handler.cc
M src/kudu/server/tracing-path-handlers.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver.h
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/tserver/tserver-path-handlers.h
M src/kudu/util/thread.cc
M src/kudu/util/web_callback_registry.h
12 files changed, 247 insertions(+), 132 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9ff890785eeb2df3eed9e7c54d0daf760c8b3924
Gerrit-Change-Number: 8141
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2159 Add metric for upserts converted to updates

[Will Berkeley (Code Review)]

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

Change subject: KUDU-2159 Add metric for upserts converted to updates
..

KUDU-2159 Add metric for upserts converted to updates

This adds a metric tracking the number of successful upsert operations
that were applied as updates, per tablet, counting since server start.

Change-Id: Iefdee21fbfdfd3c9c8dca4c06d635b89bcf15d72
Reviewed-on: http://gerrit.cloudera.org:8080/8177
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley 
---
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_metrics.cc
M src/kudu/tablet/tablet_metrics.h
4 files changed, 19 insertions(+), 3 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Will Berkeley: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iefdee21fbfdfd3c9c8dca4c06d635b89bcf15d72
Gerrit-Change-Number: 8177
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2159 Add metric for upserts converted to updates

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8177 )

Change subject: KUDU-2159 Add metric for upserts converted to updates
..


Patch Set 2: Code-Review+2

Forwarding Todd's +2 since the test failures seemed to be unrelated 
Python/Jenkins flake.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iefdee21fbfdfd3c9c8dca4c06d635b89bcf15d72
Gerrit-Change-Number: 8177
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Mon, 02 Oct 2017 16:30:40 +
Gerrit-HasComments: No

[kudu-CR] external mini cluster: don't pipe daemon subprocess stdout

[Adar Dembo (Code Review)]

Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/8187 )

Change subject: external_mini_cluster: don't pipe daemon subprocess stdout
..


Removed reviewer Kudu Jenkins with the following votes:

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Iddd8d78cd085975821207d324ea2fd5365a2c2ba
Gerrit-Change-Number: 8187
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] external mini cluster: don't pipe daemon subprocess stdout

[Adar Dembo (Code Review)]

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

Change subject: external_mini_cluster: don't pipe daemon subprocess stdout
..


Patch Set 2: Verified+1

Overriding Jenkins, there was an unrelated TSAN failure for which I filed 
KUDU-2165.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iddd8d78cd085975821207d324ea2fd5365a2c2ba
Gerrit-Change-Number: 8187
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 02 Oct 2017 16:08:52 +
Gerrit-HasComments: No

[kudu-CR] periodic: add one-shot timers

[Adar Dembo (Code Review)]

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

Change subject: periodic: add one-shot timers
..


Patch Set 3: Verified+1

Overriding Jenkins, got clock synchronization errors in some of the distributed 
tests.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Gerrit-Change-Number: 8130
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 02 Oct 2017 16:01:30 +
Gerrit-HasComments: No

[kudu-CR] subprocess: some cosmetic changes

[Dan Burkert (Code Review)]

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

Change subject: subprocess: some cosmetic changes
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d81dee93817c560c1237114fb515facbdb06b1d
Gerrit-Change-Number: 8157
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 02 Oct 2017 15:26:34 +
Gerrit-HasComments: No

[kudu-CR] mini-cluster: new module for the mini cluster implementations

[Dan Burkert (Code Review)]

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

Change subject: mini-cluster: new module for the mini cluster implementations
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0437e281da5874016d9c1f1404a6de043bfb4088
Gerrit-Change-Number: 8173
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 02 Oct 2017 15:26:10 +
Gerrit-HasComments: No