You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/09/06 00:37:11 UTC
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Hello Will Berkeley,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7967
to review the following change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
KUDU-2119. Fix failure in encoding-test
In commit d1f53cc32 I introduced randomization for the format string
used for the generated string data in this test. The random format
string could sometimes incorporate '\0' bytes, which, in the worst case,
could result in a string of length 0 or 1. This would then cause a later
assertion to fail that was checking that the encoded data be at least
two bytes per string.
The fix just replaces any '\0' characters in the random format string.
Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
---
M src/kudu/cfile/encoding-test.cc
1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/7967/1
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7967/3/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
PS3, Line 285: kCount
Is it possible for s.size() == kCount? Can it be exactly 1 byte per entry?
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7967/1//COMMIT_MSG
Commit Message:
Line 16: The fix just replaces any '\0' characters in the random format string.
Does this also fix http://dist-test.cloudera.org:8080/diagnose?key=a32eeefe-906c-11e7-8752-0242ac11000f? That's a slightly different failure:
[ RUN ] TestEncoding.TestBinaryPrefixBlockBuilderRoundTrip
I0902 22:56:26.483465 8078 test_util.cc:195] Using random seed: -1483122939
I0902 22:56:26.483530 8078 encoding-test.cc:288] Block: 000000: 1000 3930 1000 0000 0000 ..90......
/data/jenkins-workspace/kudu-workspace/src/kudu/cfile/encoding-test.cc:295: Failure
Failed
Bad status: Not found: no keys in data block
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7967/2/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
PS2, Line 271: return RandomString(len, &loca
> yea I think that's fine and desirable since it tests zero-length strings. T
ok, I see -- thank you for the clarification.
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7967/1/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
PS1, Line 277: LOG(INFO) << "format string: " << format_string;
nit: is it important to have this information to be output during the test even in case of success? If not, consider replaces this LOG(INFO) and LOG(INFO) at like 289 below with signle
SCOPED_TRACE(strings::Substitute("format string: $0; block: $1", format_string, HexDump(s)));
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7967/2/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
PS2, Line 101: uint
Nit: GSG says use int for loop variable.
PS2, Line 103: slices.emplace_back(to_insert[i]);
Why not slices.emplace_back(formatter(i))? Does to_insert have a purpose?
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7967/1//COMMIT_MSG
Commit Message:
Line 16: The fix just replaces any '\0' characters in the random format string.
> Does this also fix http://dist-test.cloudera.org:8080/diagnose?key=a32eeefe
it does now (ensured that it now always adds at least one element to the block)
http://gerrit.cloudera.org:8080/#/c/7967/1/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
PS1, Line 255: or '\0', but
> Will brought this up when we were discussing this earlier and thought I'd b
restructured this test substantially so now it tests random-length binary sequences including nulls
PS1, Line 277: LOG(INFO) << "format string: " << format_string;
> nit: is it important to have this information to be output during the test
removed
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7967
to look at the new patch set (#4).
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
KUDU-2119. Fix failure in encoding-test
In commit d1f53cc32 I introduced randomization for the format string
used for the generated string data in this test. The random format
string could sometimes incorporate '\0' bytes, which, in the worst case,
could result in a string of length 0 or 1. This would then cause a later
assertion to fail that was checking that the encoded data be at least
two bytes per string.
The fix switches from using a printf-style string to instead use a
std::function to generate the data. The implementation of the function
avoids using C strings and thus permits embedded null bytes.
Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
---
M src/kudu/cfile/encoding-test.cc
M src/kudu/util/random_util.cc
M src/kudu/util/random_util.h
3 files changed, 49 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/7967/4
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7967/2/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
PS2, Line 271: int len = local_rng.Uniform(8)
> What if len == 0 as a result of calling local_rng.Uniform(8)? Is that the
yea I think that's fine and desirable since it tests zero-length strings. The "1 byte per entry" is still valid since we expect there to be at least a one-byte length for each entry, even if it's zero, in the current string encodings. (perhaps in the future we would encode lengths more efficiently but not today)
http://gerrit.cloudera.org:8080/#/c/7967/3/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
Line 279: const uint kCount = r.Uniform(1000) + 1;
> Why need to '+1' here? Is it to ensure to add at least one element to the b
will do
PS3, Line 285: kCount
> Is it possible for s.size() == kCount? Can it be exactly 1 byte per entry?
given there is some footer/header stuff as well, we're guaranteed more than one byte per
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7967/3/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
Line 279: const uint kCount = r.Uniform(1000) + 1;
Why need to '+1' here? Is it to ensure to add at least one element to the block? Can you add a comment for it?
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7967/2/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
PS2, Line 103: slices.emplace_back(to_insert[i]);
> yea, Slices are just non-owned pointers, so we have to keep the string obje
:thumbsup:
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7967/2/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
PS2, Line 271: int len = local_rng.Uniform(8)
What if len == 0 as a result of calling local_rng.Uniform(8)? Is that the desired behavior?
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7967
to look at the new patch set (#2).
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
KUDU-2119. Fix failure in encoding-test
In commit d1f53cc32 I introduced randomization for the format string
used for the generated string data in this test. The random format
string could sometimes incorporate '\0' bytes, which, in the worst case,
could result in a string of length 0 or 1. This would then cause a later
assertion to fail that was checking that the encoded data be at least
two bytes per string.
The fix switches from using a printf-style string to instead use a
std::function to generate the data. The implementation of the function
avoids using C strings and thus permits embedded null bytes.
Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
---
M src/kudu/cfile/encoding-test.cc
M src/kudu/util/random_util.cc
M src/kudu/util/random_util.h
3 files changed, 41 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/7967/2
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7967/2/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
PS2, Line 101: uint
> Nit: GSG says use int for loop variable.
Done
PS2, Line 103: slices.emplace_back(to_insert[i]);
> Why not slices.emplace_back(formatter(i))? Does to_insert have a purpose?
yea, Slices are just non-owned pointers, so we have to keep the string objects alive somewhere or else the underlying memory gets freed.
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 4: Code-Review+2
I didn't actually review this, but three +1s should warrant a +2.
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7967
to look at the new patch set (#3).
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
KUDU-2119. Fix failure in encoding-test
In commit d1f53cc32 I introduced randomization for the format string
used for the generated string data in this test. The random format
string could sometimes incorporate '\0' bytes, which, in the worst case,
could result in a string of length 0 or 1. This would then cause a later
assertion to fail that was checking that the encoded data be at least
two bytes per string.
The fix switches from using a printf-style string to instead use a
std::function to generate the data. The implementation of the function
avoids using C strings and thus permits embedded null bytes.
Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
---
M src/kudu/cfile/encoding-test.cc
M src/kudu/util/random_util.cc
M src/kudu/util/random_util.h
3 files changed, 42 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/7967/3
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
KUDU-2119. Fix failure in encoding-test
In commit d1f53cc32 I introduced randomization for the format string
used for the generated string data in this test. The random format
string could sometimes incorporate '\0' bytes, which, in the worst case,
could result in a string of length 0 or 1. This would then cause a later
assertion to fail that was checking that the encoded data be at least
two bytes per string.
The fix switches from using a printf-style string to instead use a
std::function to generate the data. The implementation of the function
avoids using C strings and thus permits embedded null bytes.
Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Reviewed-on: http://gerrit.cloudera.org:8080/7967
Tested-by: Kudu Jenkins
Reviewed-by: Hao Hao <ha...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/cfile/encoding-test.cc
M src/kudu/util/random_util.cc
M src/kudu/util/random_util.h
3 files changed, 49 insertions(+), 38 deletions(-)
Approvals:
Hao Hao: Looks good to me, but someone else must approve
Andrew Wong: Looks good to me, but someone else must approve
Adar Dembo: Looks good to me, approved
Alexey Serbin: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2119. Fix failure in encoding-test
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-2119. Fix failure in encoding-test
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7967/1/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
PS1, Line 255: or '\0', but
Will brought this up when we were discussing this earlier and thought I'd bring it up: do we miss out test coverage for binary blobs that do start with '\0' or are there other tests that cover?
--
To view, visit http://gerrit.cloudera.org:8080/7967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic46d2a993235e560475d931c7b023eb5b4faf7c2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes