[kudu-CR] mini cluster: Add scripts to build binaries for testing use
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11377 ) Change subject: mini cluster: Add scripts to build binaries for testing use .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11377/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11377/1//COMMIT_MSG@12 PS1, Line 12: preferably an old OS, such as RHEL 6 > Statically linking results in a larger artifact because we have to ship bot I tested building on EL6 and running on Ubuntu 16.04. -- To view, visit http://gerrit.cloudera.org:8080/11377 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 Gerrit-Change-Number: 11377 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 06 Sep 2018 04:34:08 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Hello Mike Percy, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11394 to review the following change. Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer To address KUDU-686, we're going to repurpose DMSIterator's PrepareBatch() machinery and associated in-memory state for use in the DeltaFileIterator. Doing so obviates the need for a "multi-pass" approach to ApplyUpdates() because DeltaFileIterator will no longer be performing any I/O or decoding there, having done all of it in PrepareBatch(). This patch lays the groundwork by refactoring the guts of DMSIterator into the new DeltaPreparer class. DMSIterator will continue to concern itself with CBTree iteration, but will delegate the delta preparation and service to DeltaPreparer. In performing this refactor, I tried to be as faithful as possible to the original code, even when I didn't really understand it (the prepared_idx_ and prepared_count_ state tracking gave me a lot of grief). No new tests; I figured there was enough test coverage of DMSIterator, and testing DeltaPreparer directly seemed like it'd be low bang for the buck. Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 --- M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.h M src/kudu/tablet/delta_store.cc 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 8 files changed, 397 insertions(+), 273 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/11394/1 -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] deltafile-test: DeltaFileIterator fuzz test
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11140 to look at the new patch set (#8). Change subject: deltafile-test: DeltaFileIterator fuzz test .. deltafile-test: DeltaFileIterator fuzz test Here's a new fuzz test for deltafile-test that generates random deltas and iterates at random snapshots over it, testing the various delta iterator functions. The test introduces MirroredDeltas, a utility that's useful for tracking deltas that are written into a delta file (or into a DMS, for that matter). I snuck in a change to map-util.h that I ultimately didn't use, but could be useful for others. Change-Id: I539ff0f2055cf398a42efb824238188230e25516 --- M src/kudu/gutil/map-util.h M src/kudu/tablet/CMakeLists.txt M src/kudu/tablet/delta_key.h M src/kudu/tablet/delta_store.h M src/kudu/tablet/deltafile-test.cc A src/kudu/tablet/tablet-test-util.cc M src/kudu/tablet/tablet-test-util.h 7 files changed, 481 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/11140/8 -- To view, visit http://gerrit.cloudera.org:8080/11140 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I539ff0f2055cf398a42efb824238188230e25516 Gerrit-Change-Number: 11140 Gerrit-PatchSet: 8 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-686 (part 2/2): use DeltaPreparer in DeltaFileIterator
Hello Mike Percy, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11395 to review the following change. Change subject: KUDU-686 (part 2/2): use DeltaPreparer in DeltaFileIterator .. KUDU-686 (part 2/2): use DeltaPreparer in DeltaFileIterator This patch rewrites much of DeltaFileIterator to leverage DeltaPreparer for delta preparation and service. Seeing as DeltaPreparer was originally built for DMSIterator, here are the various augmentations that were necessary to support DeltaFileIterator: - REINSERT support, which meant more complicated deletion state tracking. - FilterColumnIdsAndCollectDeltas support, cribbed from DeltaFileIterator. - A templatized traits system to control which features were enabled. This also meant templatizing both DeltaPreparer and DeltaFileIterator. - Early out from the "apply all deltas for a row" loop when the timestamp is no longer relevant. I modified DMSIterator to take advantage of this, which should result in a performance improvement. I tried to centralize as much state tracking in DeltaPreparer, though there were several aspects of this that were confusing (namely prepared_idx_, last_added_idx_, and prepared_count_). The patch's improvements should be most noticeable on wide schemas where the column-by-column ApplyUpdates() approach yielded a lot of unnecessary delta I/O and decoding. I don't have a suitable microbenchmark to prove this, but I did run diskrowset-test's TestDeltaApplicationPerformance under perf and the resulting flame graphs showed the bulk of the iteration time as having moved from ApplyUpdates() to PrepareBatch(). Change-Id: I87de52092262c4b42c1bd5107f9139edfc3888b5 --- M src/kudu/tablet/delta_key.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/delta_store.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/mvcc.cc M src/kudu/tablet/mvcc.h M src/kudu/tablet/tablet-test-util.h 11 files changed, 417 insertions(+), 393 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/11395/1 -- To view, visit http://gerrit.cloudera.org:8080/11395 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I87de52092262c4b42c1bd5107f9139edfc3888b5 Gerrit-Change-Number: 11395 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add delete external catalogs flag to table delete tool
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11197 ) Change subject: Add delete_external_catalogs flag to table delete tool .. Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/11197/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11197/1//COMMIT_MSG@9 PS1, Line 9: o recover from metadata : inconsistency between Kudu and the HMS, it is a good idea to introduce : a flag to control the tab > Not really understanding this: this commit introduces this flag, so how cou Sorry for the confusion. We have 'hms fix' tool which hopefully can cover all the cases when metadata between Kudu and the HMS become unsynchronized. But there might be some corner cases we do not foresee, that is why I introduce this commit. I will revise the commit message to better capture this. http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/client/client.h@539 PS1, Line 539: /// the necessary credentials to authenticate to the cluster, as well as to > Can you move this so it's next to DeleteTable? Done http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/client/client.h@547 PS1, Line 547: /// The resulting binary authentication credentials. > Could this just be called DeleteTable? 'WithOption' implies there would be Done http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/client/client.cc@400 PS1, Line 400: Status KuduClient::DeleteTable(const string& table_name) { > Could simplify this by forwarding to the new API. Done http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/master/master.proto@449 PS1, Line 449: optional bool modify_external_catalogs = 2 [default = true]; > What do you think about unifying the names of 'delete_external_catalogs' an Done http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/tools/tool_action_table.cc@39 PS1, Line 39: : DECLARE_string(tables); : DEFINE_bool(modify_external_catalogs, true, : "Whether to modify external catalogs, such as the Hive Metastore, " : "when renaming a table."); : DEFINE_bool(list_tablets, false, > These only exist to facilitate "upgrade" workflows that take you from no HM Not really, these can be used in normal workflows by admin to recover a cluster when somehow metadata in Kudu and the HMS go out of sync. http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/tools/tool_action_table.cc@129 PS1, Line 129: RETURN_NOT_OK(CreateKuduClient(context, )); > Add a CLI test. Done -- To view, visit http://gerrit.cloudera.org:8080/11197 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 Gerrit-Change-Number: 11197 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 06 Sep 2018 01:17:20 + Gerrit-HasComments: Yes
[kudu-CR] Add delete external catalogs flag to table delete tool
Hello Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11197 to look at the new patch set (#3). Change subject: Add delete_external_catalogs flag to table delete tool .. Add delete_external_catalogs flag to table delete tool Despite of 'hms fix' tool which intends to recover from metadata inconsistency between Kudu and the HMS, it is a good idea to introduce a flag to control the table deletion in the Kudu catalog independently of the HMS, to repair tables when HMS integration is enabled. Since there might be some edge cases we do not foresee and hence not covered in 'hms fix' tool. Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc M src/kudu/tools/tool_action_table.cc 14 files changed, 76 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/11197/3 -- To view, visit http://gerrit.cloudera.org:8080/11197 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 Gerrit-Change-Number: 11197 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add delete external catalogs flag to table delete tool
Hello Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11197 to look at the new patch set (#2). Change subject: Add delete_external_catalogs flag to table delete tool .. Add delete_external_catalogs flag to table delete tool Even with 'hms fix' tool which intendd to recover from metadata inconsistency between Kudu and the HMS, it is a good idea to introduce a flag to control the table deletion in the Kudu catalog independently of the HMS to repair tables when HMS integration is enabled. Since there might be some edge cases wo do not foresee and hence not covered in 'hms fix' tool Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc M src/kudu/tools/tool_action_table.cc 14 files changed, 76 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/11197/2 -- To view, visit http://gerrit.cloudera.org:8080/11197 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 Gerrit-Change-Number: 11197 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [location awareness] replica selection honors placement policy
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11207 ) Change subject: [location_awareness] replica selection honors placement policy .. Patch Set 7: (20 comments) http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG@16 PS7, Line 16: avaialable > available Done http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG@17 PS7, Line 17: load > replicas Done http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@878 PS7, Line 878: Select required number tablet servers to place replicas for the given newly : // created table > Howabout "Select the tablet servers where the given newly-created tablet's Woops, indeed that was a strange wording. It seems I wrote that while heaving that headache last Friday, sorry. Updated. http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@880 PS7, Line 880: is > are Done http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@882 PS7, Line 882: The > nit: Remove Done http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@4336 PS7, Line 4336: RETURN_NOT_OK(policy.PlaceTabletReplicas(nreplicas, )); > Can we add the tablet ID and table name to the status returned here? Done http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@4345 PS7, Line 4345: // TODO(unknown): this is temporary, we will use only UUIDs > nit: I don't think this is temporary anymore :). If there's no JIRA related Done http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h File src/kudu/master/placement_policy.h: http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h@41 PS7, Line 41: the > Remove Done http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h@73 PS7, Line 73: of > Remove Done http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc File src/kudu/master/placement_policy.cc: http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@55 PS7, Line 55: a > the Done http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@62 PS7, Line 62: deltas: information location of replicas that not yet placed > Stale? Done http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@63 PS7, Line 63: ltd > Out of order w.r.t the function parameters. Done http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@74 PS7, Line 74: auto num_live_replicas = accumulate( : ts_descriptors.begin(), ts_descriptors.end(), 0, : [](int val, const shared_ptr& desc) { : return val + desc->num_live_replicas(); : }); : const auto liit = locations_info.find(location); : if (liit != locations_info.end()) { : num_live_replicas += liit->second; : } > Is this computing R then adding R to it, so we return 2x the load? Nope, that's computing R for already existing replicas, and then adding number of replicas just slated for placement, but not instantiated yet. I'll add a comment to clarify on that. http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@107 PS7, Line 107: > > Should we (D?)CHECK that > doesn't happen? That's a good idea; added CHECK_LE(). http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@115 PS7, Line 115: for > Remove Done http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@115 PS7, Line 115: to > of Done. I really need to re-read my writings at least twice. http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@125 PS7, Line 125: if (location_per_load.empty()) { : // If placing an additional replica to any location will make it containing : // the majority of replilcas of the same tablet, place the replica into : // one of the less loaded locations (see below). : location_per_load.swap(location_per_load_majority); : } > If this happens and the RF is 2k, then each location must have k replicas, That's right, but there is a case when it's not enough tablet serves to place the requested number of replicas. The way how it's written is a convenient way to avoid placing a majority of replicas at the same location, even if the load-based criterion would favor that placement. It seems some comments were wrong/misleading. I
[kudu-CR] [location awareness] replica selection honors placement policy
Hello Will Berkeley, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11207 to look at the new patch set (#8). Change subject: [location_awareness] replica selection honors placement policy .. [location_awareness] replica selection honors placement policy This patch introduces placement policy into the catalog manager's replica selection process. The replica selection logic is factored out into the new PlacementPolicy class. In essence (for details see [1]), the placement policy is about: * in case of N locations, N > 2, not placing the majority of replicas in one location * spreading replicas evenly among available locations * within a location, spreading replicas evenly among tablet servers This patch also adds a few test scenarios for the new functionality. [1] https://s.apache.org/location-awareness-design Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd --- M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/placement_policy-test.cc A src/kudu/master/placement_policy.cc A src/kudu/master/placement_policy.h 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 10 files changed, 1,099 insertions(+), 194 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11207/8 -- To view, visit http://gerrit.cloudera.org:8080/11207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 8 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-1882 Configuration improvements for Flume Kudu Sink regexp operations producer.
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11391 to look at the new patch set (#3). Change subject: KUDU-1882 Configuration improvements for Flume Kudu Sink regexp operations producer. .. KUDU-1882 Configuration improvements for Flume Kudu Sink regexp operations producer. Adding new properties for the different parsing error policies. Deprecating the old ones. Default value constants are not removed as they are public variables on a public class and it would be an API change. Adding new test class to test the configuration and behaviour. Change-Id: I0b352b27583ae9eee6cdbe28a768362bea36e00f --- M java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java A java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerParseErrorTest.java 2 files changed, 380 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/11391/3 -- To view, visit http://gerrit.cloudera.org:8080/11391 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0b352b27583ae9eee6cdbe28a768362bea36e00f Gerrit-Change-Number: 11391 Gerrit-PatchSet: 3 Gerrit-Owner: Ferenc Szabo Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1882 Configuration improvements for Flume Kudu Sink regexp operations producer.
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11391 to look at the new patch set (#2). Change subject: KUDU-1882 Configuration improvements for Flume Kudu Sink regexp operations producer. .. KUDU-1882 Configuration improvements for Flume Kudu Sink regexp operations producer. Adding new properties for the different parsing error policies. Deprecating the old ones. Default value constants are not removed as they are public variables on a public class and it would be an API change. Adding new test class to test the configuration and behaviour. Change-Id: I0b352b27583ae9eee6cdbe28a768362bea36e00f --- M java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java A java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerParseErrorTest.java 2 files changed, 380 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/11391/2 -- To view, visit http://gerrit.cloudera.org:8080/11391 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0b352b27583ae9eee6cdbe28a768362bea36e00f Gerrit-Change-Number: 11391 Gerrit-PatchSet: 2 Gerrit-Owner: Ferenc Szabo Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1882 Configuration improvements for Flume Kudu Sink regexp operations producer.
Ferenc Szabo has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11391 Change subject: KUDU-1882 Configuration improvements for Flume Kudu Sink regexp operations producer. .. KUDU-1882 Configuration improvements for Flume Kudu Sink regexp operations producer. Adding new properties for the different parsing error policies. Deprecating the old ones. Default value constants are not removed as they are public variables on a public class and it would be an API change. Adding new test class to test the configuration and behaviour. Change-Id: I0b352b27583ae9eee6cdbe28a768362bea36e00f --- M java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java A java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerParseErrorTest.java 2 files changed, 380 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/11391/1 -- To view, visit http://gerrit.cloudera.org:8080/11391 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I0b352b27583ae9eee6cdbe28a768362bea36e00f Gerrit-Change-Number: 11391 Gerrit-PatchSet: 1 Gerrit-Owner: Ferenc Szabo
[kudu-CR] [location awareness] replica selection honors placement policy
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11207 ) Change subject: [location_awareness] replica selection honors placement policy .. Patch Set 7: (21 comments) Just small things and I think this is good to go. http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG@16 PS7, Line 16: avaialable available http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG@17 PS7, Line 17: load replicas http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@878 PS7, Line 878: Select required number tablet servers to place replicas for the given newly : // created table Howabout "Select the tablet servers where the given newly-created tablet's replica will be placed"? http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@880 PS7, Line 880: is are http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@882 PS7, Line 882: The nit: Remove http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@3391 PS7, Line 3391: for (const auto& ts_desc : ts_descs) { : if (IsRaftConfigMember(ts_desc->permanent_uuid(), cstate_.committed_config())) { : InsertOrDie(, ts_desc); : } : } It isn't very important because this code path isn't hot and the number of TS will be small, but wouldn't it be better to iterate over the UUIDs of the peers in the config to populate the set, rather than filtering all TS using the config? The difference is that 'ts_descs' excludes presumed dead TS, so if we changed how 'existing' is populated it might contain presumed dead TS. This should be harmless since we'd never propose to use a presumed dead TS since our options come from 'ts_descs'. http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@4336 PS7, Line 4336: RETURN_NOT_OK(policy.PlaceTabletReplicas(nreplicas, )); Can we add the tablet ID and table name to the status returned here? http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@4345 PS7, Line 4345: // TODO(unknown): this is temporary, we will use only UUIDs nit: I don't think this is temporary anymore :). If there's no JIRA related to this I feel like we can drop it. http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h File src/kudu/master/placement_policy.h: http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h@41 PS7, Line 41: the Remove http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h@73 PS7, Line 73: of Remove http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc File src/kudu/master/placement_policy.cc: http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@55 PS7, Line 55: a the http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@62 PS7, Line 62: deltas: information location of replicas that not yet placed Stale? http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@63 PS7, Line 63: ltd Out of order w.r.t the function parameters. http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@74 PS7, Line 74: auto num_live_replicas = accumulate( : ts_descriptors.begin(), ts_descriptors.end(), 0, : [](int val, const shared_ptr& desc) { : return val + desc->num_live_replicas(); : }); : const auto liit = locations_info.find(location); : if (liit != locations_info.end()) { : num_live_replicas += liit->second; : } Is this computing R then adding R to it, so we return 2x the load? http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@107 PS7, Line 107: > Should we (D?)CHECK that > doesn't happen? http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@115 PS7, Line 115: to of http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@115 PS7, Line 115: for Remove http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@125 PS7, Line 125: if (location_per_load.empty()) { : // If placing an additional replica to any location will make it containing : // the majority of replilcas of the same tablet, place the replica into : // one of the less loaded locations (see below). :
[kudu-CR] KUDU-2529: Add a "-tables=" flag to the "kudu table list".
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11360 ) Change subject: KUDU-2529: Add a "-tables=" flag to the "kudu table list". .. KUDU-2529: Add a "-tables=" flag to the "kudu table list". "-tables=" flag can help to filter the tables that you want while running "kudu table list -list_tables". Change-Id: I23006a3f3e4f157e6c6c10968b8692e89892d1ce Reviewed-on: http://gerrit.cloudera.org:8080/11360 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/tools/ksck.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_table.cc 6 files changed, 152 insertions(+), 19 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/11360 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I23006a3f3e4f157e6c6c10968b8692e89892d1ce Gerrit-Change-Number: 11360 Gerrit-PatchSet: 9 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] [location awareness] Add 'location' column in tserver list
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11313 ) Change subject: [location_awareness] Add 'location' column in tserver list .. [location_awareness] Add 'location' column in tserver list Command: kudu tserver list -columns=uuid,location Example result: uuid | location --+-- 1259764cdc5f489984900d49b545802f | loc0 14446895a8bf47cd92e73836de623ffb | 9d7a11e19b324f62b2e6d074f6003ca4 | loc1 This command will list the location of each tserver. If the location of the tserver has not been set, '' will be displayed. Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Reviewed-on: http://gerrit.cloudera.org:8080/11313 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin Reviewed-by: Will Berkeley --- M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/first_argument.sh M src/kudu/tools/tool_action_tserver.cc 6 files changed, 61 insertions(+), 1 deletion(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, but someone else must approve Will Berkeley: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 7 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2529: Add a "-tables=" flag to the "kudu table list".
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11360 ) Change subject: KUDU-2529: Add a "-tables=" flag to the "kudu table list". .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11360 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I23006a3f3e4f157e6c6c10968b8692e89892d1ce Gerrit-Change-Number: 11360 Gerrit-PatchSet: 8 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 05 Sep 2018 07:33:54 + Gerrit-HasComments: No
[kudu-CR] KUDU-2529: Add a "-tables=" flag to the "kudu table list".
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11360 to look at the new patch set (#8). Change subject: KUDU-2529: Add a "-tables=" flag to the "kudu table list". .. KUDU-2529: Add a "-tables=" flag to the "kudu table list". "-tables=" flag can help to filter the tables that you want while running "kudu table list -list_tables". Change-Id: I23006a3f3e4f157e6c6c10968b8692e89892d1ce --- M src/kudu/tools/ksck.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_table.cc 6 files changed, 152 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/11360/8 -- To view, visit http://gerrit.cloudera.org:8080/11360 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I23006a3f3e4f157e6c6c10968b8692e89892d1ce Gerrit-Change-Number: 11360 Gerrit-PatchSet: 8 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] cfile-test: pass in IOContext on Open()
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11389 ) Change subject: cfile-test: pass in IOContext on Open() .. Patch Set 2: > > Patch Set 1: Code-Review+2 > > > > I'm curious why ASAN pre-commit build didn't catch that? > > Yeah, it's surprising that it passed entirely on CentOs 6.6. That > said, ASAN will cover: > - Use after free (dangling pointer dereference) > - Heap buffer overflow > - Stack buffer overflow > - Global buffer overflow > - Use after return > - Use after scope > - Initialization order bugs > - Memory leaks > > Of which this sort of error doesn't fall into any. It's certainly > similar, but not quite any of them. UBSAN would have probably > caught this though. Well, I meant ASAN build, but our ASAN build has both address and undefined sanitizers included. BTW, I'm not sure our ASAN configuration enables use-after-return and use-after-free traps. -- To view, visit http://gerrit.cloudera.org:8080/11389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaed883d26075ed1823882a7f7b18834f39b0732d Gerrit-Change-Number: 11389 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 05 Sep 2018 06:19:30 + Gerrit-HasComments: No
[kudu-CR] cfile-test: pass in IOContext on Open()
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11389 ) Change subject: cfile-test: pass in IOContext on Open() .. cfile-test: pass in IOContext on Open() Previously, on certain machines, cfile-test's TestDataCorruption would fail 100% of the time when attempting to read from a null IOContext. The passed-in ReaderOptions struct was constructed with the default constructor, and because no default was set for its io_context member, this resulted in undefined behavior, which explains why this wasn't caught in pre-commit. This patch updates cfile-test to correctly pass in an IOContext, and assigns a default for ReaderOption's io_context. Change-Id: Iaed883d26075ed1823882a7f7b18834f39b0732d Reviewed-on: http://gerrit.cloudera.org:8080/11389 Reviewed-by: Adar Dembo Reviewed-by: Alexey Serbin Tested-by: Kudu Jenkins --- M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_util.h 2 files changed, 5 insertions(+), 3 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Alexey Serbin: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/11389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Iaed883d26075ed1823882a7f7b18834f39b0732d Gerrit-Change-Number: 11389 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] cfile-test: pass in IOContext on Open()
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11389 ) Change subject: cfile-test: pass in IOContext on Open() .. Patch Set 1: > Patch Set 1: Code-Review+2 > > I'm curious why ASAN pre-commit build didn't catch that? Yeah, it's surprising that it passed entirely on CentOs 6.6. That said, ASAN will cover: - Use after free (dangling pointer dereference) - Heap buffer overflow - Stack buffer overflow - Global buffer overflow - Use after return - Use after scope - Initialization order bugs - Memory leaks Of which this sort of error doesn't fall into any. It's certainly similar, but not quite any of them. UBSAN would have probably caught this though. -- To view, visit http://gerrit.cloudera.org:8080/11389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaed883d26075ed1823882a7f7b18834f39b0732d Gerrit-Change-Number: 11389 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 05 Sep 2018 06:11:12 + Gerrit-HasComments: No