[kudu-CR] schema: add is deleted virtual column
Hello Tidy Bot, Mike Percy, Dan Burkert, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10968 to look at the new patch set (#11). Change subject: schema: add is_deleted virtual column .. schema: add is_deleted virtual column This patch introduces a very basic concept of a "virtual column". Virtual columns borrow from other databases in that they are columns that, rather than being backed by physical data, are instead backed by Kudu itself. They may not be part of a schema during table creation/alteration, but may be added to projections during a scan. Kudu's virtual columns are defined as logical data types. As data types are not user-defined, there's no danger of a "collision" between a virtual column and a physical column as there would be if a virtual column occupied a well-defined name. A Kudu subsystem on the scan path that wishes to interact with a virtual column needs to first figure out if the projection includes it. When projected, the virtual column's data will be either some default or null (depending on exactly how it was defined in the projection); it's the subsystem's responsibility to fill in something meaningful afterwards. Beyond the basic definition, this patch introduces an IS_DELETED virtual column derived from BOOL. IS_DELETED will be used to support incremental backups by describing whether a row was deleted between two timestamps. Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e --- M src/kudu/common/common.proto M src/kudu/common/schema.cc M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/master/catalog_manager.cc M src/kudu/master/master-test.cc 6 files changed, 119 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/11 -- To view, visit http://gerrit.cloudera.org:8080/10968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e Gerrit-Change-Number: 10968 Gerrit-PatchSet: 11 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] schema: add is deleted virtual column
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 ) Change subject: schema: add is_deleted virtual column .. Patch Set 10: (2 comments) > Nice, looks like not too much boilerplate! Check out the MRS patch that follows; there's a bit of a gotcha in that there's no mandated nullability or read-default. http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/schema.h@881 PS10, Line 881: static const char* kReservedColNamePrefix; > No longer needed? Done http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h@625 PS10, Line 625: return DataTypeTraits::name(); > Should this be "is_deleted"? The derived types above don't delegate to the It was at first, but that led to a row dump that looked like this: (string key="row", uint32 val=5, is_deleted is_deleted=true) I thought that was confusing and ugly (granted in part because I named the column 'is_deleted'), so I added this bit of delegation. -- To view, visit http://gerrit.cloudera.org:8080/10968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e Gerrit-Change-Number: 10968 Gerrit-PatchSet: 10 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 02 Aug 2018 04:02:09 + Gerrit-HasComments: Yes
[kudu-CR] Add error handling to Backup and Restore
Tony Foerster has abandoned this change. ( http://gerrit.cloudera.org:8080/10941 ) Change subject: Add error handling to Backup and Restore .. Abandoned I was considering this a sort of incremental change, but looking at it I think it needs more work. I'll open a new request after reworking. -- To view, visit http://gerrit.cloudera.org:8080/10941 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I104fd604e12b70fcff9beace71ed4ef96b54d691 Gerrit-Change-Number: 10941 Gerrit-PatchSet: 9 Gerrit-Owner: Tony Foerster Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tony Foerster
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Tony Foerster has abandoned this change. ( http://gerrit.cloudera.org:8080/7749 ) Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API .. Abandoned Keepalive shouldn't rely on getting the same replica from the cached RemoteTablet. -- To view, visit http://gerrit.cloudera.org:8080/7749 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713 Gerrit-Change-Number: 7749 Gerrit-PatchSet: 9 Gerrit-Owner: Tony Foerster Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Tony Foerster
[kudu-CR] schema: add is deleted virtual column
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 ) Change subject: schema: add is_deleted virtual column .. Patch Set 10: (2 comments) Nice, looks like not too much boilerplate! http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/schema.h@881 PS10, Line 881: static const char* kReservedColNamePrefix; No longer needed? http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h@625 PS10, Line 625: return DataTypeTraits::name(); Should this be "is_deleted"? The derived types above don't delegate to their physical type. -- To view, visit http://gerrit.cloudera.org:8080/10968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e Gerrit-Change-Number: 10968 Gerrit-PatchSet: 10 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 02 Aug 2018 03:14:39 + Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Hello Tidy Bot, Mike Percy, Dan Burkert, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10968 to look at the new patch set (#10). Change subject: schema: add is_deleted virtual column .. schema: add is_deleted virtual column This patch introduces a very basic concept of a "virtual column". Virtual columns borrow from other databases in that they are columns that, rather than being backed by physical data, are instead backed by Kudu itself. They may not be part of a schema during table creation/alteration, but may be added to projections during a scan. Kudu's virtual columns are defined as logical data types. As data types are not user-defined, there's no danger of a "collision" between a virtual column and a physical column as there would be if a virtual column occupied a well-defined name. A Kudu subsystem on the scan path that wishes to interact with a virtual column needs to first figure out if the projection includes it. When projected, the virtual column's data will be either some default or null (depending on exactly how it was defined in the projection); it's the subsystem's responsibility to fill in something meaningful afterwards. Beyond the basic definition, this patch introduces an IS_DELETED virtual column derived from BOOL. IS_DELETED will be used to support incremental backups by describing whether a row was deleted between two timestamps. Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e --- M src/kudu/common/common.proto M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/master/catalog_manager.cc M src/kudu/master/master-test.cc 7 files changed, 122 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/10 -- To view, visit http://gerrit.cloudera.org:8080/10968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e Gerrit-Change-Number: 10968 Gerrit-PatchSet: 10 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] memrowset: support iteration with is deleted virtual column
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10990 to look at the new patch set (#8). Change subject: memrowset: support iteration with is_deleted virtual column .. memrowset: support iteration with is_deleted virtual column This patch rounds out the MemRowSet changes for incremental backups. Taken together, it is now possible to iterate on a specific time range and to learn which rows were deleted during that time range. Data type based virtual columns show a wart here: the iterator doesn't know whether IS_DELETED was defined as nullable or with a read default value, and it must be prepared for either. We could require one or the other, but that pokes a hole in the abstraction of a virtual column as just a data type, which seems undesirable. Change-Id: Ic6b053f5a3696eb9d7c26b8e3d96752f4f87bcd8 --- M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h 3 files changed, 135 insertions(+), 49 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/10990/8 -- To view, visit http://gerrit.cloudera.org:8080/10990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic6b053f5a3696eb9d7c26b8e3d96752f4f87bcd8 Gerrit-Change-Number: 10990 Gerrit-PatchSet: 8 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Kudu-1291 Efficiently support predicates on non-prefix key components
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10983 ) Change subject: Kudu-1291 Efficiently support predicates on non-prefix key components .. Patch Set 5: > > I took a quick look: my comments are nits regarding the style and > other minor stuff; I didn't look the semantics or the patch yet. > > The patch is still at a point that I think we should mostly focus > on substance rather than nits, although it will need a detailed nit > pass later. Ah, I see -- thank you for letting me know. I thought since the WIP tag was removed it's more or less ready. -- To view, visit http://gerrit.cloudera.org:8080/10983 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I230cd5a288e28ace796b352a603e0d1bcc1e4e0f Gerrit-Change-Number: 10983 Gerrit-PatchSet: 5 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Anupama Gupta Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 02 Aug 2018 00:46:06 + Gerrit-HasComments: No
[kudu-CR] Kudu-1291 Efficiently support predicates on non-prefix key components
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10983 ) Change subject: Kudu-1291 Efficiently support predicates on non-prefix key components .. Patch Set 5: > I took a quick look: my comments are nits regarding the style and other minor > stuff; I didn't look the semantics or the patch yet. The patch is still at a point that I think we should mostly focus on substance rather than nits, although it will need a detailed nit pass later. -- To view, visit http://gerrit.cloudera.org:8080/10983 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I230cd5a288e28ace796b352a603e0d1bcc1e4e0f Gerrit-Change-Number: 10983 Gerrit-PatchSet: 5 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Anupama Gupta Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 02 Aug 2018 00:42:45 + Gerrit-HasComments: No
[kudu-CR] Kudu-1291 Efficiently support predicates on non-prefix key components
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10983 ) Change subject: Kudu-1291 Efficiently support predicates on non-prefix key components .. Patch Set 5: (5 comments) still reviewing, here are some initial comments http://gerrit.cloudera.org:8080/#/c/10983/5/src/kudu/cfile/cfile_reader.h File src/kudu/cfile/cfile_reader.h: http://gerrit.cloudera.org:8080/#/c/10983/5/src/kudu/cfile/cfile_reader.h@340 PS5, Line 340: set_current_value document this parameter in the header file comment http://gerrit.cloudera.org:8080/#/c/10983/5/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/10983/5/src/kudu/tablet/cfile_set.cc@413 PS5, Line 413: // Initialize predicate column id to an upperbound value nit: add punctuation to all your comments (add a period at the end of the sentence) http://gerrit.cloudera.org:8080/#/c/10983/5/src/kudu/tablet/cfile_set.cc@511 PS5, Line 511: // Increment "num_cols" no of columns this kind of api doc should go into the header file, not the implementation http://gerrit.cloudera.org:8080/#/c/10983/5/src/kudu/tablet/cfile_set.cc@527 PS5, Line 527: _match since we are not using this, can we just make this /* exact_match= */ nullptr ? http://gerrit.cloudera.org:8080/#/c/10983/5/src/kudu/tablet/index_skipscan-test.cc File src/kudu/tablet/index_skipscan-test.cc: http://gerrit.cloudera.org:8080/#/c/10983/5/src/kudu/tablet/index_skipscan-test.cc@400 PS5, Line 400: int predicate_col_id = 2; please add a random test case that does not put the predicate on the last column -- To view, visit http://gerrit.cloudera.org:8080/10983 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I230cd5a288e28ace796b352a603e0d1bcc1e4e0f Gerrit-Change-Number: 10983 Gerrit-PatchSet: 5 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Anupama Gupta Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 02 Aug 2018 00:41:02 + Gerrit-HasComments: Yes
[kudu-CR] Kudu-1291 Efficiently support predicates on non-prefix key components
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10983 ) Change subject: Kudu-1291 Efficiently support predicates on non-prefix key components .. Patch Set 4: (7 comments) I took a quick look: my comments are nits regarding the style and other minor stuff; I didn't look the semantics or the patch yet. http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/cfile/cfile_reader.h File src/kudu/cfile/cfile_reader.h: http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/cfile/cfile_reader.h@343 PS4, Line 343: std::string GetCurrentValue(); nit: add const specifier for the method. Also, consider returning const reference from this method, if possible. http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/cfile/cfile_reader.cc@788 PS4, Line 788: prepared_blocks_ what if prepared_blocks_ is empty? If that the thing-that-cannot-be, add DCHECK() for the non-emptiness condition. http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h File src/kudu/tablet/cfile_set.h: http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@248 PS4, Line 248: ScanSpec *spec style nit: here and elsewhere -- we tend to stick the asterisk and ampersand to the type, not to the variable/parameter name. http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@399 PS4, Line 399: ScanSpec If 'spec' instance is not modified here, why not to pass it as a const reference? http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@414 PS4, Line 414: std::unordered_map predicates Is copying necessary here? Why not to use const reference? http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@418 PS4, Line 418: (it->second) nit: I don't think the parentheses are necessary here http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@500 PS4, Line 500: skip_scan_enabled_ = CanUseSkipScan(spec); Why not to assign the 'skip_scan_enabled_' field in the CanUseSkipScan() method itself since other aux members (like 'suffix_key_column_id_') are assigned in that method already? And rename CanUseSkipScan() into TryEnableSkipScan() or alike? -- To view, visit http://gerrit.cloudera.org:8080/10983 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I230cd5a288e28ace796b352a603e0d1bcc1e4e0f Gerrit-Change-Number: 10983 Gerrit-PatchSet: 4 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Anupama Gupta Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 02 Aug 2018 00:33:01 + Gerrit-HasComments: Yes
[kudu-CR] Kudu-1291 Efficiently support predicates on non-prefix key components
Anupama Gupta has posted comments on this change. (
http://gerrit.cloudera.org:8080/10983 )
Change subject: Kudu-1291 Efficiently support predicates on non-prefix key
components
..
Patch Set 4:
(22 comments)
Please review the changes.
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/cfile/cfile_reader.cc@785
PS4, Line 785: Slice ret;
> add: DCHECK_EQ(nullptr, validx_iter_);
Added a check for this. Actually no, I think there is no guarantee that value
index will always be string. I have added a flag to invoke this function
(set_current_value) now . I wonder if there is an alternative to deal with this.
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h
File src/kudu/tablet/cfile_set.h:
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@204
PS4, Line 204: // This function is used to place the validx_iter_ at the next
greater "prefix_key".
> nit: you should try to maintain the same order in the header file as the .c
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@283
PS4, Line 283: skip_scan_enabled_
> rename to use_skip_scan_ because this is confusingly similar to the gflag n
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@286
PS4, Line 286: suffix_key_pred_value_
> rename to skip_scan_predicate_value_
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@289
PS4, Line 289: suffix_key_column_id_
> rename to skip_scan_predicate_column_id_
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@291
PS4, Line 291: // Row id of the last entry of "suffix_key" predicate value
wrt to the
: // "prefix_key" currently pointed to by validx_iter_
> use something like this as the description:
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@293
PS4, Line 293: int
> make this an int64_t
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@538
PS4, Line 538: if (skip_scan_enabled_ && (static_cast(cur_idx_) >
suffix_key_upper_bound_idx_)) {
> Can you break this out into its own function?
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@540
PS4, Line 540: bool suffix_key_match, continue_seeking_next_prefix = false;
> nit: declare these variables separately
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@542
PS4, Line 542: Status next_entry_match, suffix_key_upper_bound_match;
> Can we avoid maintaining so much state outside the loop?
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@602
PS4, Line 602: continue_seeking_next_prefix = true;
> not indented
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc
File src/kudu/tablet/index_skipscan-test.cc:
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@56
PS4, Line 56: kRandom
> the random tests here are not very random. Can you add an actually-random t
Thanks for this suggestion. I have added the random tests now.
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@145
PS4, Line 145: int32_t kNumDistinctP1 = 2;
: int16_t kNumDistinctP2 = 10;
: int kNumDistinctP3 = 5;
: int8_t kNumDistinctP4 = 1;
: int8_t kNumDistinctP5 = 20;
> declare all of these variables const
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@163
PS4, Line 163: int32_t
> int16_t for p2 here and in the below loops
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@226
PS4, Line 226: //Only 1 row inserted
> nit: formatting, should be:
Done.
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@253
PS4, Line 253: for (int i = 1; i <= 1; i++) {
> remove this
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@255
PS4, Line 255: i+1
> style nit: please put spaces around infix operators
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@275
PS4, Line 275: p1 ++
> style nit: please no spaces before postfix operators
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@297
PS4, Line 297:
> style nit: collapse >1 consecutive empty lines into a single empty line so
Done
http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@306
PS4, Line 306: void ScanTablet(ScanSpec *spec, vector *results, const
char *descr) {
> warning: parameter 'descr' is unused
[kudu-CR] Kudu-1291 Efficiently support predicates on non-prefix key components
Hello Tidy Bot, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10983 to look at the new patch set (#5). Change subject: Kudu-1291 Efficiently support predicates on non-prefix key components .. Kudu-1291 Efficiently support predicates on non-prefix key components This patch implements index skip scan approach in the case when there exists an equality predicate on any of the non-first primary key columns. This feature uses the following approach: 1. Seek at the first unique prefix key in the ad-hoc index tree. 2. Seek to the relevant entry corresponding to the unique prefix key found in 1. and the predicate value on the suffix key column. 3. To handle the case where multiple entries exist with respect to a unique prefix key, store the upper bound index of these entries. 4. Repeat steps 1-3, till all the unique prefix keys are scanned. Change-Id: I230cd5a288e28ace796b352a603e0d1bcc1e4e0f --- M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/common/encoded_key.cc M src/kudu/common/encoded_key.h M src/kudu/common/partial_row.h M src/kudu/tablet/CMakeLists.txt M src/kudu/tablet/cfile_set.cc M src/kudu/tablet/cfile_set.h A src/kudu/tablet/index_skipscan-test.cc 9 files changed, 788 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10983/5 -- To view, visit http://gerrit.cloudera.org:8080/10983 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I230cd5a288e28ace796b352a603e0d1bcc1e4e0f Gerrit-Change-Number: 10983 Gerrit-PatchSet: 5 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Anupama Gupta Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] Kudu-1291 Efficiently support predicates on non-prefix key components
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11067 to look at the new patch set (#13). Change subject: Kudu-1291 Efficiently support predicates on non-prefix key components .. Kudu-1291 Efficiently support predicates on non-prefix key components This patch implements index skip scan approach in the case when there exists an equality predicate on any of the non-first primary key columns. This feature uses the following approach: 1. Seek at the first unique prefix key in the ad-hoc index tree. 2. Seek to the relevant entry corresponding to the unique prefix key found in 1. and the predicate value on the suffix key column. 3. To handle the case where multiple entries exist with respect to a unique prefix key, store the upper bound index of these entries. 4. Repeat steps 1-3, till all the unique prefix keys are scanned. Change-Id: Iffbc240233e5ad52f6a1c7912d4e44baf32a5f2d --- M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/common/encoded_key.cc M src/kudu/common/encoded_key.h M src/kudu/common/partial_row.h M src/kudu/tablet/CMakeLists.txt M src/kudu/tablet/cfile_set.cc M src/kudu/tablet/cfile_set.h A src/kudu/tablet/index_skipscan-test.cc 9 files changed, 788 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/11067/13 -- To view, visit http://gerrit.cloudera.org:8080/11067 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iffbc240233e5ad52f6a1c7912d4e44baf32a5f2d Gerrit-Change-Number: 11067 Gerrit-PatchSet: 13 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] [tools] run rebalancer during 'election storm'
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11107 Change subject: [tools] run rebalancer during 'election storm' .. [tools] run rebalancer during 'election storm' Added an integration test to run the rebalancer tool in an 'election storm' environment. The rebalancer should run with no issues unless a tablet server is reported as unavailable. Change-Id: Ic98684dbe55049bbc411513faa0b6bbaef20f434 --- M src/kudu/tools/kudu-admin-test.cc 1 file changed, 154 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11107/1 -- To view, visit http://gerrit.cloudera.org:8080/11107 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ic98684dbe55049bbc411513faa0b6bbaef20f434 Gerrit-Change-Number: 11107 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] Kudu-1291 Efficiently support predicates on non-prefix key components
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11067 to look at the new patch set (#12). Change subject: Kudu-1291 Efficiently support predicates on non-prefix key components .. Kudu-1291 Efficiently support predicates on non-prefix key components This patch implements index skip scan approach in the case when there exists an equality predicate on any of the non-first primary key columns. This feature uses the following approach: 1. Seek at the first unique prefix key in the ad-hoc index tree. 2. Seek to the relevant entry corresponding to the unique prefix key found in 1. and the predicate value on the suffix key column. 3. To handle the case where multiple entries exist with respect to a unique prefix key, store the upper bound index of these entries. 4. Repeat steps 1-3, till all the unique prefix keys are scanned. Change-Id: Iffbc240233e5ad52f6a1c7912d4e44baf32a5f2d --- M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/common/encoded_key.cc M src/kudu/common/encoded_key.h M src/kudu/common/partial_row.h M src/kudu/tablet/CMakeLists.txt M src/kudu/tablet/cfile_set.cc M src/kudu/tablet/cfile_set.h A src/kudu/tablet/index_skipscan-test.cc 9 files changed, 784 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/11067/12 -- To view, visit http://gerrit.cloudera.org:8080/11067 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iffbc240233e5ad52f6a1c7912d4e44baf32a5f2d Gerrit-Change-Number: 11067 Gerrit-PatchSet: 12 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] Kudu-1291 Efficiently support predicates on non-prefix key components
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11067 to look at the new patch set (#11). Change subject: Kudu-1291 Efficiently support predicates on non-prefix key components .. Kudu-1291 Efficiently support predicates on non-prefix key components This patch implements index skip scan approach in the case when there exists an equality predicate on any of the non-first primary key columns. This feature uses the following approach: 1. Seek at the first unique prefix key in the ad-hoc index tree. 2. Seek to the relevant entry corresponding to the unique prefix key found in 1. and the predicate value on the suffix key column. 3. To handle the case where multiple entries exist with respect to a unique prefix key, store the upper bound index of these entries. 4. Repeat steps 1-3, till all the unique prefix keys are scanned. Change-Id: Iffbc240233e5ad52f6a1c7912d4e44baf32a5f2d --- M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/common/encoded_key.cc M src/kudu/common/encoded_key.h M src/kudu/common/partial_row.h M src/kudu/tablet/CMakeLists.txt M src/kudu/tablet/cfile_set.cc M src/kudu/tablet/cfile_set.h A src/kudu/tablet/index_skipscan-test.cc 9 files changed, 798 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/11067/11 -- To view, visit http://gerrit.cloudera.org:8080/11067 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iffbc240233e5ad52f6a1c7912d4e44baf32a5f2d Gerrit-Change-Number: 11067 Gerrit-PatchSet: 11 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 ) Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. Patch Set 16: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 16 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 01 Aug 2018 20:40:11 + Gerrit-HasComments: No
[kudu-CR] KUDU-2524: temporarily disable scalafmt in gradle build
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11101 ) Change subject: KUDU-2524: temporarily disable scalafmt in gradle build .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I30cf5cfaf72b65c0178bc951e4abb75da0ad7069 Gerrit-Change-Number: 11101 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tony Foerster
[kudu-CR] KUDU-2524: temporarily disable scalafmt in gradle build
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11101 ) Change subject: KUDU-2524: temporarily disable scalafmt in gradle build .. KUDU-2524: temporarily disable scalafmt in gradle build Change-Id: I30cf5cfaf72b65c0178bc951e4abb75da0ad7069 Reviewed-on: http://gerrit.cloudera.org:8080/11101 Reviewed-by: Mike Percy Tested-by: Adar Dembo --- M build-support/jenkins/build-and-test.sh 1 file changed, 3 insertions(+), 0 deletions(-) Approvals: Mike Percy: Looks good to me, approved Adar Dembo: Verified -- To view, visit http://gerrit.cloudera.org:8080/11101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I30cf5cfaf72b65c0178bc951e4abb75da0ad7069 Gerrit-Change-Number: 11101 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tony Foerster
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Shriya Gupta has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 ) Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. Patch Set 16: (2 comments) http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc@265 PS14, Line 265: : // Not all Kudu tablenames are also valid Impala identifiers. We need to : // replace such names with a placeholder when they are used as Impala > These lines look like they're too long: please wrap at 80 characters. Done http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc@273 PS14, Line 273: // 4. Every character must be alphanumeric or an underscore. > Style nit: when you want to emphasize that a comment applies to a particula That's a good pointer. Done. -- To view, visit http://gerrit.cloudera.org:8080/10656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 16 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 01 Aug 2018 20:08:11 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10656 to look at the new patch set (#16). Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the tablename tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to derive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results. We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore with this update, invalid tablenames have been identified and updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 28 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/16 -- To view, visit http://gerrit.cloudera.org:8080/10656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 16 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
[kudu-CR] hms-tool: lookup master addresses config from master
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11083 ) Change subject: hms-tool: lookup master addresses config from master .. Patch Set 8: Code-Review+2 Carrying over Hao's +2. -- To view, visit http://gerrit.cloudera.org:8080/11083 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If170cebe6e5d7fa05fbd7faf18755aa57bdfeeec Gerrit-Change-Number: 11083 Gerrit-PatchSet: 8 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 01 Aug 2018 19:47:29 + Gerrit-HasComments: No
[kudu-CR] hms precheck tool
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11040 ) Change subject: hms precheck tool .. hms precheck tool This commit adds a 'kudu hms precheck' tool that is meant to be used before enabling the HMS integration on an existing cluster. The tool checks for tables whose names are not unique when compared with Hive's case-insensitive identifier rules. These conflicting tables would cause the master to fail to startup after enabling the Hive Metastore integration. Change-Id: Ia7b23cb802b6434eba7e38443d3361ef70e6543e Reviewed-on: http://gerrit.cloudera.org:8080/11040 Reviewed-by: Alexey Serbin Tested-by: Kudu Jenkins Reviewed-by: Hao Hao --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 2 files changed, 144 insertions(+), 15 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Kudu Jenkins: Verified Hao Hao: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ia7b23cb802b6434eba7e38443d3361ef70e6543e Gerrit-Change-Number: 11040 Gerrit-PatchSet: 13 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2524: temporarily disable scalafmt in gradle build
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11101 ) Change subject: KUDU-2524: temporarily disable scalafmt in gradle build .. Patch Set 2: Sorry for rebasing, I thought there was a conflict. -- To view, visit http://gerrit.cloudera.org:8080/11101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30cf5cfaf72b65c0178bc951e4abb75da0ad7069 Gerrit-Change-Number: 11101 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tony Foerster Gerrit-Comment-Date: Wed, 01 Aug 2018 19:40:08 + Gerrit-HasComments: No
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10656 to look at the new patch set (#15). Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the tablename tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to derive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results. We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore with this update, invalid tablenames have been identified and updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 25 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/15 -- To view, visit http://gerrit.cloudera.org:8080/10656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 15 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
[kudu-CR] KUDU-2524: temporarily disable scalafmt in gradle build
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11101 ) Change subject: KUDU-2524: temporarily disable scalafmt in gradle build .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30cf5cfaf72b65c0178bc951e4abb75da0ad7069 Gerrit-Change-Number: 11101 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tony Foerster Gerrit-Comment-Date: Wed, 01 Aug 2018 19:38:52 + Gerrit-HasComments: No
[kudu-CR] [tools] --report only option for 'kudu cluster rebalance'
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11097 ) Change subject: [tools] --report_only option for 'kudu cluster rebalance' .. [tools] --report_only option for 'kudu cluster rebalance' Added --report_only option for the 'kudu cluster rebalance' CLI tool along with corresponding test. Change-Id: Id8a81448a31ba76bb651b13e052f2c508cd0acd6 Reviewed-on: http://gerrit.cloudera.org:8080/11097 Reviewed-by: Will Berkeley Tested-by: Kudu Jenkins --- M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_cluster.cc 2 files changed, 47 insertions(+), 0 deletions(-) Approvals: Will Berkeley: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/11097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Id8a81448a31ba76bb651b13e052f2c508cd0acd6 Gerrit-Change-Number: 11097 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [tools] --report only option for 'kudu cluster rebalance'
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11097 ) Change subject: [tools] --report_only option for 'kudu cluster rebalance' .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8a81448a31ba76bb651b13e052f2c508cd0acd6 Gerrit-Change-Number: 11097 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 01 Aug 2018 19:00:34 + Gerrit-HasComments: No
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 ) Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. Patch Set 14: (2 comments) http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc@265 PS14, Line 265: // Not all Kudu tablenames are also valid Impala identifiers. We need to replace such names : // with a placeholder when they are used as Impala identifiers, for example as Impala tablename : // in the Kudu Web UI. Valid Impala identifiers conform to the following rules: These lines look like they're too long: please wrap at 80 characters. http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc@273 PS14, Line 273: string impala_name = ""; Style nit: when you want to emphasize that a comment applies to a particular block of code, format it like this: What's important here? That the comment and the block of code aren't separated by an empty line, and that the two are separated from the code before and after with empty lines. This helps emphasize that the comment and the code should be considered together. -- To view, visit http://gerrit.cloudera.org:8080/10656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 14 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 01 Aug 2018 18:58:08 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2524: temporarily disable scalafmt in gradle build
Tony Foerster has posted comments on this change. ( http://gerrit.cloudera.org:8080/11101 ) Change subject: KUDU-2524: temporarily disable scalafmt in gradle build .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30cf5cfaf72b65c0178bc951e4abb75da0ad7069 Gerrit-Change-Number: 11101 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tony Foerster Gerrit-Comment-Date: Wed, 01 Aug 2018 18:47:01 + Gerrit-HasComments: No
[kudu-CR] hms precheck tool
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11040 ) Change subject: hms precheck tool .. Patch Set 12: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia7b23cb802b6434eba7e38443d3361ef70e6543e Gerrit-Change-Number: 11040 Gerrit-PatchSet: 12 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 01 Aug 2018 18:40:35 + Gerrit-HasComments: No
[kudu-CR] KUDU-2524: temporarily disable scalafmt in gradle build
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11101 ) Change subject: KUDU-2524: temporarily disable scalafmt in gradle build .. Patch Set 1: I verified locally that this causes scalafmt tasks to not show up in the list of tasks executed, but have yet to verify it end to end on an older JDK8 due to several infrastructure issues. -- To view, visit http://gerrit.cloudera.org:8080/11101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30cf5cfaf72b65c0178bc951e4abb75da0ad7069 Gerrit-Change-Number: 11101 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tony Foerster Gerrit-Comment-Date: Wed, 01 Aug 2018 18:36:19 + Gerrit-HasComments: No
[kudu-CR] KUDU-2524: temporarily disable scalafmt in gradle build
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11101 ) Change subject: KUDU-2524: temporarily disable scalafmt in gradle build .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30cf5cfaf72b65c0178bc951e4abb75da0ad7069 Gerrit-Change-Number: 11101 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tony Foerster Gerrit-Comment-Date: Wed, 01 Aug 2018 18:36:25 + Gerrit-HasComments: No
[kudu-CR] hms tools: do not require HMS configuration flags
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 ) Change subject: hms tools: do not require HMS configuration flags .. Patch Set 14: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 14 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 01 Aug 2018 18:35:43 + Gerrit-HasComments: No
[kudu-CR] KUDU-2524: temporarily disable scalafmt in gradle build
Hello Tony Foerster, Andrew Wong, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11101 to review the following change. Change subject: KUDU-2524: temporarily disable scalafmt in gradle build .. KUDU-2524: temporarily disable scalafmt in gradle build Change-Id: I30cf5cfaf72b65c0178bc951e4abb75da0ad7069 --- M build-support/jenkins/build-and-test.sh 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/11101/1 -- To view, visit http://gerrit.cloudera.org:8080/11101 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I30cf5cfaf72b65c0178bc951e4abb75da0ad7069 Gerrit-Change-Number: 11101 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Tony Foerster
[kudu-CR] hms tools: do not require HMS configuration flags
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 ) Change subject: hms tools: do not require HMS configuration flags .. Patch Set 14: (1 comment) http://gerrit.cloudera.org:8080/#/c/11036/13/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11036/13/src/kudu/tools/kudu-tool-test.cc@2230 PS13, Line 2230: ore_sasl_enabled are automatic > nit: Would you mind adding a comment to say that this is to test without pr Done -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 14 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 01 Aug 2018 18:34:31 + Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11036 to look at the new patch set (#14). Change subject: hms tools: do not require HMS configuration flags .. hms tools: do not require HMS configuration flags The HMS tools will now lookup required HMS configuration (hive_metastore_uris and hive_metastore_sasl_enabled) from the master when it is not provided via command line flags. Looking up the configs on the master is more ergonomic and less error-prone. Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 --- M src/kudu/hms/hms_catalog.cc M src/kudu/mini-cluster/external_mini_cluster.cc 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 6 files changed, 157 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/14 -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 14 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10656 to look at the new patch set (#14). Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the tablename tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to derive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results. We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore with this update, invalid tablenames have been identified and updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 22 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/14 -- To view, visit http://gerrit.cloudera.org:8080/10656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 14 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
[kudu-CR] hms tools: do not require HMS configuration flags
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 ) Change subject: hms tools: do not require HMS configuration flags .. Patch Set 13: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11036/13/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11036/13/src/kudu/tools/kudu-tool-test.cc@2230 PS13, Line 2230: hms downgrade $0", master_addr nit: Would you mind adding a comment to say that this is to test without providing hive_metastore_uris and hive_metastore_sasl_enabled via command line flags, it also works. -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 13 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 01 Aug 2018 18:27:25 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Shriya Gupta has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 ) Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. Patch Set 13: (7 comments) > (7 comments) > > I filed KUDU-2523 for the test failure, which looks unrelated to > your patch. Yep, thank you, I wasn't sure why a commit message update on a successful build would warrant/fail a debug build test. I have made the other updates too. http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@12 PS12, Line 12: ablename > Across this commit message (and the code), you're writing both "table name" Done http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@17 PS12, Line 17: deriv > derive Done http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@19 PS12, Line 19: W > Nit: space before "We" Done http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@21 PS12, Line 21: invalid tablenames have been identified and updated by a placeholder : tablename for end user to replace with a valid tablename. The rules to > Nit: you have trailing whitespace on these two lines. Done http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@269 PS5, Line 269: // 2. Length must not exceed 128 characters. > I don't think a combined if statement with line breaks is so bad: Done http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc@268 PS12, Line 268: // 1. Must not be empty. > Style nit: format single-line comments as "// foo bar" not "//foo bar". Done http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc@270 PS12, Line 270: // 3. First character must be alphabetic. > I think these comments would be more readable if grouped into a single bloc Done -- To view, visit http://gerrit.cloudera.org:8080/10656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 13 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 01 Aug 2018 18:16:20 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10656 to look at the new patch set (#13). Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the tablename tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to derive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results. We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore with this update, invalid tablenames have been identified and updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 22 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/13 -- To view, visit http://gerrit.cloudera.org:8080/10656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 13 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
[kudu-CR] [tools] --report only option for 'kudu cluster rebalance'
Hello Will Berkeley, Fengling Wang, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11097 to look at the new patch set (#3). Change subject: [tools] --report_only option for 'kudu cluster rebalance' .. [tools] --report_only option for 'kudu cluster rebalance' Added --report_only option for the 'kudu cluster rebalance' CLI tool along with corresponding test. Change-Id: Id8a81448a31ba76bb651b13e052f2c508cd0acd6 --- M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_cluster.cc 2 files changed, 47 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/11097/3 -- To view, visit http://gerrit.cloudera.org:8080/11097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id8a81448a31ba76bb651b13e052f2c508cd0acd6 Gerrit-Change-Number: 11097 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Will Berkeley
[kudu-CR] [tools] --report only option for 'kudu cluster rebalance'
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11097 ) Change subject: [tools] --report_only option for 'kudu cluster rebalance' .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11097/2/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/11097/2/src/kudu/tools/kudu-admin-test.cc@1370 PS2, Line 1370: ASSERT_STR_NOT_CONTAINS > You meant ASSERT_STR_CONTAINS, I think? It's funny then because the precomm Indeed :) That's really funny -- I will take a closer look. -- To view, visit http://gerrit.cloudera.org:8080/11097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8a81448a31ba76bb651b13e052f2c508cd0acd6 Gerrit-Change-Number: 11097 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 01 Aug 2018 17:41:06 + Gerrit-HasComments: Yes
[kudu-CR] [tools] --report only option for 'kudu cluster rebalance'
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11097 ) Change subject: [tools] --report_only option for 'kudu cluster rebalance' .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11097/2/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/11097/2/src/kudu/tools/kudu-admin-test.cc@1370 PS2, Line 1370: ASSERT_STR_NOT_CONTAINS You meant ASSERT_STR_CONTAINS, I think? It's funny then because the precommit runs of this test all passed. -- To view, visit http://gerrit.cloudera.org:8080/11097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8a81448a31ba76bb651b13e052f2c508cd0acd6 Gerrit-Change-Number: 11097 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 01 Aug 2018 17:39:29 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/10656 )
Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
..
Patch Set 12:
(7 comments)
I filed KUDU-2523 for the test failure, which looks unrelated to your patch.
http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@12
PS12, Line 12: tablename
Across this commit message (and the code), you're writing both "table name" and
"tablename". Can you pick one term and use it consistently?
http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@17
PS12, Line 17: drive
derive
http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@19
PS12, Line 19: We
Nit: space before "We"
http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@21
PS12, Line 21: invalid tablenames have been identified and updated by a
placeholder
: tablename for end user to replace with a valid tablename. The
rules to
Nit: you have trailing whitespace on these two lines.
http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@269
PS5, Line 269: if (orig_name.length() <= 128) {
> Combining these if-conditions exceeds the total code line length requiremen
I don't think a combined if statement with line breaks is so bad:
if (!orig_name.empty() &&
orig_name.length() <= 128 &&
ascii_isalpha(orig_name[0]) &&
find_if(orig_name.begin(), orig_name.end(), [](char c) {
return !(isalnum(c) || (c == '_'));
}) == orig_name.end()) {
impala_name = orig_name;
}
http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc@268
PS12, Line 268: //Ensuring non-zero-length tablename
Style nit: format single-line comments as "// foo bar" not "//foo bar".
http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc@270
PS12, Line 270: //Ensuring that the tablename length does not exceed
128 characters
I think these comments would be more readable if grouped into a single block at
the beginning. Something like:
// Not all Kudu table names are also valid Impala identifiers. We need to
replace such names
// with a placeholder because ...
//
// Valid Impala identifiers conform to the following rules:
// 1. Must not be empty.
// 2. Length must not exceed 128 characters.
// 3. First character must be alphabetic.
// 4. Every character must be alphanumeric or an underscore.
--
To view, visit http://gerrit.cloudera.org:8080/10656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 12
Gerrit-Owner: Shriya Gupta
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Wed, 01 Aug 2018 17:13:45 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10656 to look at the new patch set (#12). Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the table name tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to drive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results.We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore with this update, invalid tablenames have been identified and updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 24 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/12 -- To view, visit http://gerrit.cloudera.org:8080/10656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 12 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
[kudu-CR] Implement BloomFilter Predicate in server side.
ZhangYao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11100 Change subject: Implement BloomFilter Predicate in server side. .. Implement BloomFilter Predicate in server side. Change-Id: I62c2de42667d0255d94e19db773240f7f9ee636c --- M src/kudu/common/column_predicate-test.cc M src/kudu/common/column_predicate.cc M src/kudu/common/column_predicate.h M src/kudu/common/common.proto M src/kudu/common/key_util.cc M src/kudu/common/scan_spec.cc M src/kudu/common/wire_protocol-test.cc M src/kudu/common/wire_protocol.cc M src/kudu/tablet/cfile_set-test.cc 9 files changed, 988 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/11100/1 -- To view, visit http://gerrit.cloudera.org:8080/11100 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I62c2de42667d0255d94e19db773240f7f9ee636c Gerrit-Change-Number: 11100 Gerrit-PatchSet: 1 Gerrit-Owner: ZhangYao
