[kudu-CR] tool: basic integration test
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
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] Add AvroKuduEventProducer to Kudu-Flume integration
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] KUDU-687: add replication factor to KuduTable
Adar Dembo has uploaded a new change for review. http://gerrit.cloudera.org:8080/4123 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 --- 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(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/4123/1 -- To view, visit http://gerrit.cloudera.org:8080/4123 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If76ec6c3001e78a31517991d7432f79d4645fccc Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo
[kudu-CR] tool: add ksck
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 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] Remove pessimizing std::move() in return statement
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 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 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 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
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
Hello Mike Percy, Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4099 to look at the new patch set (#4). Change subject: KUDU-1534 : Added software_version to ListMasters RPC .. KUDU-1534 : Added software_version to ListMasters RPC This change also consolidates TSRegistrationPB and ServerRegistrationPB and merges them into latter as they seem to bear the same signature. Sample output is at: https://github.com/dineshabbi/scripts/blob/master/MasterRegWebUI.pdf Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c --- M src/kudu/common/wire_protocol.proto M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/registration-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master-path-handlers.h M src/kudu/master/master-test.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h M src/kudu/tserver/heartbeater.cc 17 files changed, 65 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/4099/4 -- To view, visit http://gerrit.cloudera.org:8080/4099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I750eeb78c989daaa6fd5d9793b1218ae5993653c Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
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
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
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
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] Rolling-upgrades compat for changing TSRegistrationPB
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-1534 : Added software version to ListMasters RPC
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] KUDU-456 Implement AUTO FLUSH BACKGROUND flush mode
Alexey Serbin has posted comments on this change. Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode .. Patch Set 15: (8 comments) Thank you for your feedback. I'm posting a new version in a minute. http://gerrit.cloudera.org:8080/#/c/3952/15/src/kudu/client/client.h File src/kudu/client/client.h: Line 1146: Status SetMutationBufferFlushWatermark(double watermark_pct) > I don't think it's a big deal either way. Sessions are cheap (especially no OK, that seems to be good reasoning to me. No getters then. http://gerrit.cloudera.org:8080/#/c/3952/15/src/kudu/client/session-internal.cc File src/kudu/client/session-internal.cc: PS15, Line 64: -1, > I don't think the existing behavior is necessarily worth preserving; an emp All right, I removed the semantics of flushing an empty batcher. That will make a difference when there is a tight limit on maximum number of batchers allowed to exist simultaneously. Line 66: PeriodicFlushTask(Status::OK(), messenger_, session); > Sorry, I don't understand what you're suggesting. Can you explain it in mor I'm sorry for that poorly articulated and poorly written piece. To clarify on that thought, I'm copy-pasting a comment from the code which implements that idea, please see below. Let's measure age of a batcher as the time elapsed from the moment of adding the very first operation into the batcher. The idea is to flush a batcher when its age is very close to the flush_interval_: let's call it 'batcher flush age'. If current batcher hasn't reached its flush age yet, just re-schedule the task to re-evaluate the age of then-will-be-current batcher, so if the current batcher is still current at that time, it will be exactly of its flush age. Doing so allows to address the issue you pointed at. Line 109: flow_control_cond_.Broadcast(); > Nope, simplifying the implementation is the underlying motivation. But I th OK, I see. It seems that after recent discussions an additional reason appeared: we want to have some sort of parity with the Java client. Line 135: CHECK_GE(timeout_ms, 0); > I think that's reasonable provided it's documented. Done PS15, Line 230: RETURN_NOT_OK(TryApplyWriteOp(write_op, &required_size, : ¤t_flush_mode, ¤t_flush_watermark)); > I don't understand why friendship factors into this: both ApplyWriteOp and I thought you were talking about calling the batcher directly. Yes, I addressed the required_size so it's no longer an output parameter of the TryApplyWriteOp() method. Line 398: data->NextBatcher(session, 0, nullptr); > I find the semantics of -1, 0, and >0 to be non-obvious. Perhaps we can con Done http://gerrit.cloudera.org:8080/#/c/3952/15/src/kudu/client/session-internal.h File src/kudu/client/session-internal.h: PS15, Line 87: On successful return, the output flush_mode parameter is set : // to the effective flush mode. > I don't understand how flush_mode_ can change in the middle of ApplyWriteOp Yes, this is a holdover. The comment was written when I was under impression we allow to have multiple threads working with a single KuduSession object. Also, calls of Apply() from different threads are not relevant here -- actually, the reason was that other thread might call KuduSession::SetFlushMode() while waiting on the condition variable. But now all those concerns are gone. We don't allow to change the mode while operations are pending and we don't allow making calls to a single KuduSession object from multiple threads. -- 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: 15 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
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-456 Implement AUTO FLUSH BACKGROUND flush mode
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
Kudu Jenkins has posted comments on this change. Change subject: Add AvroKuduEventProducer to Kudu-Flume integration .. Patch Set 3: Build Started http://104.196.14.100/job/kudu-gerrit/3073/ -- 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: 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 Gerrit-HasComments: No
[kudu-CR] Add AvroKuduEventProducer to Kudu-Flume integration
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] Predicate evaluation pushdown
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 (#5). 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, 927 insertions(+), 124 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/3990/5 -- 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: 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
[kudu-CR] Predicate evaluation pushdown
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 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(&s, &Synchronizer::StatusCB); : NextBatcher(session, BATCHER_WATERMARK_NON_EMPTY, &ksmcb, 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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComment
[kudu-CR] WIP: ts-cli: add commands to trigger step-down and copying tablets
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] WIP: ts-cli: add commands to trigger step-down and copying tablets
Kudu Jenkins has posted comments on this change. Change subject: WIP: ts-cli: add commands to trigger step-down and copying tablets .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/3075/ -- To view, visit http://gerrit.cloudera.org:8080/3582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment 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 Gerrit-HasComments: No
[kudu-CR] tool: basic integration test
Todd Lipcon has posted comments on this change. Change subject: tool: basic integration test .. Patch Set 5: Code-Review+2 -- 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: add ksck
Todd Lipcon has submitted this change and it was merged. Change subject: tool: add ksck .. tool: add ksck This patch adds ksck functionality to the new CLI tool. ksck is still available as a library used by several integration tests. Change-Id: Iad9aaffa5c0c77080dcaeb2cdfa572dcbeeb1407 Reviewed-on: http://gerrit.cloudera.org:8080/4121 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon --- M docs/release_notes.adoc M src/kudu/tools/CMakeLists.txt D src/kudu/tools/kudu-ksck.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h A src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_main.cc 7 files changed, 162 insertions(+), 156 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Iad9aaffa5c0c77080dcaeb2cdfa572dcbeeb1407 Gerrit-PatchSet: 4 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
[kudu-CR] tool: basic integration test
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] tool: add ksck
Todd Lipcon has posted comments on this change. Change subject: tool: add ksck .. Patch Set 3: Code-Review+2 -- 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
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] KUDU-687: add replication factor to KuduTable
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
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] KUDU-687: add replication factor to KuduTable
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] Rolling-upgrades compat for changing TSRegistrationPB
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
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] Predicate evaluation pushdown
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] KUDU-456 Implement AUTO FLUSH BACKGROUND flush mode
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(&s, &Synchronizer::StatusCB); : NextBatcher(session, BATCHER_WATERMARK_NON_EMPTY, &ksmcb, 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
[kudu-CR] KUDU-456 Implement AUTO FLUSH BACKGROUND flush mode
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(&s, &Synchronizer::StatusCB); : NextBatcher(session, BATCHER_WATERMARK_NON_EMPTY, &ksmcb, 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
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] docs: update installation with new OS support
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] KUDU-1534 : Added software version to ListMasters RPC
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 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 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 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(&s, &Synchronizer::StatusCB); : NextBatcher(session, BATCHER_WATERMARK_NON_EMPTY, &ksmcb, 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] docs: update installation with new OS support
Adar Dembo has posted comments on this change. Change subject: docs: update installation with new OS support .. Patch Set 1: Verified+1 Unrelated test flake. -- 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: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-456 Implement AUTO FLUSH BACKGROUND flush mode
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(&s, &Synchronizer::StatusCB); : NextBatcher(session, BATCHER_WATERMARK_NON_EMPTY, &ksmcb, 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 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 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(&s, &Synchronizer::StatusCB); : NextBatcher(session, BATCHER_WATERMARK_NON_EMPTY, &ksmcb, 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] docs: update installation with new OS support
Alexey Serbin has posted comments on this change. Change subject: docs: update installation with new OS support .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/4128/1//COMMIT_MSG Commit Message: Line 10: 1. The SLES 12, Jessie, and Xenial Cloudera repo files contain unsubstituted nit: could you make the lines in the detailed description to be less than 72 symbols long? http://gerrit.cloudera.org:8080/#/c/4128/1/docs/installation.adoc File docs/installation.adoc: Line 113: provide the operating system commands to start and stop Kudu. Nit: consider 'These packages provide configuration files along with init.d service scripts to manage Kudu processes. Once these packages are installed, the corresponding Kudu processes are started/stopped automatically on OS start-up/shutdown.'. Also, is it worth mentioning that these packages should not be installed if running the CM (ClouderaManager)? PS1, Line 115: O O --> o ? I.e., why is the capital 'o' letter? And in other places with the install section. -- 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: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1534 : Added software version to ListMasters RPC
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] KUDU-1534 : Added software version to ListMasters RPC
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 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] tools: wrap descriptions
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
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] ksck: colorize and clean up output
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] java: fix leak of TabletClient objects in client2tablets map
Kudu Jenkins has posted comments on this change. Change subject: java: fix leak of TabletClient objects in client2tablets map .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/3080/ -- 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] tools: wrap descriptions
Hello Dan Burkert, Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/4130 to review the following change. Change subject: tools: wrap descriptions .. tools: wrap descriptions Wraps the help and usage output on a 78-column width so that it's much more readable. New output: todd@todd-ThinkPad-T540p:~/git/kudu$ ./build/latest/bin/kudu cluster Usage: ./build/latest/bin/kudu cluster [] can be one of the following: ksck Check the health of a Kudu cluster. By default, ksck checks that master and tablet server processes are running, and that table metadata is consistent. Use the 'checksum' flag to check that tablet data is consistent (also see the 'tables' and 'tablets' flags). Use the 'checksum_snapshot' along with 'checksum' if the table or tablets are actively receiving inserts or updates. todd@todd-ThinkPad-T540p:~/git/kudu$ ./build/latest/bin/kudu cluster ksck --help Usage: ./build/latest/bin/kudu cluster ksck [-checksum_scan] [-nochecksum_snapshot] [-tables=] [-tablets=] [-color=] Check the health of a Kudu cluster. By default, ksck checks that master and tablet server processes are running, and that table metadata is consistent. Use the 'checksum' flag to check that tablet data is consistent (also see the 'tables' and 'tablets' flags). Use the 'checksum_snapshot' along with 'checksum' if the table or tablets are actively receiving inserts or updates. master_address (Kudu Master RPC address of form hostname:port) type: string default: "" -checksum_scan (Perform a checksum scan on data in the cluster.) type: bool default: false -checksum_snapshot (Should the checksum scanner use a snapshot scan) type: bool default: true -tables (Tables to check (comma-separated list of names). If not specified, checks all tables.) type: string default: "" -tablets (Tablets to check (comma-separated list of IDs) If not specified, checks all tablets.) type: string default: "" -color (Specifies whether ksck output is colorized. The default value 'auto' colorizes output if the output is a terminal. The other valid values are 'always' or 'never'.) type: string default: "auto" Change-Id: Ie63b72e1e1a3479819730c348f5a0f00a7164d02 --- M src/kudu/tools/tool_action.cc 1 file changed, 58 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/4130/1 -- To view, visit http://gerrit.cloudera.org:8080/4130 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange 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
[kudu-CR] java: fix leak of TabletClient objects in client2tablets map
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] tools: wrap descriptions
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] ksck: colorize and clean up output
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 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] docs: update installation with new OS support
Todd Lipcon has posted comments on this change. Change subject: docs: update installation with new OS support .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/4128/1/docs/installation.adoc File docs/installation.adoc: PS1, Line 43: When using replication, a minimum of : three masters is necessary. I think we should explicitly recommend three or five. People might think it's a good idea to run masters on every node in their cluster, which, while technically possible, isn't within the realm that we really test. -- 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: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] java: inherit from ASF parent pom
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4118 to look at the new patch set (#2). Change subject: java: inherit from ASF parent pom .. java: inherit from ASF parent pom This removes the Cloudera distribution management section and adds the ASF parent pom to pick up the ASF nexus for distribution management. Change-Id: Icf4e46b9ba50c74fb7943aabd2065609e3c5e011 --- M java/pom.xml 1 file changed, 8 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/4118/2 -- To view, visit http://gerrit.cloudera.org:8080/4118 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset 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
[kudu-CR] java: inherit from ASF parent pom
Kudu Jenkins has posted comments on this change. Change subject: java: inherit from ASF parent pom .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/3085/ -- 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-HasComments: No
[kudu-CR] java: inherit from ASF parent pom
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
[kudu-CR] master: include TS address in log messages
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] master: include TS address in log messages
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
Dinesh Bhat has posted comments on this change. Change subject: master: include TS address in log messages .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/4131/1//COMMIT_MSG Commit Message: Line 15: server. Thanks, yeah this is super useful while browsing logs with sclable nodes/tablets. Could you also see if there are places like MasterServiceImpl::TSHeartbeat() where it makes sense to add this ToString() from error paths. -- 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: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes