You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2017/11/01 19:29:07 UTC
[kudu-CR] cfile set: don't reset BloomFileReader on error
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8440
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
cfile_set: don't reset BloomFileReader on error
There is a race between UpdateCompactionStats and the error path of
CFileSet::FindRow(). When FindRow() fails to check its BloomFileReader,
it clears it, but the reader is necessary to determine the rowset size.
Both access the bloom reader without locks.
The reset is an optimization to allow the CFileSet to be scanned
post-error without having to read the bloom file. This isn't compelling:
the failure of a bloom reader doesn't affect the correctness of a scan,
and if the error is a permanent error (e.g. a disk failure), it would be
desirable to eventually fail and replace the tablet; if the error is
transient, then re-reading the bloom filter could yield successful
scans. In either case, clearing the reader doesn't seem necessary.
This patch removes the BloomFileReader reset on error.
Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
---
M src/kudu/tablet/cfile_set.cc
1 file changed, 2 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/8440/1
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed a vote on this change.
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
Removed Code-Review+1 by Andrew Wong <aw...@cloudera.com>
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8440 )
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8440/1//COMMIT_MSG
Commit Message:
PS1:
> Makes sense, but given the age of this "optimization", could you get a seco
looks fine to me (Andrew chatted about it with me ahead of writing the patch)
http://gerrit.cloudera.org:8080/#/c/8440/1/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:
http://gerrit.cloudera.org:8080/#/c/8440/1/src/kudu/tablet/cfile_set.cc@258
PS1, Line 258: LOG(WARNING) << "Unable to query bloom: " << s.ToString();
in the case that we somehow have a corrupt bloom file, maybe this warning would start flooding the logs? I think we'd be better off if we included a few more things in the log message (eg the block id) and also used a KLOG_EVERY_N_SECS macro to ensure it doesn't turn into a flood
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 00:06:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8440 )
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 18:43:22 +0000
Gerrit-HasComments: No
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8440 )
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8440/1//COMMIT_MSG
Commit Message:
PS1:
> looks fine to me (Andrew chatted about it with me ahead of writing the patc
Done
http://gerrit.cloudera.org:8080/#/c/8440/1/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:
http://gerrit.cloudera.org:8080/#/c/8440/1/src/kudu/tablet/cfile_set.cc@244
PS1, Line 244:
> Under what circumstances is bloom_reader_ null now? Seems like any CFileSet
Ah, you're right. Removed this.
http://gerrit.cloudera.org:8080/#/c/8440/1/src/kudu/tablet/cfile_set.cc@258
PS1, Line 258: *idx = boost::none;
> in the case that we somehow have a corrupt bloom file, maybe this warning
Done
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 01:40:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8440 )
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8440/1//COMMIT_MSG
Commit Message:
PS1:
Makes sense, but given the age of this "optimization", could you get a second opinion from Todd, who originally wrote it?
http://gerrit.cloudera.org:8080/#/c/8440/1/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:
http://gerrit.cloudera.org:8080/#/c/8440/1/src/kudu/tablet/cfile_set.cc@244
PS1, Line 244: if (bloom_reader_ && FLAGS_consult_bloom_filters) {
Under what circumstances is bloom_reader_ null now? Seems like any CFileSet would always have one created, no?
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 01 Nov 2017 20:02:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8440 )
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
cfile_set: don't reset BloomFileReader on error
There is a race between UpdateCompactionStats and the error path of
CFileSet::FindRow(). When FindRow() fails to check its BloomFileReader,
it clears it, but the reader is necessary to determine the rowset size.
Both access the bloom reader without locks.
The reset is an optimization to allow the CFileSet to be scanned
post-error without having to read the bloom file. This isn't compelling:
the failure of a bloom reader doesn't affect the correctness of a scan,
and if the error is a permanent error (e.g. a disk failure), it would be
desirable to eventually fail and replace the tablet; if the error is
transient, then re-reading the bloom filter could yield successful
scans. In either case, clearing the reader doesn't seem necessary.
This patch removes the BloomFileReader reset on error. A couple of
places previously checked the presence of the bloom reader because of
the reset; this is no longer necessary. Also includes some updates to
style in a few related places.
Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Reviewed-on: http://gerrit.cloudera.org:8080/8440
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/cfile/bloomfile-test-base.h
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
6 files changed, 33 insertions(+), 36 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8440 )
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8440/2/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:
http://gerrit.cloudera.org:8080/#/c/8440/2/src/kudu/tablet/cfile_set.cc@35
PS2, Line 35: #include "kudu/common/rowid.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 04:54:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8440
to look at the new patch set (#3).
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
cfile_set: don't reset BloomFileReader on error
There is a race between UpdateCompactionStats and the error path of
CFileSet::FindRow(). When FindRow() fails to check its BloomFileReader,
it clears it, but the reader is necessary to determine the rowset size.
Both access the bloom reader without locks.
The reset is an optimization to allow the CFileSet to be scanned
post-error without having to read the bloom file. This isn't compelling:
the failure of a bloom reader doesn't affect the correctness of a scan,
and if the error is a permanent error (e.g. a disk failure), it would be
desirable to eventually fail and replace the tablet; if the error is
transient, then re-reading the bloom filter could yield successful
scans. In either case, clearing the reader doesn't seem necessary.
This patch removes the BloomFileReader reset on error. A couple of
places previously checked the presence of the bloom reader because of
the reset; this is no longer necessary. Also includes some updates to
style in a few related places.
Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
---
M src/kudu/cfile/bloomfile-test-base.h
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
6 files changed, 35 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/8440/3
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8440 )
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
Patch Set 4: Code-Review+1
Failure of negotiation-test seems unrelated.
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 05:34:25 +0000
Gerrit-HasComments: No
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8440 )
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8440/3/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:
http://gerrit.cloudera.org:8080/#/c/8440/3/src/kudu/tablet/cfile_set.cc@39
PS3, Line 39: #include "kudu/gutil/dynamic_annotations.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 05:06:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8440
to look at the new patch set (#2).
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
cfile_set: don't reset BloomFileReader on error
There is a race between UpdateCompactionStats and the error path of
CFileSet::FindRow(). When FindRow() fails to check its BloomFileReader,
it clears it, but the reader is necessary to determine the rowset size.
Both access the bloom reader without locks.
The reset is an optimization to allow the CFileSet to be scanned
post-error without having to read the bloom file. This isn't compelling:
the failure of a bloom reader doesn't affect the correctness of a scan,
and if the error is a permanent error (e.g. a disk failure), it would be
desirable to eventually fail and replace the tablet; if the error is
transient, then re-reading the bloom filter could yield successful
scans. In either case, clearing the reader doesn't seem necessary.
This patch removes the BloomFileReader reset on error. A couple of
places previously checked the presence of the bloom reader because of
the reset; this is no longer necessary. Also includes some updates to
style in a few related places.
Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
---
M src/kudu/cfile/bloomfile-test-base.h
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
6 files changed, 35 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/8440/2
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8440 )
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
Patch Set 4: Verified+1
Andrew meant to add a +1 Verified, not a +1 Code-Review.
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 18:43:59 +0000
Gerrit-HasComments: No
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8440 )
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
Patch Set 2: Code-Review+2
Though looks like clang-tidy had a few comments.
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 01:54:42 +0000
Gerrit-HasComments: No
[kudu-CR] cfile set: don't reset BloomFileReader on error
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8440
to look at the new patch set (#4).
Change subject: cfile_set: don't reset BloomFileReader on error
......................................................................
cfile_set: don't reset BloomFileReader on error
There is a race between UpdateCompactionStats and the error path of
CFileSet::FindRow(). When FindRow() fails to check its BloomFileReader,
it clears it, but the reader is necessary to determine the rowset size.
Both access the bloom reader without locks.
The reset is an optimization to allow the CFileSet to be scanned
post-error without having to read the bloom file. This isn't compelling:
the failure of a bloom reader doesn't affect the correctness of a scan,
and if the error is a permanent error (e.g. a disk failure), it would be
desirable to eventually fail and replace the tablet; if the error is
transient, then re-reading the bloom filter could yield successful
scans. In either case, clearing the reader doesn't seem necessary.
This patch removes the BloomFileReader reset on error. A couple of
places previously checked the presence of the bloom reader because of
the reset; this is no longer necessary. Also includes some updates to
style in a few related places.
Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
---
M src/kudu/cfile/bloomfile-test-base.h
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
6 files changed, 33 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/8440/4
--
To view, visit http://gerrit.cloudera.org:8080/8440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Gerrit-Change-Number: 8440
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>