[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 15:
(39 comments)
http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:
http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/cfile/cfile_reader.cc@792
PS14, Line 792: // Currently this block holds encoded composite key.
: Arena arena(16*1024);
:
: DCHECK(!prepared_blocks_.empty());
:
> nit: As a general rule of thumb, if something is indicative of a programmer
Thanks for this pointer. I have removed the DCHECK() in this case.
http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/cfile/cfile_reader.cc@797
PS14, Line 797: us
> nit: I know some of the older code has this reversed, but for new code, we
Done
http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/cfile/cfile_reader.cc@871
PS14, Line 871: last_prepare_idx_ = b->first_row_idx() +
b->dblk_->GetCurrentIndex();
: last_prepare_count_ = 0;
:
: p
> Not sure if this is necessary to avoid breaking encapsulation, as I think w
ACK.
http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/cfile/cfile_reader.cc@871
PS14, Line 871: last_prepare_idx_ = b->first_row_idx() +
b->dblk_->GetCurrentIndex();
: last_prepare_count_ = 0;
:
: p
> It's worth documenting in the header the side effect of `cache_seeked_value
If `cache_seeked_value` is true, then a call to StoreCurrentValue() will not
seek an extra row. Instead, due to the side effect of using
CopyNextValues(size_t *n, ColumnDataView *dst) the first value from the last
prepared PK column block will be fetched into 'dst'.
I have now documented this in the header file.
http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:
http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/cfile/cfile_reader.cc@790
PS15, Line 790: Todo
> nit: s/Todo/TODO/
Done
http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/cfile/cfile_reader.cc@793
PS15, Line 793: 16*1024
> Why did we go from using a constant back to using a magic number? http://wi
Made this a member variable of CFileIterator as a followup of the review
comment in cfile_set.cc (L553). After discussing with Alexey, we came to the
conclusion that it is safe to initialize Arena with an initial buffer size of
1M. If required, the Arena size grows wrt the data being allocated to it.
http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/cfile/cfile_reader.cc@795
PS15, Line 795: DCHECK(!prepared_blocks_.empty());
: if (prepared_blocks_.empty()) {
: return Status::NotFound("blocks not found");
: }
> This has a code smell. https://martinfowler.com/bliki/CodeSmell.html In deb
Yes it is possible. I have removed DCHECK and handled this condition by
returning Status::NotFound.
http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/common/encoded_key.cc
File src/kudu/common/encoded_key.cc:
http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/common/encoded_key.cc@92
PS14, Line 92:gscoped_ptr
*key,
:Arena* arena, int32_t
num_columns
> Per the GSG (https://google.github.io/styleguide/cppguide.html):
Done
http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.h
File src/kudu/tablet/cfile_set.h:
http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.h@175
PS15, Line 175: // Due to the caveat(see below) listed for
SkipToNextScan(size_t *remaining),
: // this class should not reference a separate "client" class
that uses key_iter->cur_val.
> I find this part of the comment confusing and I think we should just remove
Done
http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.h@248
PS15, Line 248: Caveat:
> I don't think we need to specifically call this out as a caveat
Done
http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.h@249
PS15, Line 249: key_iter->cur_val_
> How about just key_iter_? Is that sufficient? Talking about a private membe
Ok. Makes sense. Changed this to just key->iter_.
http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.h@362
PS15, Line 362: skip_scan_num_seeks_cutoff
> missing underscore suffix on member variable
Done
http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/cfile_set.h
File src/kudu/tablet/cfile_set.h:
PS14:
> nit: s/unique prefix/distinct prefix
Done
http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/cfile_set.h@174
PS14, Line 174: //
: // Due to the caveat(see below) listed for
SkipToNextScan(size_t
[kudu-CR] KUDU-1291. Efficiently support predicates on non-prefix key components
Hello Tidy Bot, Mike Percy, Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10983 to look at the new patch set (#16). 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-test.cc 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 10 files changed, 1,261 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10983/16 -- 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: 16 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Anupama Gupta Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] Efficiently support predicates on non-prefix key components
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11252 to look at the new patch set (#4). Change subject: Efficiently support predicates on non-prefix key components .. 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: If9f5a4f2e9da39211d645b4d6d163ecdf919e5d2 --- M src/kudu/cfile/cfile-test.cc 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 10 files changed, 1,261 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/11252/4 -- To view, visit http://gerrit.cloudera.org:8080/11252 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If9f5a4f2e9da39211d645b4d6d163ecdf919e5d2 Gerrit-Change-Number: 11252 Gerrit-PatchSet: 4 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2469 pt 1: add an IOContext
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11248 ) Change subject: KUDU-2469 pt 1: add an IOContext .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h File src/kudu/fs/io_context.h: http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h@33 PS1, Line 33: // Utility class used to pass strings across many layers, avoiding excessive : // plumbing. > I'd considered going through hte iters, but there are a couple paths to CFi I don't think plumbing into the constructor makes sense; wasn't one of the goals to avoid adding new members to CFileReader? Besides, the ownership model for those classes isn't always straight-forward: IIRC, CFileSet is shared ownership because a new instance is swapped into a DRS during a compaction. Further, I expect IOContext to eventually contain information about the initiator of the IO, in addition to "static" information like the tablet ID. You can imagine implementing interesting QoS or resource management strategies based on the initiator's allowance. You wouldn't be able to account for this stuff if an IOContext was bound to a CFileReader at constructor time. However, you may be able to get away with plumbing an IOContext into the _iterator_ constructors. That means the IOContext can't change between scan chunks (thus no per-RPC state), but maybe that's good enough? Having looked at the second patch, I have an additional concern with the current approach: the loose coupling between the IO "initiator" and the IO "performer" means it's really tough to tell if we're 100% covered on all necessary SCOPED_IO_CONTEXT calls. That loose coupling means we probably can't assert that every IO path actually has an IOContext set, which makes the code more complex. -- To view, visit http://gerrit.cloudera.org:8080/11248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I645e10a2fda1f2564326b7052e6a0debc5ad738c Gerrit-Change-Number: 11248 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 17 Aug 2018 03:15:00 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2469 pt 1: add an IOContext
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11248 )
Change subject: KUDU-2469 pt 1: add an IOContext
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h
File src/kudu/fs/io_context.h:
http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h@33
PS1, Line 33: // Utility class used to pass strings across many layers,
avoiding excessive
: // plumbing.
> You're right it's a one-time cost, though I leaned this way since the inten
I'd considered going through hte iters, but there are a couple paths to CFiles
that weren't covered just going through the scan path:
- We also want the context when we Open() the Tablet; we need to read the
min/max keys to populate the rowset tree, and open blooms (lazily).
- Whenever we insert, we check if the row is present.
If we did want to plumb the IOContext _only_ where we might do IO, these cases
would need to be covered too. OTOH, these other cases (and scans via the iters)
roughly go through the same call chain:
Tablet-->DiskRowSet-->CFileSet-->{BloomFileReader/CFileReader}
`->DeltaTracker-->DeltaFileReader-->CFileReader
We could plumb down the IOContext through the constructors of these classes.
Less in the spirit of plumbing only when we do IO, but that's not really a
requirement.
--
To view, visit http://gerrit.cloudera.org:8080/11248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I645e10a2fda1f2564326b7052e6a0debc5ad738c
Gerrit-Change-Number: 11248
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 17 Aug 2018 01:30:19 +
Gerrit-HasComments: Yes
[kudu-CR] Efficiently support predicates on non-prefix key components
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11252 to look at the new patch set (#3). Change subject: Efficiently support predicates on non-prefix key components .. 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: If9f5a4f2e9da39211d645b4d6d163ecdf919e5d2 --- M src/kudu/cfile/cfile-test.cc 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 10 files changed, 1,256 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/11252/3 -- To view, visit http://gerrit.cloudera.org:8080/11252 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If9f5a4f2e9da39211d645b4d6d163ecdf919e5d2 Gerrit-Change-Number: 11252 Gerrit-PatchSet: 3 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Efficiently support predicates on non-prefix key components
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11252 to look at the new patch set (#2). Change subject: Efficiently support predicates on non-prefix key components .. 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: If9f5a4f2e9da39211d645b4d6d163ecdf919e5d2 --- M src/kudu/cfile/cfile-test.cc 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 10 files changed, 1,256 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/11252/2 -- To view, visit http://gerrit.cloudera.org:8080/11252 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If9f5a4f2e9da39211d645b4d6d163ecdf919e5d2 Gerrit-Change-Number: 11252 Gerrit-PatchSet: 2 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Efficiently support predicates on non-prefix key components
Anupama Gupta has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11252 Change subject: Efficiently support predicates on non-prefix key components .. 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: If9f5a4f2e9da39211d645b4d6d163ecdf919e5d2 --- M src/kudu/cfile/cfile-test.cc 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 10 files changed, 1,248 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/11252/1 -- To view, visit http://gerrit.cloudera.org:8080/11252 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: If9f5a4f2e9da39211d645b4d6d163ecdf919e5d2 Gerrit-Change-Number: 11252 Gerrit-PatchSet: 1 Gerrit-Owner: Anupama Gupta
[kudu-CR] [WIP] KUDU-2245 Graceful leadership transfer
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11251 Change subject: [WIP] KUDU-2245 Graceful leadership transfer .. [WIP] KUDU-2245 Graceful leadership transfer I will write more here. Need to dist-test loop the patch. Expect update soon. Change-Id: Ic97343af9eb349556424c999799ed5e2941f0083 --- M src/kudu/consensus/consensus-test-util.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_peers.cc M src/kudu/consensus/consensus_peers.h M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/consensus_queue.h M src/kudu/consensus/peer_manager.cc M src/kudu/consensus/peer_manager.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_tablet.cc M src/kudu/tools/tool_replica_util.cc M src/kudu/tools/tool_replica_util.h M src/kudu/tserver/tablet_service.cc 16 files changed, 531 insertions(+), 41 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/11251/1 -- To view, visit http://gerrit.cloudera.org:8080/11251 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ic97343af9eb349556424c999799ed5e2941f0083 Gerrit-Change-Number: 11251 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley
[kudu-CR](gh-pages) [site] Add current Apache event
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/11211 ) Change subject: [site] Add current Apache event .. Patch Set 5: > Patch Set 5: > > I don't have a strong preference. I think if you put it in the footer having > it on every page is nice. we can also show it on the top next to the Kudu logo on the rest of the pages though -- To view, visit http://gerrit.cloudera.org:8080/11211 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I945ad0432773d748bd21c21a7c55e5efca70b429 Gerrit-Change-Number: 11211 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Aug 2018 22:31:51 + Gerrit-HasComments: No
[kudu-CR] [master] replica selection honors placement policy
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11207 ) Change subject: [master] replica selection honors placement policy .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc File src/kudu/master/placement_policy.cc: http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@254 PS3, Line 254: avalailable available http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@256 PS3, Line 256: table tablet http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@264 PS3, Line 264: break; This loop combo is a little tricky. It took me a bit of thinking to see why we couldn't get stuck here. Could you add a comment about why we break / return at each point (except the one at L270 that's obvious) so it's clearer why we won't get stuck? -- 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: comment Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 16 Aug 2018 21:19:59 + Gerrit-HasComments: Yes
[kudu-CR](gh-pages) [site] Add current Apache event
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11211 ) Change subject: [site] Add current Apache event .. Patch Set 5: I don't have a strong preference. I think if you put it in the footer having it on every page is nice. -- To view, visit http://gerrit.cloudera.org:8080/11211 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I945ad0432773d748bd21c21a7c55e5efca70b429 Gerrit-Change-Number: 11211 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Aug 2018 21:10:18 + Gerrit-HasComments: No
[kudu-CR] KUDU-2469 pt 1: add an IOContext
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11248 )
Change subject: KUDU-2469 pt 1: add an IOContext
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h
File src/kudu/fs/io_context.h:
http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h@33
PS1, Line 33: // Utility class used to pass strings across many layers,
avoiding excessive
: // plumbing.
> Ugh, why do we need to store IOContext in a threadlocal? Is the plumbing in
You're right it's a one-time cost, though I leaned this way since the intended
immediate use of the IOContext is error handling. It felt like overkill to
explicitly plumb down things specifically for that. Though you raise a good
point that the IOContext may in the future be used for more substantial things.
http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h@39
PS1, Line 39: there is significantly less visibility into
: // the string being passed.
> Not sure I understand this. For one, the intention of the IOContext abstrac
Right, it should be generalizable to anything, not just strings.
It's less visible in that any arbitrary thread could call CurrentIOContext()
and hope it gets something useful back. With the plumbing alternative, it's
more obvious when something _should_ have an IOContext.
http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h@42
PS1, Line 42: class IOContext : public RefCountedThreadSafe {
> Please doc why IOContext needs to have shared ownership.
Ack
http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h@51
PS1, Line 51: void set_tablet_id(std::string tablet_id) {
: tablet_id_ = std::move(tablet_id);
: }
> Why do we need the ability to manipulate tablet_id_ via both setter and con
We don't, will remove.
http://gerrit.cloudera.org:8080/#/c/11248/1/src/kudu/fs/io_context.h@89
PS1, Line 89: IOContext::threadlocal_io_context_ = old_context_;
> Is this "nesting" feature required? Under what circumstances does a thread
It's not required, but I added it to match the "scoped" semantics we have
elsewhere. An alternative would be to disallow such preemption, perhaps with a
DCHECK or somesuch.
--
To view, visit http://gerrit.cloudera.org:8080/11248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I645e10a2fda1f2564326b7052e6a0debc5ad738c
Gerrit-Change-Number: 11248
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Thu, 16 Aug 2018 21:00:36 +
Gerrit-HasComments: Yes
[kudu-CR](gh-pages) [site] Add current Apache event
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/11211 ) Change subject: [site] Add current Apache event .. Patch Set 5: > Patch Set 5: > > yea, either bottom or top are preferable in my book but no strong feelings. Grant, do you prefer top or bottom? Also, if we do top, should it still be there on non-index pages next to the Kudu logo or should the Kudu logo replace it? -- To view, visit http://gerrit.cloudera.org:8080/11211 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I945ad0432773d748bd21c21a7c55e5efca70b429 Gerrit-Change-Number: 11211 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Aug 2018 20:35:25 + Gerrit-HasComments: No
[kudu-CR](gh-pages) [site] Add current Apache event
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11211 ) Change subject: [site] Add current Apache event .. Patch Set 5: yea, either bottom or top are preferable in my book but no strong feelings. -- To view, visit http://gerrit.cloudera.org:8080/11211 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I945ad0432773d748bd21c21a7c55e5efca70b429 Gerrit-Change-Number: 11211 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Aug 2018 19:02:06 + Gerrit-HasComments: No
[kudu-CR] WIP KUDU-2469 pt 2: fail replicas on CFile corruption
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11249 )
Change subject: WIP KUDU-2469 pt 2: fail replicas on CFile corruption
..
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11249/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11249/2//COMMIT_MSG@18
PS2, Line 18:
: I opted to add a new CFILE ErrorHandlerType enum rather than
using the
: TABLET enum (which is currently a no-op and serves to serialize
certain
: failures, see error_manager.h for more), as using the TABLET enum
would
: change the existing error handling behavior of certain errors.
I read through the comment in error_manager.h but still don't understand why we
can't reuse TABLET for these errors. I don't remember the details of our
initial discussion on error serialization, unfortunately.
http://gerrit.cloudera.org:8080/#/c/11249/2/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:
http://gerrit.cloudera.org:8080/#/c/11249/2/src/kudu/cfile/cfile_reader.cc@348
PS2, Line 348: if (ctx && !ctx->tablet_id().empty()) {
Can we assert that this exists?
http://gerrit.cloudera.org:8080/#/c/11249/2/src/kudu/fs/error_manager.h
File src/kudu/fs/error_manager.h:
http://gerrit.cloudera.org:8080/#/c/11249/2/src/kudu/fs/error_manager.h@103
PS2, Line 103: saftey
safety
--
To view, visit http://gerrit.cloudera.org:8080/11249
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f
Gerrit-Change-Number: 11249
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 16 Aug 2018 18:40:02 +
Gerrit-HasComments: Yes
[kudu-CR] WIP KUDU-2469 pt 2: fail replicas on CFile corruption
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/11249 )
Change subject: WIP KUDU-2469 pt 2: fail replicas on CFile corruption
..
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11249/2/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:
http://gerrit.cloudera.org:8080/#/c/11249/2/src/kudu/cfile/cfile_reader.cc@81
PS2, Line 81: DEFINE_double(cfile_inject_corruption, 0,
One of the reasons I didn't do this while writing the checksum code was due to
all the locations it would need to be called/used. Instead I wrote a test that
manually corrupts each bit. See cfile-test.cc CorruptAndReadBlock.
Not saying you can't do this, it's just another option.
http://gerrit.cloudera.org:8080/#/c/11249/2/src/kudu/cfile/cfile_reader.cc@227
PS2, Line 227: if (s.IsCorruption()) {
Should we use a macro for this pattern? Something similar to the
HANDLE_DISK_FAILURE macro?
http://gerrit.cloudera.org:8080/#/c/11249/2/src/kudu/fs/error_manager.h
File src/kudu/fs/error_manager.h:
http://gerrit.cloudera.org:8080/#/c/11249/2/src/kudu/fs/error_manager.h@76
PS2, Line 76: CFILE,
Do we need a new handler type? Or is TABLET good enough to re-use?
Is there a different action we would take because it's CFILE and not TABLET?
--
To view, visit http://gerrit.cloudera.org:8080/11249
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f
Gerrit-Change-Number: 11249
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 16 Aug 2018 17:30:09 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2469 pt 1: add an IOContext
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11248 ) Change subject: KUDU-2469 pt 1: add an IOContext .. Patch Set 1: NOTE: the implementation is very much inspired by util/trace.h -- To view, visit http://gerrit.cloudera.org:8080/11248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I645e10a2fda1f2564326b7052e6a0debc5ad738c Gerrit-Change-Number: 11248 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 16 Aug 2018 17:09:41 + Gerrit-HasComments: No
[kudu-CR] WIP KUDU-2469 pt 2: fail replicas on CFile corruption
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11249 to look at the new patch set (#2). Change subject: WIP KUDU-2469 pt 2: fail replicas on CFile corruption .. WIP KUDU-2469 pt 2: fail replicas on CFile corruption wip: add tests This patch adds handling for CFile corruption errors via the error manager, pulling the tablet id of interest with the SCOPED_IO_CONTEXT. The tablet id is plumbed down at various points that may incur CFile IO: - when performing any maintenance op - when opening the tablet to open the keys - when beginning a scan I opted to add a new CFILE ErrorHandlerType enum rather than using the TABLET enum (which is currently a no-op and serves to serialize certain failures, see error_manager.h for more), as using the TABLET enum would change the existing error handling behavior of certain errors. Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f --- M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/fs/error_manager.cc M src/kudu/fs/error_manager.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet_mm_ops.cc M src/kudu/tablet/tablet_replica_mm_ops.cc M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/tablet_service.cc 10 files changed, 125 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/11249/2 -- To view, visit http://gerrit.cloudera.org:8080/11249 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f Gerrit-Change-Number: 11249 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] WIP KUDU-2469 pt 2: fail replicas on CFile corruption
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11249 Change subject: WIP KUDU-2469 pt 2: fail replicas on CFile corruption .. WIP KUDU-2469 pt 2: fail replicas on CFile corruption wip: add tests This patch adds handling for CFile corruption errors via the error manager, pulling the tablet id of interest with the SCOPED_IO_CONTEXT. The tablet id is plumbed down at various points that may incur CFile IO: - when performing any maintenance op - when opening the tablet to open the keys - when beginning a scan I opted to add a new CFILE ErrorHandlerType enum rather than using the TABLET enum (which is currently a no-op and serves to serialize certain failures, see error_manager.h for more), as updating the TABLET error handling behavior would change the existing error handling behavior. Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f --- M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/fs/error_manager.cc M src/kudu/fs/error_manager.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet_mm_ops.cc M src/kudu/tablet/tablet_replica_mm_ops.cc M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/tablet_service.cc 10 files changed, 125 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/11249/1 -- To view, visit http://gerrit.cloudera.org:8080/11249 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f Gerrit-Change-Number: 11249 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] KUDU-2469 pt 1: add an IOContext
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11248 Change subject: KUDU-2469 pt 1: add an IOContext .. KUDU-2469 pt 1: add an IOContext This patch introduces the IOContext class that can be used to pass strings across layers without excessive plumbing. The goal is to use this to plumb tablet ids from the tablet/tserver layers down to the CFile layer so they can be used for error handling, e.g. by calling a callback that fails the tablet in the event of a CFile corruption. Change-Id: I645e10a2fda1f2564326b7052e6a0debc5ad738c --- M src/kudu/fs/CMakeLists.txt A src/kudu/fs/io_context-test.cc A src/kudu/fs/io_context.cc A src/kudu/fs/io_context.h 4 files changed, 210 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/11248/1 -- To view, visit http://gerrit.cloudera.org:8080/11248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I645e10a2fda1f2564326b7052e6a0debc5ad738c Gerrit-Change-Number: 11248 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR](gh-pages) [site] Add current Apache event
Anonymous Coward #425 has posted comments on this change. ( http://gerrit.cloudera.org:8080/11211 ) Change subject: [site] Add current Apache event .. Patch Set 5: > Patch Set 5: > > (1 comment) Any of those 3 look fine to me - think top may be best. -- To view, visit http://gerrit.cloudera.org:8080/11211 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I945ad0432773d748bd21c21a7c55e5efca70b429 Gerrit-Change-Number: 11211 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Anonymous Coward #425 Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Aug 2018 16:24:54 + Gerrit-HasComments: No
[kudu-CR](gh-pages) [site] Add current Apache event
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/11211 ) Change subject: [site] Add current Apache event .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/11211/4/index.html File index.html: http://gerrit.cloudera.org:8080/#/c/11211/4/index.html@21 PS4, Line 21: https://www.apache.org/events/current-event.html;>https://www.apache.org/events/current-event-234x60.png"/> > That's second one isn't bad. What about right aligned in the footer? That also looks good. http://people.apache.org/~abukor/kudusite_apachecon_bottom.png -- To view, visit http://gerrit.cloudera.org:8080/11211 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I945ad0432773d748bd21c21a7c55e5efca70b429 Gerrit-Change-Number: 11211 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Aug 2018 14:07:22 + Gerrit-HasComments: Yes
[kudu-CR](gh-pages) [site] Add current Apache event
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11211 ) Change subject: [site] Add current Apache event .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11211/4/index.html File index.html: http://gerrit.cloudera.org:8080/#/c/11211/4/index.html@21 PS4, Line 21: https://www.apache.org/events/current-event.html;>https://www.apache.org/events/current-event-234x60.png"/> > Yeah it's right there next to them. I wasn't sure where to put it, and it w That's second one isn't bad. What about right aligned in the footer? -- To view, visit http://gerrit.cloudera.org:8080/11211 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I945ad0432773d748bd21c21a7c55e5efca70b429 Gerrit-Change-Number: 11211 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Aug 2018 13:36:14 + Gerrit-HasComments: Yes
[kudu-CR] Implement BloomFilter Predicate in server side.
ZhangYao has posted comments on this change. (
http://gerrit.cloudera.org:8080/11100 )
Change subject: Implement BloomFilter Predicate in server side.
..
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11100/4/src/kudu/common/column_predicate.h
File src/kudu/common/column_predicate.h:
http://gerrit.cloudera.org:8080/#/c/11100/4/src/kudu/common/column_predicate.h@286
PS4, Line 286: }
> Add a space above. Is the comment correct, what's 'nhash'?
Done
http://gerrit.cloudera.org:8080/#/c/11100/4/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
http://gerrit.cloudera.org:8080/#/c/11100/4/src/kudu/common/column_predicate.cc@42
PS4, Line 42:
> (meta-comment) If I understand the changes to this file correctly, Range pr
True. I think your advice is very reasonable and I change the relevant code.
http://gerrit.cloudera.org:8080/#/c/11100/4/src/kudu/common/column_predicate.cc@460
PS4, Line 460: default: {
> Simplify with std::copy_if or something equivalent.
Done
http://gerrit.cloudera.org:8080/#/c/11100/4/src/kudu/common/column_predicate.cc@563
PS4, Line 563:
> likewise
Done
--
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: comment
Gerrit-Change-Id: I62c2de42667d0255d94e19db773240f7f9ee636c
Gerrit-Change-Number: 11100
Gerrit-PatchSet: 5
Gerrit-Owner: ZhangYao
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: ZhangYao
Gerrit-Comment-Date: Thu, 16 Aug 2018 09:35:11 +
Gerrit-HasComments: Yes
[kudu-CR] Implement BloomFilter Predicate in server side.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Andrew Wong, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11100 to look at the new patch set (#5). 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, 1,062 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/11100/5 -- 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: newpatchset Gerrit-Change-Id: I62c2de42667d0255d94e19db773240f7f9ee636c Gerrit-Change-Number: 11100 Gerrit-PatchSet: 5 Gerrit-Owner: ZhangYao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: ZhangYao
[kudu-CR] [docs] KUDU-1951 Fix links in docs
Attila Bukor has removed a vote on this change. Change subject: [docs] KUDU-1951 Fix links in docs .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I2c498c8a7f9d957a737c12c08d1b313df3d68007 Gerrit-Change-Number: 11239 Gerrit-PatchSet: 3 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [docs] KUDU-1951 Fix links in docs
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/11239 ) Change subject: [docs] KUDU-1951 Fix links in docs .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2c498c8a7f9d957a737c12c08d1b313df3d68007 Gerrit-Change-Number: 11239 Gerrit-PatchSet: 3 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Aug 2018 08:10:59 + Gerrit-HasComments: No
[kudu-CR](gh-pages) [site] add links required by ASF
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/11209 ) Change subject: [site] add links required by ASF .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/11209/5/_includes/bottom_common.html File _includes/bottom_common.html: http://gerrit.cloudera.org:8080/#/c/11209/5/_includes/bottom_common.html@6 PS5, Line 6: Apache Kudu, Kudu, Apache, the Apache feather logo, and the Apache Kudu > nit: please wrap if possible Done http://gerrit.cloudera.org:8080/#/c/11209/4/community.md File community.md: http://gerrit.cloudera.org:8080/#/c/11209/4/community.md@53 PS4, Line 53: * [**Apache Kudu Committers list**](/committers.html) > We were using that before so that these links work even on non-top-level gi I think you mean http://apache.github.io/kudu/ Looking at the current community.html, it seems Jekyll is smart enough to handle this properly: $ curl http://apache.github.io/kudu/community.html 2>/dev/null | grep "coding guidelines" the project coding guidelines are before In L95 you can see the source for this link looks like this: [coding guidelines](docs/contributing.html#_code_style) -- To view, visit http://gerrit.cloudera.org:8080/11209 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: Ia6983314e598eff73ac8f476924acd9d6f62196f Gerrit-Change-Number: 11209 Gerrit-PatchSet: 6 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Aug 2018 08:10:40 + Gerrit-HasComments: Yes
[kudu-CR](gh-pages) [site] add links required by ASF
Hello Mike Percy, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11209 to look at the new patch set (#6). Change subject: [site] add links required by ASF .. [site] add links required by ASF ASF requires the following links to be included in the navigation system[1]: --- * "License" should link to: http://www.apache.org/licenses/ * "Sponsorship" or "Donate" should link to: http://www.apache.org/foundation/sponsorship.html * "Thanks" should link to: http://www.apache.org/foundation/thanks.html * "Security" should link to either to a project-specific page detailing how users may securely report potential vulnerabilities, or to the main http://www.apache.org/security/ page * All projects must feature some prominent link back to the main ASF homepage at http://www.apache.org/ --- It also requires a trademark notice[2]. This commit adds all of these. [1] https://www.apache.org/foundation/marks/pmcs#navigation [2] https://www.apache.org/foundation/marks/pmcs#attributions Change-Id: Ia6983314e598eff73ac8f476924acd9d6f62196f --- M _includes/bottom_common.html M _includes/top_common.html M community.md M index.html 4 files changed, 17 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/11209/6 -- To view, visit http://gerrit.cloudera.org:8080/11209 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia6983314e598eff73ac8f476924acd9d6f62196f Gerrit-Change-Number: 11209 Gerrit-PatchSet: 6 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke 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 14:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:
http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@551
PS12, Line 551: atus CFileSet::Iterator::SeekToNextPrefixKey(size_t
num_prefix_cols, bool cache_seeked_value) {
: gscoped_ptr enc_key;
: Arena arena(kEncodedCompositeKeyMaxSize);
> Might be helpful: from util/memory/arena.h and the definition of Reset():
+1
Using dedicated Arena per iterator instance sounds good to me. It seems we can
easily reset it in the long loop of the SkipToNextScan() method.
http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:
http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/cfile_set.cc@770
PS14, Line 770: Check to see whether we have effectively seeked backwards
> We effectively have two pointers we are tracking with skip-scan: the primar
Adding a comment would definitely help. That might be a comment for the
method, as well as an extra for already existing high-level description of the
algorithm at line 656.
http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:
http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@557
PS15, Line 557: // columns of the cached value obtained after
> I think it's possible to pass an instance of arena from the upper level to
And the best approach I think is suggested by Andrew -- have an instance of
Arena as a member of CFileSet::Iterator. Probably, it's necessary to reset it
after use in every cycle of the loop in SkipToNextScan() method.
--
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: 14
Gerrit-Owner: Anupama Gupta
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Anupama Gupta
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 16 Aug 2018 07:24:09 +
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 15: (9 comments) http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@557 PS15, Line 557: Arena arena(FLAGS_max_encoded_key_size_bytes); I think it's possible to pass an instance of arena from the upper level to use it here, like it's done in DecodeCurrentKey(). BTW, that parameter for the Arena constructor -- it's just initial size for the arena, it can grow bigger up to the allowed max size (by default 127 * 8KB, i.e. almost 1 MB). However, setting it up to FLAGS_max_encoded_key_size_bytes is a good enough (arena grows 2x times when needed). http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@571 PS15, Line 571: ( nit: this and the pairing brace are not needed. http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@597 PS15, Line 597: KuduPartialRow *p_row, : const gscoped_ptr& cur_enc_key style nit: usually we have input parameter specified first, then go output parameters. http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@603 PS15, Line 603: auto const style nit: const auto& value http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@606 PS15, Line 606: p_row->Set(col_id, data); What if this returns non-OK? http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@611 PS15, Line 611: p_row->Set(skip_scan_predicate_column_id_, suffix_col_value); ditto http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@619 PS15, Line 619: ( nit: this and the closing parenthesis are not needed http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@742 PS15, Line 742: CheckPredicateMatch(lower_bound_key); I'm not sure this is what is needed: CheckPredicateMatch() verifies that only the first column of the predicate matches, but we want all columns of the predicate to match, no? If it's what we actually need, maybe add a comment that the lower bound key is just about the match of the first column of the predicate. http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@811 PS15, Line 811: ERROR I think this should be WARNING, because there are some legit cases where SkipToNextScan() can return non-OK. E.g.: E0815 21:19:24.748093 1681 cfile_set.cc:811] Skip scan failed: Illegal state: No lexicographically greater key exists -- 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: 15 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Anupama Gupta Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 16 Aug 2018 06:49:03 + Gerrit-HasComments: Yes
[kudu-CR](gh-pages) [site] Add current Apache event
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/11211 ) Change subject: [site] Add current Apache event .. Patch Set 4: (1 comment) > Patch Set 4: > > (1 comment) http://gerrit.cloudera.org:8080/#/c/11211/4/index.html File index.html: http://gerrit.cloudera.org:8080/#/c/11211/4/index.html@21 PS4, Line 21: https://www.apache.org/events/current-event.html;>https://www.apache.org/events/current-event-234x60.png"/> > mind posting a screenshot? It seems like this is going to be front-and-cent Yeah it's right there next to them. I wasn't sure where to put it, and it was incidentally the same height as the buttons, so my colleague suggested to put it there. http://people.apache.org/~abukor/kudusite_apachecon.png Any suggestions where to put it? I originally put it under the fast analytics column but it kinda felt weird there too. My wife just suggested putting it at the top before the navbar, what do you think? http://people.apache.org/~abukor/kudusite_apachecon_top.png -- To view, visit http://gerrit.cloudera.org:8080/11211 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I945ad0432773d748bd21c21a7c55e5efca70b429 Gerrit-Change-Number: 11211 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Aug 2018 06:08:23 + Gerrit-HasComments: Yes
[kudu-CR] [master] replica selection honors placement policy
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/11207 )
Change subject: [master] replica selection honors placement policy
..
Patch Set 3:
(29 comments)
Looks pretty good. I didn't get to review all of the test cases yet but I
wanted to post some feedback since it's taken me a while to start reviewing
this. Sorry about that.
http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG@7
PS3, Line 7: [master]
nit: Consider making the label [location_awareness] to be consistent with this
patch's parent.
http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG@9
PS3, Line 9: placement policy
Would you mind adding another sentence explaining the single policy that is
implemented? Just enough to cover that it is meant to be location aware and to
spread replicas evenly over tablet servers. You can refer people to the design
doc for the details.
http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG@10
PS3, Line 10:
nit: One space.
http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG@13
PS3, Line 13: Added
nit: This patch also adds a few...
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h@524
PS3, Line 524: std::string, int
Please add a short comment explaining what the key and value types are.
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h@880
PS3, Line 880: online
nit: I think the standard term in "live".
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h@880
PS3, Line 880: grouped by location
: // (as specified by 'ltd')
What is 'ltd'? Looks like this comment is stale.
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h@887
PS3, Line 887: PlacementPolicy*
nit: Naively I expected this to be a cref because something called a
PlacementPolicy would have no state. If the state necessary to do placements
will live in the PlacementPolicy class, I vote for a slightly different name.
Perhaps a PlacementAlgorithm?
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h
File src/kudu/master/placement_policy.h:
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@39
PS3, Line 39: at proper tablet servers
on tablet servers according to some rules.
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@39
PS3, Line 39: placing
place
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@40
PS3, Line 40: class PlacementPolicy {
Please add another sentence with a brief description of the specific policy. I
also think it'd be a good idea to add one more sentence stating that this class
implements a specific placement policy but it could eventually be a base class
for subclasses that implement other placement policies.
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@49
PS3, Line 49: destructed
"destroyed", or say that 'rng' must outlive of the PlacementPolicy.
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@82
PS3, Line 82: // TODO (aserbin): add the reference to the document once it's
in the repo
nit: End with .
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@100
PS3, Line 100: is
Remove extra word.
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@108
PS3, Line 108: : it's in constructor and cached
I think if we make this member const it enforces these two things.
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc
File src/kudu/master/placement_policy.cc:
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@51
PS3, Line 51:
the
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@51
PS3, Line 51:
a
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@51
PS3, Line 51: repilcas
replicas
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@52
PS3, Line 52: of
Remove extra 'of'.
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@55
PS3, Line 55: unordered_map
Is this a ReplicaLocationsInfo type?
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@57
PS3, Line 57: DCHECK(it != ltd.end());
: if (it == ltd.end()) {
: return numeric_limits::max();
: }
Is there a reason we shouldn't always crash if we break the invariant that
'location' must be a key present in 'locations_info'?
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@62
PS3, Line 62: DCHECK(!ts_descriptors.empty());
