[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
