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/10/14 02:07:08 UTC
[kudu-CR] once: report init succeeded instead of initted
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8276
Change subject: once: report init_succeeded instead of initted
......................................................................
once: report init_succeeded instead of initted
We use KuduOnceDynamic to lazily open various objects, ensuring various
initializations happen once and only once. By design, after the first
call to its function member, a KuduOnceDynamic will report that it has
been initted, and further calls to Init() will no-op. This can be
problematic if the function fails, as in a few places, we condition on
this initted state, assuming "initted" means "init succeeded".
This patch changes KuduOnceDynamic's API to report whether the init
succeeded, still maintaining the property that the function is run once.
One codepath that is particularly vulnerable to this is the InitOnce of
DeltaFileReader, which reads some stats from disk into memory, and can
go un-initialized until scan-time. Before, if a deltafile were corrupted
and scanned, the scan would return with an error upon initializing the
DeltaFileReader (which would at that point be considered "initted"). If
it were scanned a second time, seeing that it had been initted, the
DeltaFileReader would attempt to access the in-memory stats and hit a
nullptr error. A test is added to tablet_server-test to demonstrate
that this fails reliably.
Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/fs/fs-test-util.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/once-test.cc
M src/kudu/util/once.h
12 files changed, 131 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/8276/1
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] once: report init succeeded instead of initted
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8276 )
Change subject: once: report init_succeeded instead of initted
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8276/3/src/kudu/fs/fs-test-util.h
File src/kudu/fs/fs-test-util.h:
http://gerrit.cloudera.org:8080/#/c/8276/3/src/kudu/fs/fs-test-util.h@97
PS3, Line 97: const uint64_t corrupt_offset, uint8_t flip_bit, BlockId* out_id) {
> Nit: indentation.
Done
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
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-Comment-Date: Mon, 16 Oct 2017 23:38:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] once: report init succeeded instead of initted
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8276 )
Change subject: once: report init_succeeded instead of initted
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8276/3/src/kudu/fs/fs-test-util.h
File src/kudu/fs/fs-test-util.h:
http://gerrit.cloudera.org:8080/#/c/8276/3/src/kudu/fs/fs-test-util.h@97
PS3, Line 97: const uint64_t corrupt_offset, uint8_t flip_bit, BlockId* out_id) {
Nit: indentation.
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
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-Comment-Date: Mon, 16 Oct 2017 23:33:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] once: report init succeeded instead of initted
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8276 )
Change subject: once: report init_succeeded instead of initted
......................................................................
Patch Set 1:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/fs/fs-test-util.h
File src/kudu/fs/fs-test-util.h:
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/fs/fs-test-util.h@93
PS1, Line 93: Status CorruptBlock(FsManager* fs_manager, const BlockId in_id,
> warning: function 'CorruptBlock' defined in a header file; function definit
Done
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/fs/fs-test-util.h@94
PS1, Line 94: const uint64_t corrupt_offset, uint8_t flip_bit, BlockId* out_id) {
> The name CorruptBlock suggests that it corrupts an existing block, but the
Done
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/tserver/tablet_server-test.cc@51
PS1, Line 51: #include "kudu/fs/fs.pb.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/tserver/tablet_server-test.cc@104
PS1, Line 104: using kudu::fs::ReadableBlock;
> warning: using decl 'ReadableBlock' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/tserver/tablet_server-test.cc@105
PS1, Line 105: using kudu::pb_util::ReadPBFromPath;
> warning: using decl 'ReadPBFromPath' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/tserver/tablet_server-test.cc@108
PS1, Line 108: using kudu::pb_util::WritePBToPath;
> warning: using decl 'WritePBToPath' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/tserver/tablet_server-test.cc@116
PS1, Line 116: using std::set;
> warning: using decl 'set' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/tserver/tablet_server-test.cc@1240
PS1, Line 1240: req.set_batch_size_bytes(0); // so it won't return data right away
> Why is this important?
It's not, will take it out. Included it as a part of some initial testing.
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/util/once.h
File src/kudu/util/once.h:
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/util/once.h@52
PS1, Line 52: // More versatile version of GoogleOnceDynamic, including the following:
> Would you mind rewriting the block comment to describe the once semantics a
Done
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/util/once.h@101
PS1, Line 101: friend void internal::InitCb(void* arg);
> warning: redundant 'InitCb' declaration [readability-redundant-declaration]
Ignoring this.
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
Gerrit-PatchSet: 1
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-Comment-Date: Mon, 16 Oct 2017 22:56:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] once: report init succeeded instead of initted
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/8276
to look at the new patch set (#5).
Change subject: once: report init_succeeded instead of initted
......................................................................
once: report init_succeeded instead of initted
We use KuduOnceDynamic to lazily open various objects, ensuring various
initializations happen once and only once. By design, after the first
call to its member function, further calls to Init() will no-op, and the
KuduOnceDynamic will report that it has been initted. This can be
problematic if the function fails, as in a few places, we condition on
this initted state, assuming "initted" means "init succeeded".
This patch changes KuduOnceDynamic's API to report whether the init
succeeded, still maintaining the property that the function is run once.
One codepath that is particularly vulnerable to this is the InitOnce of
DeltaFileReader, which reads some stats from disk into memory, and can
go un-initialized until scan-time. Before, if a deltafile were corrupted
and scanned, the scan would return with an error upon initializing the
DeltaFileReader (which would at that point be considered "initted"). If
it were scanned a second time, seeing that it had been initted, the
DeltaFileReader would attempt to access the in-memory stats and hit a
nullptr error. A test is added to tablet_server-test to demonstrate
that this fails reliably.
Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/fs/fs-test-util.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/once-test.cc
M src/kudu/util/once.h
12 files changed, 134 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/8276/5
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
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] once: report init succeeded instead of initted
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8276 )
Change subject: once: report init_succeeded instead of initted
......................................................................
Patch Set 1:
(3 comments)
Having read through the patch and having understood the problem, I agree with your solution. Looks like you have some tidy/IWYU issues to deal with.
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/fs/fs-test-util.h
File src/kudu/fs/fs-test-util.h:
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/fs/fs-test-util.h@94
PS1, Line 94: const uint64_t corrupt_offset, uint8_t flip_bit, BlockId* out_id) {
The name CorruptBlock suggests that it corrupts an existing block, but the function actually creates a new corrupt block. Perhaps rename it to reflect that? Also, could you doc it?
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/tserver/tablet_server-test.cc@1240
PS1, Line 1240: req.set_batch_size_bytes(0); // so it won't return data right away
Why is this important?
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/util/once.h
File src/kudu/util/once.h:
http://gerrit.cloudera.org:8080/#/c/8276/1/src/kudu/util/once.h@52
PS1, Line 52: // More versatile version of GoogleOnceDynamic, including the following:
Would you mind rewriting the block comment to describe the once semantics a little more cohesively?
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
Gerrit-PatchSet: 1
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-Comment-Date: Mon, 16 Oct 2017 22:11:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] once: report init succeeded instead of initted
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8276
to look at the new patch set (#2).
Change subject: once: report init_succeeded instead of initted
......................................................................
once: report init_succeeded instead of initted
We use KuduOnceDynamic to lazily open various objects, ensuring various
initializations happen once and only once. By design, after the first
call to its member function, further calls to Init() will no-op, and the
KuduOnceDynamic will report that it has been initted. This can be
problematic if the function fails, as in a few places, we condition on
this initted state, assuming "initted" means "init succeeded".
This patch changes KuduOnceDynamic's API to report whether the init
succeeded, still maintaining the property that the function is run once.
One codepath that is particularly vulnerable to this is the InitOnce of
DeltaFileReader, which reads some stats from disk into memory, and can
go un-initialized until scan-time. Before, if a deltafile were corrupted
and scanned, the scan would return with an error upon initializing the
DeltaFileReader (which would at that point be considered "initted"). If
it were scanned a second time, seeing that it had been initted, the
DeltaFileReader would attempt to access the in-memory stats and hit a
nullptr error. A test is added to tablet_server-test to demonstrate
that this fails reliably.
Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/fs/fs-test-util.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/once-test.cc
M src/kudu/util/once.h
12 files changed, 137 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/8276/2
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
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
[kudu-CR] once: report init succeeded instead of initted
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8276 )
Change subject: once: report init_succeeded instead of initted
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8276/4/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8276/4/src/kudu/tserver/tablet_server-test.cc@1208
PS4, Line 1208: BlockId new_block_id;
> is this variable used? seems like it's shadowed in the loop below
Done
http://gerrit.cloudera.org:8080/#/c/8276/4/src/kudu/tserver/tablet_server-test.cc@1248
PS4, Line 1248: TEST_F(TabletServerTest, TestScannerOpenWhenServerShutsDown) {
> can you assert more specifically on the error string here? eg an ASSERT_STR
Done
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
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>
Gerrit-Comment-Date: Tue, 17 Oct 2017 15:36:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] once: report init succeeded instead of initted
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8276 )
Change subject: once: report init_succeeded instead of initted
......................................................................
Patch Set 2:
Fixed a lint error, good for review.
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
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-Comment-Date: Mon, 16 Oct 2017 23:25:20 +0000
Gerrit-HasComments: No
[kudu-CR] once: report init succeeded instead of initted
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8276
to look at the new patch set (#3).
Change subject: once: report init_succeeded instead of initted
......................................................................
once: report init_succeeded instead of initted
We use KuduOnceDynamic to lazily open various objects, ensuring various
initializations happen once and only once. By design, after the first
call to its member function, further calls to Init() will no-op, and the
KuduOnceDynamic will report that it has been initted. This can be
problematic if the function fails, as in a few places, we condition on
this initted state, assuming "initted" means "init succeeded".
This patch changes KuduOnceDynamic's API to report whether the init
succeeded, still maintaining the property that the function is run once.
One codepath that is particularly vulnerable to this is the InitOnce of
DeltaFileReader, which reads some stats from disk into memory, and can
go un-initialized until scan-time. Before, if a deltafile were corrupted
and scanned, the scan would return with an error upon initializing the
DeltaFileReader (which would at that point be considered "initted"). If
it were scanned a second time, seeing that it had been initted, the
DeltaFileReader would attempt to access the in-memory stats and hit a
nullptr error. A test is added to tablet_server-test to demonstrate
that this fails reliably.
Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/fs/fs-test-util.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/once-test.cc
M src/kudu/util/once.h
12 files changed, 138 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/8276/3
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
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
[kudu-CR] once: report init succeeded instead of initted
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8276 )
Change subject: once: report init_succeeded instead of initted
......................................................................
Patch Set 1:
There are other ways around this specific issue, but I thought I'd implement the fix in util/once for now.
Open to changing the fix to be more specific to DeltaFileReader, but I think this behavior of KuduOnceDynamic doesn't break the current usages and makes sense.
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
Gerrit-PatchSet: 1
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-Comment-Date: Sat, 14 Oct 2017 02:14:48 +0000
Gerrit-HasComments: No
[kudu-CR] once: report init succeeded instead of initted
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8276 )
Change subject: once: report init_succeeded instead of initted
......................................................................
once: report init_succeeded instead of initted
We use KuduOnceDynamic to lazily open various objects, ensuring various
initializations happen once and only once. By design, after the first
call to its member function, further calls to Init() will no-op, and the
KuduOnceDynamic will report that it has been initted. This can be
problematic if the function fails, as in a few places, we condition on
this initted state, assuming "initted" means "init succeeded".
This patch changes KuduOnceDynamic's API to report whether the init
succeeded, still maintaining the property that the function is run once.
One codepath that is particularly vulnerable to this is the InitOnce of
DeltaFileReader, which reads some stats from disk into memory, and can
go un-initialized until scan-time. Before, if a deltafile were corrupted
and scanned, the scan would return with an error upon initializing the
DeltaFileReader (which would at that point be considered "initted"). If
it were scanned a second time, seeing that it had been initted, the
DeltaFileReader would attempt to access the in-memory stats and hit a
nullptr error. A test is added to tablet_server-test to demonstrate
that this fails reliably.
Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Reviewed-on: http://gerrit.cloudera.org:8080/8276
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/fs/fs-test-util.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/once-test.cc
M src/kudu/util/once.h
12 files changed, 134 insertions(+), 53 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
Gerrit-PatchSet: 6
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] once: report init succeeded instead of initted
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8276
to look at the new patch set (#4).
Change subject: once: report init_succeeded instead of initted
......................................................................
once: report init_succeeded instead of initted
We use KuduOnceDynamic to lazily open various objects, ensuring various
initializations happen once and only once. By design, after the first
call to its member function, further calls to Init() will no-op, and the
KuduOnceDynamic will report that it has been initted. This can be
problematic if the function fails, as in a few places, we condition on
this initted state, assuming "initted" means "init succeeded".
This patch changes KuduOnceDynamic's API to report whether the init
succeeded, still maintaining the property that the function is run once.
One codepath that is particularly vulnerable to this is the InitOnce of
DeltaFileReader, which reads some stats from disk into memory, and can
go un-initialized until scan-time. Before, if a deltafile were corrupted
and scanned, the scan would return with an error upon initializing the
DeltaFileReader (which would at that point be considered "initted"). If
it were scanned a second time, seeing that it had been initted, the
DeltaFileReader would attempt to access the in-memory stats and hit a
nullptr error. A test is added to tablet_server-test to demonstrate
that this fails reliably.
Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/fs/fs-test-util.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/once-test.cc
M src/kudu/util/once.h
12 files changed, 138 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/8276/4
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
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
[kudu-CR] once: report init succeeded instead of initted
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8276 )
Change subject: once: report init_succeeded instead of initted
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
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>
Gerrit-Comment-Date: Tue, 17 Oct 2017 16:38:39 +0000
Gerrit-HasComments: No
[kudu-CR] once: report init succeeded instead of initted
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8276 )
Change subject: once: report init_succeeded instead of initted
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8276/4/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8276/4/src/kudu/tserver/tablet_server-test.cc@1208
PS4, Line 1208: unique_ptr<RowSetDataPB> rowset_pb(new RowSetDataPB());
is this variable used? seems like it's shadowed in the loop below
http://gerrit.cloudera.org:8080/#/c/8276/4/src/kudu/tserver/tablet_server-test.cc@1248
PS4, Line 1248: ASSERT_TRUE(resp.has_error());
can you assert more specifically on the error string here? eg an ASSERT_STR_CONTAINS?
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
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: Tue, 17 Oct 2017 04:45:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] once: report init succeeded instead of initted
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8276 )
Change subject: once: report init_succeeded instead of initted
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ac7131144392d673e0a72a1ba9920bcf9fd991c
Gerrit-Change-Number: 8276
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-Comment-Date: Tue, 17 Oct 2017 00:07:00 +0000
Gerrit-HasComments: No