[kudu-CR] Remove pessimizing std::move() in return statement

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Remove pessimizing std::move() in return statement
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ida46588398ca5f5982cb86aa03825530565c0923
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tool: basic integration test

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: tool: basic integration test
..


Patch Set 5:

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

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

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

[kudu-CR] tool: basic integration test

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: tool: basic integration test
..

tool: basic integration test

So far all it does is spot check some help pages, but in the future we
should augment it to test functionality too. For now that's not a big deal
because every tool function is covered in either master_migration-itest or
master_failover-itest.

Change-Id: Ib386882c1874e987d5824cfe742cc86627cd9eaa
---
M build-support/dist_test.py
M src/kudu/tools/CMakeLists.txt
A src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/test_macros.h
4 files changed, 188 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib386882c1874e987d5824cfe742cc86627cd9eaa
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tool: add ksck

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: tool: add ksck
..


Patch Set 3:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iad9aaffa5c0c77080dcaeb2cdfa572dcbeeb1407
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-687: add replication factor to KuduTable

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-687: add replication factor to KuduTable
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If76ec6c3001e78a31517991d7432f79d4645fccc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

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

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 4:

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

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

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

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

[Dinesh Bhat (Code Review)]

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

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

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

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

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

KUDU-1534 : Added software_version to ListMasters RPC

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

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

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


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

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

[kudu-CR] Add AvroKuduEventProducer to Kudu-Flume integration

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Add AvroKuduEventProducer to Kudu-Flume integration
..


Patch Set 1:

(6 comments)

This is good stuff.

http://gerrit.cloudera.org:8080/#/c/4034/1/java/kudu-flume-sink/pom.xml
File java/kudu-flume-sink/pom.xml:

Line 74:   hadoop-client
> To be honest, I wasn't sure about this, so I copied off of the map/reduce i
Yeah, I would also like to see this dep shaded.


http://gerrit.cloudera.org:8080/#/c/4034/1/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/AvroKuduEventProducer.java
File 
java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/AvroKuduEventProducer.java:

PS1, Line 52: producer.schema
> Done
Flume sinks typically use camelCaps like schemaPath for configuration options.


Line 89:   public void configure(ComponentConfiguration conf) {
Hrm. You sure you had to override this?


Line 93:   public void initialize(Event event, KuduTable table) {
> Yup, it's weird.
I don't think there are many users yet and I'd be OK with changing it pre-1.0.

We can change it in a follow-up patch though.

What I think would be better would be KuduEventProducer.initialize(KuduTable 
table) called once per KuduSink.start() -- after configure() is called. Then 
change KuduEventProducer.getOperations() to take Event as a parameter.

To be honest, I think KuduEventProducer is also a bad name. This thing reads 
Flume Events and produces Kudu Operations. Maybe a better name would be 
KuduOperationsProducer.


Line 133: row.addBoolean(name, (boolean) value);
> We could improve the API, but it would mean changing KuduSink and the KuduE
If we use a single schema for the sink then we could probably do the validation 
elsewhere. However I think it would be useful to support per-Event schema URLs 
as well, or maybe instead of this. Here is what the Kite sink does. They allow 
putting an avro schema URL in each event using the header key 
"flume.avro.schema.url". See 
https://flume.apache.org/FlumeUserGuide.html#kite-dataset-sink

They also allow an Avro JSON literal in each Event as 
"flume.avro.schema.literal" but I'm not sure how popular that is since it takes 
up so much space.


http://gerrit.cloudera.org:8080/#/c/4034/1/java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduEventProducerTest.java
File 
java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduEventProducerTest.java:

Line 149: parameters.put(KuduSinkConfigurationConstants.PRODUCER, 
"org.apache.kudu.flume.sink.AvroKuduEventProducer");
nit: long line


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6715df72e447e72f4801a2e026f6840d09b401e1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] tool: add ksck

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: tool: add ksck
..


Patch Set 2: Verified+1

Overriding Jenkins, known flake.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iad9aaffa5c0c77080dcaeb2cdfa572dcbeeb1407
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tool: basic integration test

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: tool: basic integration test
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4058/4/src/kudu/util/test_macros.h
File src/kudu/util/test_macros.h:

Line 89:  matched = true; \
> nit:tabs
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib386882c1874e987d5824cfe742cc86627cd9eaa
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Remove pessimizing std::move() in return statement

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: Remove pessimizing std::move() in return statement
..


Remove pessimizing std::move() in return statement

This change fixes this Clang warning:

[334/463] Building CXX object 
src/kudu/consensus/CMakeFiles/raft_consensus_quorum-test.dir/raft_consensus_quorum-test.cc.o
../../src/kudu/consensus/raft_consensus_quorum-test.cc:534:12: warning: moving 
a local object in a return statement prevents copy elision [-Wpessimizing-move]
return std::move(cmeta);
   ^
../../src/kudu/consensus/raft_consensus_quorum-test.cc:534:12: note: remove 
std::move call here
return std::move(cmeta);
   ^~ ~
1 warning generated.

Change-Id: Ida46588398ca5f5982cb86aa03825530565c0923
Reviewed-on: http://gerrit.cloudera.org:8080/4122
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
---
M src/kudu/consensus/raft_consensus_quorum-test.cc
1 file changed, 1 insertion(+), 1 deletion(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ida46588398ca5f5982cb86aa03825530565c0923
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Rolling-upgrades compat for changing TSRegistrationPB
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4062/5/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:

PS5, Line 60: static bool HostPortPBsEqual(const 
::google::protobuf::RepeatedPtrField<::kudu::HostPortPB>& pb1,
:  const 
::google::protobuf::RepeatedPtrField<::kudu::HostPortPB>& pb2)
> Not related to your change, but can you get away with HostPortPB instead of
Done


http://gerrit.cloudera.org:8080/#/c/4062/5/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:

PS5, Line 55:   bool Equals(const HostPort& other) const {
: return port_ == other.port_ && host_ == other.host_;
:   }
> Since this is new, consider dropping it in favor of just operator==, which 
Done


Line 59:   bool operator==(const HostPort& other) const {
> Google style guide says const binary operators should be defined as free fu
Hmm, didn't know about that one. ODR requires me to rejigger a couple things. 
Done.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Actually support downgrade to version that has LocalConsensus

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Actually support downgrade to version that has LocalConsensus
..


Patch Set 4:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Rolling-upgrades compat for changing TSRegistrationPB
..


Patch Set 6:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Rolling-upgrades compat for changing TSRegistrationPB
..

Rolling-upgrades compat for changing TSRegistrationPB

This should allow for adding fields to TSRegistrationPB.
Only the host/port is checked.

Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
---
M src/kudu/master/master-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
4 files changed, 70 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

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

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

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


Patch Set 4:

(14 comments)

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

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

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


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

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


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

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


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

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


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

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


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

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


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


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

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


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

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


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


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

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


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


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

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


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


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

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

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

[Dinesh Bhat (Code Review)]

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

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

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

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

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

KUDU-1534 : Added software_version to ListMasters RPC

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

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

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


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

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

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

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 5:

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

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

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

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

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 5:

(3 comments)

Just a couple of nits but this is otherwise good, though we still need to find 
out whether this maintains backwards compatibility.

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

PS5, Line 82: This entails RPC/HTTP addresses and software version on
: // the servers and used in the registation/heartbeat phase.
Nit: how about "Basic properties common to both masters and tservers. 
Guaranteed not to change unless the server is restarted."

- "entails" is an odd word choice.
- There's no point to listing all of the fields; that list is self-explanatory, 
plus it'll become obsolete when a new field is added.
- I don't think this is the right place to explain how/when the message is 
used. Better to find the usage sites and comment on them (though I think 
they're already commented).


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

PS5, Line 511: std::string MasterPathHandlers::RegistrationToHtml(
 : const ServerRegistrationPB& reg,
 : const std::string& link_text) const {
Nit: don't need to qualify string with std.


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

Line 34: class ServerRegistrationPB;
Nit: should come before Sockaddr alphabetically.


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

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

[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Rolling-upgrades compat for changing TSRegistrationPB
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

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

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 22:

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

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

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

[kudu-CR] Add AvroKuduEventProducer to Kudu-Flume integration

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add AvroKuduEventProducer to Kudu-Flume integration
..

Add AvroKuduEventProducer to Kudu-Flume integration

This adds a KuduEventProducer that accepts events with Avro-encoded
bodies and allows the KuduSink to push them to Kudu. It only accepts
schemas of Record type that hold values of types compatible with
Kudu's types. The schema is provided to the EventProducer ahead of
time or in the event header, as a path.

Change-Id: I6715df72e447e72f4801a2e026f6840d09b401e1
---
M java/kudu-flume-sink/pom.xml
A 
java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/AvroKuduEventProducer.java
A 
java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduEventProducerTest.java
A java/kudu-flume-sink/src/test/resources/testAvroEventProducer.avsc
A 
java/kudu-flume-sink/src/test/resources/testCustomSchemaAcroKuduEventProducer.avsc
5 files changed, 477 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6715df72e447e72f4801a2e026f6840d09b401e1
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

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

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

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

KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Implemented AUTO_FLUSH_BACKGROUND for the Kudu C++ client library.
In AUTO_FLUSH_BACKGROUND mode,
the KuduSession::Apply() method blocks if total amount of data
for pending operations reaches the buffer size limit.  The limit
on the buffer size can be set by the
KuduSession::SetMutationBufferSpace() method.

The background flush logic checks whether at least one of the
following two conditions is satisfied to determine whether it's time
to flush the accumulated write operations:
  * The over-the-watermark criterion: check whether the total size of the
freshly submitted (i.e. not-yet-scheduled-for-flush) write operations
is over the threshold.  The threshold can be set as a percentage
of the total buffer size using the
KuduSession::SetMutationBufferFlushWatermark() method.
  * The maximum wait time criterion: check whether the current batch
of operations has been accumulating for more than the maximum
wait time.  The maximum wait time can be specified in milliseconds
using the KuduSession::SetMutationBufferFlushInterval() method.
A KuduSession object uses RPC messenger's thread pool to monitor
batches' maximum wait time.

This change also addresses the following JIRA issue:
  KUDU-1376 KuduSession::SetMutationBufferSpace is not defined

Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
---
M python/kudu/tests/test_client.py
M src/kudu/client/batcher.cc
M src/kudu/client/batcher.h
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/client/write_op.cc
M src/kudu/client/write_op.h
11 files changed, 1,445 insertions(+), 183 deletions(-)


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

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

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

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 7:

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

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

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

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

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

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


Patch Set 6:

(2 comments)

TFTR Adar, will follow the JIRA suggestions for more testing tonight.

http://gerrit.cloudera.org:8080/#/c/4099/6/src/kudu/integration-tests/create-table-itest.cc
File src/kudu/integration-tests/create-table-itest.cc:

PS6, Line 124:   const int kNumServers = 3;
 :   const int kNumTablets = 3;
> Why did you change these values?
Oops, reverted. that shouldn't have gone in this branch. thanks.


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

PS6, Line 513: std::string
> Missed one.
Hmmm... if you see this source(as well as header file), all the routines use 
std:;string. I would rather be consistent here although I know it's a moot 
namespace prefix here. I only removed return value to be string instead of 
std::string(again something followed by all other routines). We can prolly fix 
this in a different change may be?


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

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

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

[Dinesh Bhat (Code Review)]

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

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

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

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

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

KUDU-1534 : Added software_version to ListMasters RPC

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

As a precaution, testing is performed to make sure downgrade/upgrade
are compatible with the message formats being consolidated. Testing
details are attached under JIRA.

Also added registration string to 'Masters' webui.
Sample output is at:
https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf

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


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

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

[kudu-CR] ksck: colorize and clean up output

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: ksck: colorize and clean up output
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icc5196d63cbcbcbb2a9aba1ff17377b678c104bd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] tools: wrap descriptions

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: tools: wrap descriptions
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie63b72e1e1a3479819730c348f5a0f00a7164d02
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] ksck: colorize and clean up output

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Adar Dembo,

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

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

to review the following change.

Change subject: ksck: colorize and clean up output
..

ksck: colorize and clean up output

Dan and I were looking at some ksck output earlier and found it somewhat
hard to read. This colorizes the output and also cleans up the format to
be slightly less messy. The output now goes to stdout instead of stderr
as well, so it's easier to pipe to a file or 'less', which is a common
use case.

Example output:

Tablet 0e4e79de2c2547c091779b13735d2b73 of table 'my_table' is 
under-replicated: 1 replica(s) not RUNNING
  63c4785c3b6f47f5a1c23ffbf6e52b71 (ts-023.foo.com:7050): RUNNING
  cd20c68cd23c4cf986eda0936a5691cc (ts-004.foo.com:7050): RUNNING [LEADER]
  5edf82f0516b4897b3a7991a7e67d71c (ts-028.foo.com:7050): bad state
State:   NOT_STARTED
Data state:  TABLET_DATA_COPYING
Last status: TabletCopy: Downloading block 4611685629730158289 (3491/9569)

Change-Id: Icc5196d63cbcbcbb2a9aba1ff17377b678c104bd
---
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/tool_action_cluster.cc
5 files changed, 178 insertions(+), 142 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Icc5196d63cbcbcbb2a9aba1ff17377b678c104bd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: java: fix leak of TabletClient objects in client2tablets map
..


Patch Set 2:

ignore the re-push, accidentally rebased

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP: ts-cli: add commands to trigger step-down and copying tablets

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: ts-cli: add commands to trigger step-down and copying 
tablets
..

WIP: ts-cli: add commands to trigger step-down and copying tablets

With these I was able to recover a "stuck" tablet on a test cluster.

Change-Id: I94fd59c307d4f5d921d5975a19ca1f3116c6ff70
---
M src/kudu/tools/ts-cli.cc
1 file changed, 93 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94fd59c307d4f5d921d5975a19ca1f3116c6ff70
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] tool: basic integration test

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: tool: basic integration test
..


tool: basic integration test

So far all it does is spot check some help pages, but in the future we
should augment it to test functionality too. For now that's not a big deal
because every tool function is covered in either master_migration-itest or
master_failover-itest.

Change-Id: Ib386882c1874e987d5824cfe742cc86627cd9eaa
Reviewed-on: http://gerrit.cloudera.org:8080/4058
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M build-support/dist_test.py
M src/kudu/tools/CMakeLists.txt
A src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/test_macros.h
4 files changed, 188 insertions(+), 3 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib386882c1874e987d5824cfe742cc86627cd9eaa
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-687: add replication factor to KuduTable

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-687: add replication factor to KuduTable
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4123/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

Line 3262:   for (int i : { 1, 3, 5, 7, 9 }) {
hrm, how does this work if there are not 9 tablet servers in the minicluster?


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

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

[kudu-CR] Predicate evaluation pushdown

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Predicate evaluation pushdown
..


Patch Set 5:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31e4cce21e99f63b089d7c84410af8ed914cb576
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

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

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 22:

(18 comments)

Did another pass. I'm still blown away by the overall complexity; I wonder if 
reducing the number of locks will help here, or making other infrastructure 
improvements (e.g. allowing reactor tasks to be canceled).

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

Line 189:   // The time when the very first operation was added into the 
batcher.
Should note that this is protected by lock_. While you're there, can you make 
sure all other protected fields are annotated as such?


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

PS22, Line 1037: current
Nit: "the current"


PS22, Line 1143: when
Nit: "only when", right?


PS22, Line 1180: called
Nit: "called the"


PS22, Line 1183: taken care
Nit: "taken care of"


PS22, Line 1184: Once flushing of the prior
   :   /// mutation buffer is initiated, the new one is created and 
set current
   :   /// to accommodate incoming write operations, limit 
permitting.
Nit: "Upon flushing the current mutation buffer, a new buffer is created to 
replace it, provided the limit is not exceeded."


Line 1188:   /// The default and the minimum setting for this parameter is 2 
(two).
Hmm, technically couldn't the minimum be 1? If set to 1, an Apply() with an 
outstanding background flush ought to block (and an ApplyAsync() should return 
some sort of backpressure error). Though I guess you'd need that kind of 
behavior when the limit is reached regardless of whether it's 1 or higher.


PS22, Line 1191: the
Nit: drop 'the'


PS22, Line 1201: implicitly amended
   :   ///   as if it were 0.
Nit: "implicitly set to 0".


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

PS22, Line 80: the
Nit: double the


Line 130:   buffer_bytes_limit_ = size;
Likewise, I don't see why this needs to be protected.


Line 145:   buffer_watermark_pct_ = watermark_pct;
Why does this need to be protected? It's only ever accessed by the writer 
thread (not a reactor thread).


PS22, Line 191:   Synchronizer s;
  :   KuduStatusMemberCallback ksmcb(, 
::StatusCB);
  :   NextBatcher(session, BATCHER_WATERMARK_NON_EMPTY, , 
BLOCK);
  :   RETURN_NOT_OK(s.Wait());
Why do we need to use the synchronizer to wait on this batch if, below, we wait 
until total_bytes_used_ reaches 0? Is it just to report errors? Would the error 
collector usage below capture that?


PS22, Line 222: relevent
Nit: relevant


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

PS22, Line 104:   // On successful return, the output flush_mode parameter
  :   // is set to the effective flush mode.
I thought we no longer needed the output parameter for flush mode?

Oh, is this because we read flush_mode_ from the flusher task, which means we 
need to protect it with something, or make it an atomic type of some kind? And 
so you'd rather pass back the flush mode from ApplyWriteOp() to its caller to 
avoid taking the lock a second time?

If so, it seems like that's working around the lack of "canceling" a scheduled 
task. That is, if we could cancel the flusher task (e.g. when the flush mode 
changes away from AUTO_BACKGROUND_FLUSH), it'd never need to read the flush 
mode. I wonder how much complexity is involved with adding such cancellation 
behavior; we've wanted it elsewhere and it would certainly simplify the flusher 
task considerably.


PS22, Line 142:   
Nit: looks like there are two spaces here. Below too.


PS22, Line 147: BATCHER_WATERMARK_NON_EMPTY
Our convention for naming class constants like these "kCamelCaseFoo". We 
reserve ALL_UPPER_CASE for macros.


PS22, Line 158:   // A dedicated lock to prevent multiple threads executing the 
ApplyWriteOp()
  :   // methods simultaneously. Should not be used for any other 
purpose.
I don't understand; we've expressly forbidden multiple Apply() threads. So why 
bother to do this? It's just another lock to maintain; if this is to prevent 
people from shooting themselves in the foot, I'd rather remove it and simplify.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
Gerrit-PatchSet: 22
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 

[kudu-CR] master: include TS address in log messages

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Adar Dembo,

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

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

to review the following change.

Change subject: master: include TS address in log messages
..

master: include TS address in log messages

When looking at master logs, it's quite annoying to have to translate
back from UUIDs to actual hostnames, since the operator typically wants
to ssh into that node to look at logs, etc.

This patch adds TSDescriptor::ToString() and calls it from all the
points in CatalogManager where log messages refer to an individual
server.

This also adds validation that TS registrations must include at least
one HTTP and one RPC address. This has always been the case but wasn't
verified.

Change-Id: Ic55fa7e818a115de70f9fc6aca12581c3b4779c7
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M src/kudu/master/ts_manager.cc
4 files changed, 45 insertions(+), 35 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic55fa7e818a115de70f9fc6aca12581c3b4779c7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] master: include TS address in log messages

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: master: include TS address in log messages
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic55fa7e818a115de70f9fc6aca12581c3b4779c7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-687: add replication factor to KuduTable

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-687: add replication factor to KuduTable
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4123/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

Line 3262:   for (int i : { 1, 3, 5, 7, 9 }) {
> hrm, how does this work if there are not 9 tablet servers in the minicluste
ClientTest::CreateTable() is magical; it adds enough tservers to satisfy the 
table's replication factor.


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

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

[kudu-CR] KUDU-687: add replication factor to KuduTable

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: KUDU-687: add replication factor to KuduTable
..


KUDU-687: add replication factor to KuduTable

This is generally useful, and necessary if ksck is to use the C++ client.

While I was at it, I reduced the use of a second table in client-test to
only those tests that actually needed it.

Change-Id: If76ec6c3001e78a31517991d7432f79d4645fccc
Reviewed-on: http://gerrit.cloudera.org:8080/4123
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table-internal.cc
M src/kudu/client/table-internal.h
7 files changed, 65 insertions(+), 26 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: If76ec6c3001e78a31517991d7432f79d4645fccc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-687: add replication factor to KuduTable

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-687: add replication factor to KuduTable
..


Patch Set 1: Code-Review+2

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

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

[kudu-CR] Rolling-upgrades compat for changing TSRegistrationPB

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: Rolling-upgrades compat for changing TSRegistrationPB
..


Rolling-upgrades compat for changing TSRegistrationPB

This should allow for adding fields to TSRegistrationPB.
Only the host/port is checked.

Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Reviewed-on: http://gerrit.cloudera.org:8080/4062
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/master/master-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
4 files changed, 70 insertions(+), 1 deletion(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I794517d200b1c9ad74b3897b17aa808d68d7924b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Predicate evaluation pushdown

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Predicate evaluation pushdown
..


Patch Set 6:

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

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

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

[kudu-CR] Predicate evaluation pushdown

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Predicate evaluation pushdown
..

Predicate evaluation pushdown

The premise of this patch is to avoid the excessive use of CPU when
evaluating column predicates in specific cases. Dictionary blocks,
for instance, can evaluate predicates by comparing codewords (UINT32)
rather than doing string comparisons.

See the performance doc for a look into the performance differences
for dictionary encoding and plain encoding:
https://github.com/anjuwong/kudu/blob/pred-pushdown/docs/decoder-eval-perf.md

See the design-doc for predicate-eval-pushdown for a brief overview of the
considered implementations:
https://github.com/anjuwong/kudu/blob/sorted-dict-block/docs/design-docs/predicate-eval-pushdown.md

This patch uses the predicate-set approach using a bitmap.

Change-Id: I31e4cce21e99f63b089d7c84410af8ed914cb576
---
M src/kudu/cfile/binary_dict_block.cc
M src/kudu/cfile/binary_dict_block.h
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/cfile/binary_plain_block.h
M src/kudu/cfile/binary_prefix_block.h
M src/kudu/cfile/block_encodings.h
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/cfile/cfile_util.cc
M src/kudu/cfile/gvint_block.h
M src/kudu/cfile/plain_bitmap_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
A src/kudu/common/column_eval_context.h
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/generic_iterators-test.cc
M src/kudu/common/generic_iterators.cc
M src/kudu/common/generic_iterators.h
M src/kudu/common/iterator.h
M src/kudu/common/rowblock.h
M src/kudu/common/schema.h
M src/kudu/common/types.h
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tablet/cfile_set-test.cc
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/delta_applier.cc
M src/kudu/tablet/delta_applier.h
M src/kudu/tablet/delta_iterator_merger.cc
M src/kudu/tablet/delta_iterator_merger.h
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
A src/kudu/tablet/tablet-decoder-eval-test.cc
M src/kudu/tablet/tablet-test-util.h
41 files changed, 943 insertions(+), 124 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I31e4cce21e99f63b089d7c84410af8ed914cb576
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

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

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 22:

(1 comment)

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

PS22, Line 191:   Synchronizer s;
  :   KuduStatusMemberCallback ksmcb(, 
::StatusCB);
  :   NextBatcher(session, BATCHER_WATERMARK_NON_EMPTY, , 
BLOCK);
  :   RETURN_NOT_OK(s.Wait());
> OK, I see the point now.
Cool. So regardless of what change you do or don't make to NextBatcher(), could 
you add a comment here explaining the various events we're waiting on?

Separately, could you look into splitting NextBatcher()'s responsibilities? 
Naively I think it'd simplify things, but it might be one of those things that 
you can only really evaluate once you're staring at the new code.


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

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

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

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 6:

(2 comments)

I left you some more comments in the JIRA regarding the testing.

http://gerrit.cloudera.org:8080/#/c/4099/6/src/kudu/integration-tests/create-table-itest.cc
File src/kudu/integration-tests/create-table-itest.cc:

PS6, Line 124:   const int kNumServers = 3;
 :   const int kNumTablets = 3;
Why did you change these values?


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

PS6, Line 513: std::string
Missed one.


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

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

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

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

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


Patch Set 22:

(1 comment)

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

PS22, Line 191:   Synchronizer s;
  :   KuduStatusMemberCallback ksmcb(, 
::StatusCB);
  :   NextBatcher(session, BATCHER_WATERMARK_NON_EMPTY, , 
BLOCK);
  :   RETURN_NOT_OK(s.Wait());
> Cool. So regardless of what change you do or don't make to NextBatcher(), c
Sure, I'll add those comments.

And just another thought to justify separating the responsibilities of the 
NextBatcher() method (most likely you have spotted that already):  it will 
allow more robust flushes in case if maximum number of batchers is reached.  
This is because NextBatch() in blocking mode first waits until it's possible to 
allocate a new batcher and only after that initiates flushing of the 
corresponding batcher.


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

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

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

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 22:

(2 comments)

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

Line 1188:   /// The default and the minimum setting for this parameter is 2 
(two).
> Yes, technically the minimum could be 1.
OK. If it simplifies the implementation, let's keep the minimum at 2. We can 
always remove the restriction later.


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

PS22, Line 191:   Synchronizer s;
  :   KuduStatusMemberCallback ksmcb(, 
::StatusCB);
  :   NextBatcher(session, BATCHER_WATERMARK_NON_EMPTY, , 
BLOCK);
  :   RETURN_NOT_OK(s.Wait());
> NextBatcher() not only flushes the current batcher, but it also allocates a
I wasn't asking about NextBatcher()'s wait; I was asking about s.Wait(). To 
help answer the question, here are all of the waits going on:

1. BLOCK in NextBatcher(). This ensures that a new batcher has become available.
2. s.Wait(). This ensures that the batcher we've flushed has finished flushing.
3. total_bytes_used_ == 0. This ensures that any other batchers in flight have 
finished flushing.

Do we need all of these? Naively, I think waiting for total_bytes_used_ to 
reach 0 is a superset of s.Wait(), no?

Moreover, don't we only need to get (and wait for) a new batcher on the next 
Apply()/ApplyAsync(), rather than here?


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

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

[kudu-CR] docs: update installation with new OS support

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: docs: update installation with new OS support
..


Patch Set 1:

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

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

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

[kudu-CR] docs: update installation with new OS support

[Adar Dembo (Code Review)]

Hello Todd Lipcon,

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

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

to review the following change.

Change subject: docs: update installation with new OS support
..

docs: update installation with new OS support

At the time of writing, a couple things are broken:
1. The SLES 12, Jessie, and Xenial Cloudera repo files contain unsubstituted
   template variables.
2. The SLES 12 Cloudera kudu package has a dependency (cyrus-sasl-lib) that
   does not exist on SLES 12.

I'm testing a patch to fix #2 and we're trying to fix #1 live. I did verify
that the SLES 12 packages work if installation is forced.

Change-Id: I2f51b55561f18fbe28d6bf4ed507dfba65947dc0
---
M docs/installation.adoc
1 file changed, 43 insertions(+), 11 deletions(-)


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

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

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

[Dinesh Bhat (Code Review)]

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

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

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

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

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

KUDU-1534 : Added software_version to ListMasters RPC

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

As a precaution, testing is performed to make sure downgrade/upgrade
are compatible with the message formats being consolidated. Testing
details are attached under JIRA.

Also added registration string to 'Masters' webui.
Sample output is at:
https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf

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


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

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

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

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 6:

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

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

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

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

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

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


Patch Set 4:

Addressed all the rev comments below, thanks Adar. Also updated commit message 
to reflect upgrade/downgrade testing.

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

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

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

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

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


Patch Set 22:

(17 comments)

Thank you for the review.

I'll try to take a fresh look in the context of simplifying the code.

Frankly, it would help a lot if it were clear from the very beginning that the 
KuduSession interface is not supposed to be thread-safe.  De-facto I started 
with the idea to have most of its methods thread-safe (as it was advertised by 
the comments in client.h).

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

PS22, Line 1037: current
> Nit: "the current"
Done


PS22, Line 1143: when
> Nit: "only when", right?
Done


PS22, Line 1180: called
> Nit: "called the"
Done


PS22, Line 1183: taken care
> Nit: "taken care of"
Done


PS22, Line 1184: Once flushing of the prior
   :   /// mutation buffer is initiated, the new one is created and 
set current
   :   /// to accommodate incoming write operations, limit 
permitting.
> Nit: "Upon flushing the current mutation buffer, a new buffer is created to
Done


Line 1188:   /// The default and the minimum setting for this parameter is 2 
(two).
> Hmm, technically couldn't the minimum be 1? If set to 1, an Apply() with an
Yes, technically the minimum could be 1.

Yes, the behavior of the KuduSession::Apply() you described is there, but it 
now comes into play only when the incoming operation triggers flush of the 
current buffer.

I put 2 to keep current semantics of always having the current batcher 
available for incoming operations and allocating it when starting flush of the 
prior one.

I can modify the code and make it 1.  That would involve checking for the 
current batcher availability at every Apply() call.  Right now it's guaranteed 
the current batcher is always at place.


PS22, Line 1191: the
> Nit: drop 'the'
Done


PS22, Line 1201: implicitly amended
   :   ///   as if it were 0.
> Nit: "implicitly set to 0".
Done


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

PS22, Line 80: the
> Nit: double the
Done


Line 130:   buffer_bytes_limit_ = size;
> Likewise, I don't see why this needs to be protected.
It seems that's a remnant from the older code which was written with 
multi-threading safety requirement in mind.

Thanks, will remove.


Line 145:   buffer_watermark_pct_ = watermark_pct;
> Why does this need to be protected? It's only ever accessed by the writer t
Right.  That's the same as for the KuduSession::SetBufferBytesLimit() -- 
remnant from the past.


PS22, Line 191:   Synchronizer s;
  :   KuduStatusMemberCallback ksmcb(, 
::StatusCB);
  :   NextBatcher(session, BATCHER_WATERMARK_NON_EMPTY, , 
BLOCK);
  :   RETURN_NOT_OK(s.Wait());
> Why do we need to use the synchronizer to wait on this batch if, below, we 
NextBatcher() not only flushes the current batcher, but it also allocates a new 
one for next incoming operations.

Probably, it's better to separate those functions, and then it will be possible 
to make 1 the minimum number for the batchers_num_limit_.  I'll do that.

Back to your question.  The NextBatcher() method has two modes of operation: 
blocking and non-blocking.  In non-blocking mode it reports an error if it's 
not possible to allocate a new batcher due to the batchers_num_limit_ setting; 
in blocking mode it blocks until it's possible to allocate a new one and do its 
job.

So, it's necessary to initiate flushing current batcher and then wait for 
total_bytes_used_ to become 0.

BTW, the error collector does not capture errors due to batchers_num_limit_ 
limitations -- those errors are reported  via the provided callback for the 
NextBatcher() (which usually comes from the FlushAsync()).


PS22, Line 222: relevent
> Nit: relevant
Done


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

PS22, Line 104:   // On successful return, the output flush_mode parameter
  :   // is set to the effective flush mode.
> I thought we no longer needed the output parameter for flush mode?
Right.

The reason of returning the effective flush_mode in output parameter is that I 
don't want to acquire the lock protecting flush_mode_ again just in the 
upper-level KuduSession::Apply() -- that needs to know the flush mode to decide 
whether it's necessary to do Flush() in case of AUTO_FLUSH_SYNC mode.  Strictly 
speaking, it's necessary either to access it under lock or make it atomic.

As for the task cancelation, I don't think it would be a better approach.


PS22, Line 142:   
> Nit: looks like there are two spaces here. Below too.
Done


PS22, Line 147: BATCHER_WATERMARK_NON_EMPTY
> Our convention for naming class constants like these "kCamelCaseFoo". We 

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

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

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


Patch Set 22:

(1 comment)

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

PS22, Line 191:   Synchronizer s;
  :   KuduStatusMemberCallback ksmcb(, 
::StatusCB);
  :   NextBatcher(session, BATCHER_WATERMARK_NON_EMPTY, , 
BLOCK);
  :   RETURN_NOT_OK(s.Wait());
> I wasn't asking about NextBatcher()'s wait; I was asking about s.Wait(). To
OK, I see the point now.

Right -- waiting on total_bytes_used_ should be more than enough given the fact 
the flush of the current batcher has been initiated.

I was concerned with collecting the exact error, if any, from the current 
batcher and missed the point that error_collector_ would account for that error 
as well.

And yes, if we separate two responsibilities of the NextBatch() method, then 
all that inconsistency will go away, so the next batcher would be allocated in 
Apply()/ApplyAsync() and FlushAsync() would not report on problems due to max 
number of batchers.


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

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

[kudu-CR] ksck: colorize and clean up output

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: ksck: colorize and clean up output
..

ksck: colorize and clean up output

Dan and I were looking at some ksck output earlier and found it somewhat
hard to read. This colorizes the output and also cleans up the format to
be slightly less messy. The output now goes to stdout instead of stderr
as well, so it's easier to pipe to a file or 'less', which is a common
use case.

Example output:

Tablet 0e4e79de2c2547c091779b13735d2b73 of table 'my_table' is 
under-replicated: 1 replica(s) not RUNNING
  63c4785c3b6f47f5a1c23ffbf6e52b71 (ts-023.foo.com:7050): RUNNING
  cd20c68cd23c4cf986eda0936a5691cc (ts-004.foo.com:7050): RUNNING [LEADER]
  5edf82f0516b4897b3a7991a7e67d71c (ts-028.foo.com:7050): bad state
State:   NOT_STARTED
Data state:  TABLET_DATA_COPYING
Last status: TabletCopy: Downloading block 4611685629730158289 (3491/9569)

Change-Id: Icc5196d63cbcbcbb2a9aba1ff17377b678c104bd
---
M src/kudu/tools/CMakeLists.txt
A src/kudu/tools/color.cc
A src/kudu/tools/color.h
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/tool_action_cluster.cc
7 files changed, 294 insertions(+), 142 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icc5196d63cbcbcbb2a9aba1ff17377b678c104bd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] ksck: colorize and clean up output

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: ksck: colorize and clean up output
..


Patch Set 2:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icc5196d63cbcbcbb2a9aba1ff17377b678c104bd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] tools: wrap descriptions

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: tools: wrap descriptions
..


Patch Set 2:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie63b72e1e1a3479819730c348f5a0f00a7164d02
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] java: inherit from ASF parent pom

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: java: inherit from ASF parent pom
..


Patch Set 2:

- updated to v18 of the parent pom.
- I believe I've updated Cloudera's internal mvn-deploy job so that committing 
this won't break anything, but I'll stand by and fix it if it breaks after this 
is committed :)

I'll also take on adding a public job which publishes snapshots to the Apache 
repo.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf4e46b9ba50c74fb7943aabd2065609e3c5e011
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No