[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. encoding-test: Clean up bitshuffle tests a little Also adds functions in random number generator to generate 128-bit integers. Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Reviewed-on: http://gerrit.cloudera.org:8080/15043 Reviewed-by: Andrew Wong Tested-by: Kudu Jenkins --- M src/kudu/cfile/encoding-test.cc M src/kudu/util/random.h M src/kudu/util/random_util-test.cc M src/kudu/util/random_util.h 4 files changed, 158 insertions(+), 34 deletions(-) Approvals: Andrew Wong: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 13 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. Patch Set 12: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h File src/kudu/util/random_util.h: http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@146 PS9, Line 146: dom(max_v > Done Thanks! -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 12 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 09 Apr 2020 01:49:52 + Gerrit-HasComments: Yes
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. Patch Set 12: (1 comment) http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h File src/kudu/util/random_util.h: http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@146 PS9, Line 146: dom(max_v > It seems convenient to place it where the suppression is, especially for su Done -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 12 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 09 Apr 2020 01:40:24 + Gerrit-HasComments: Yes
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has uploaded a new patch set (#12) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. encoding-test: Clean up bitshuffle tests a little Also adds functions in random number generator to generate 128-bit integers. Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 --- M src/kudu/cfile/encoding-test.cc M src/kudu/util/random.h M src/kudu/util/random_util-test.cc M src/kudu/util/random_util.h 4 files changed, 158 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/15043/12 -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 12 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h File src/kudu/util/random_util.h: http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@146 PS9, Line 146: exception > I think comment is better placed close to the line that raises ASAN/UBSAN w It seems convenient to place it where the suppression is, especially for such a small function. When I read the code here, I'm thinking more about random number generation than I am about ASAN, so this comment seems a little out of place, at least without more context. Maybe be a bit more explicit here then: "Hence the ASAN suppression for this template" or somesuch. -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 11 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 09 Apr 2020 01:21:32 + Gerrit-HasComments: Yes
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has posted comments on this change. (
http://gerrit.cloudera.org:8080/15043 )
Change subject: encoding-test: Clean up bitshuffle tests a little
..
Patch Set 11:
> Patch Set 11: Verified-1
>
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/21379/ : FAILURE
Unrelated test failures.
TSAN Java test failure is due to failure to start chronyd.
00:57:38.307 [INFO - cluster stderr printer] (MiniKuduCluster.java:590) W0409
00:57:38.235510 345 mini_chronyd.cc:189] Time spent starting chronyd: real
1.028s user 0.015s sys 0.038s
00:57:38.350 [DEBUG - main] (MiniKuduCluster.java:176) Response: error {
code: TIMED_OUT
message: "failed to start NTP server 0: failed to contact chronyd in 1.000s"
}
RELEASE build failure is due to some network error for python.
17:47:41 Downloading
https://files.pythonhosted.org/packages/05/d4/72eaba30abdcee0bb99cbdb21dbdb3f5d23a5041574fa7d94003b9afd3bc/numpy-1.15.4-cp34-cp34m-manylinux1_x86_64.whl
(13.8MB)
17:47:41 Exception:
17:47:41 Traceback (most recent call last):
17:47:41 File
"/home/jenkins-slave/workspace/kudu-master/1/build/release/py_env/lib/python3.4/site-packages/pip/_vendor/urllib3/response.py",
line 331, in _error_catcher
17:47:41 yield
17:47:41 File
"/home/jenkins-slave/workspace/kudu-master/1/build/release/py_env/lib/python3.4/site-packages/pip/_vendor/urllib3/response.py",
line 413, in read
17:47:41 data = self._fp.read(amt)
17:47:41 File "/usr/lib/python3.4/http/client.py", line 529, in read
17:47:41 return super(HTTPResponse, self).read(amt)
17:47:41 File "/usr/lib/python3.4/http/client.py", line 568, in readinto
17:47:41 n = self.fp.readinto(b)
17:47:41 File "/usr/lib/python3.4/socket.py", line 374, in readinto
17:47:41 return self._sock.recv_into(b)
17:47:41 File "/usr/lib/python3.4/ssl.py", line 769, in recv_into
17:47:41 return self.read(nbytes, buffer)
17:47:41 File "/usr/lib/python3.4/ssl.py", line 641, in read
17:47:41 v = self._sslobj.read(len, buffer)
17:47:41 ConnectionResetError: [Errno 104] Connection reset by peer
--
To view, visit http://gerrit.cloudera.org:8080/15043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284
Gerrit-Change-Number: 15043
Gerrit-PatchSet: 11
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Thu, 09 Apr 2020 01:22:04 +
Gerrit-HasComments: No
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has uploaded a new patch set (#11) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. encoding-test: Clean up bitshuffle tests a little Also adds functions in random number generator to generate 128-bit integers. Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 --- M src/kudu/cfile/encoding-test.cc M src/kudu/util/random.h M src/kudu/util/random_util-test.cc M src/kudu/util/random_util.h 4 files changed, 159 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/15043/11 -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 11 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. Patch Set 10: (3 comments) http://gerrit.cloudera.org:8080/#/c/15043/10/src/kudu/util/random_util-test.cc File src/kudu/util/random_util-test.cc: http://gerrit.cloudera.org:8080/#/c/15043/10/src/kudu/util/random_util-test.cc@123 PS10, Line 123: static constexpr IntType min_val = std::numeric_limits::min() / 2; : static constexpr IntType max_val = std::numeric_limits::max() / 2; > Could you also exercise the min/max cases? Just to exercise what you mentio Done http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h File src/kudu/util/random_util.h: http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@128 PS9, Line 128: > IMO it's best practice -- sentences without Oxford commas read ambiguously. Done http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@146 PS9, Line 146: exception > Ah I see. Could you move this comment up to the be a function header so the I think comment is better placed close to the line that raises ASAN/UBSAN warning instead of the top of the function. -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 10 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 09 Apr 2020 00:16:02 + Gerrit-HasComments: Yes
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h File src/kudu/util/random.h: http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h@247 PS9, Line 247: return sizeof(IntType) == 4 ? rand->Next32() : : sizeof(IntType) == 8 ? rand->Next64() : rand->Next128(); > This is a constexpr function so can't use if/switch in C++11. See comment o Wow, C++14+ can't come soon enough. http://gerrit.cloudera.org:8080/#/c/15043/10/src/kudu/util/random_util-test.cc File src/kudu/util/random_util-test.cc: http://gerrit.cloudera.org:8080/#/c/15043/10/src/kudu/util/random_util-test.cc@123 PS10, Line 123: static constexpr IntType min_val = std::numeric_limits::min() / 2; : static constexpr IntType max_val = std::numeric_limits::max() / 2; Could you also exercise the min/max cases? Just to exercise what you mentioned in the random_util comment. http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h File src/kudu/util/random_util.h: http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@128 PS9, Line 128: > Oxford comma is optional unless our coding guidelines explicitly require it IMO it's best practice -- sentences without Oxford commas read ambiguously. I suspect you'll find more sentences with it than without it in this codebase. But if you feel strongly against it, I'm fine leaving it as is. http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@146 PS9, Line 146: exception > This is the comment for ASAN exception added for this function. See L130. Ah I see. Could you move this comment up to the be a function header so the explanation is closer to the more interesting code? -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 10 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 08 Apr 2020 23:13:42 + Gerrit-HasComments: Yes
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has uploaded a new patch set (#10) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. encoding-test: Clean up bitshuffle tests a little Also adds functions in random number generator to generate 128-bit integers. Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 --- M src/kudu/cfile/encoding-test.cc M src/kudu/util/random.h M src/kudu/util/random_util-test.cc M src/kudu/util/random_util.h 4 files changed, 151 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/15043/10 -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 10 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h File src/kudu/util/random.h: http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h@91 PS9, Line 91: upto > nit: missing space Done http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h@236 PS9, Line 236: Next128 > nit: for consistency, add () Done http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h@247 PS9, Line 247: return sizeof(IntType) == 4 ? rand->Next32() : : sizeof(IntType) == 8 ? rand->Next64() : rand->Next128(); > nit: may be simpler with a switch. Same below This is a constexpr function so can't use if/switch in C++11. See comment on line 242. Moved the comment right above this line for better readability. http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h@252 PS9, Line 252: integer > nit: maybe specify that this can be signed or unsigned Done http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h File src/kudu/util/random_util.h: http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@128 PS9, Line 128: > nit: add a comma; same elsewhere. Oxford comma is optional unless our coding guidelines explicitly require it. So I'm going to drop this comment. http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@131 PS9, Line 131: std::vector CreateRandomIntegersInRange(const int num_values, > Could you also add a small test for this in util/random_util-test.cc? Done http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@146 PS9, Line 146: exception > What exception is this? This is the comment for ASAN exception added for this function. See L130. -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 9 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 08 Apr 2020 22:11:37 + Gerrit-HasComments: Yes
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h File src/kudu/util/random.h: http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h@91 PS9, Line 91: upto nit: missing space http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h@236 PS9, Line 236: Next128 nit: for consistency, add () http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h@247 PS9, Line 247: return sizeof(IntType) == 4 ? rand->Next32() : : sizeof(IntType) == 8 ? rand->Next64() : rand->Next128(); nit: may be simpler with a switch. Same below http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random.h@252 PS9, Line 252: integer nit: maybe specify that this can be signed or unsigned http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h File src/kudu/util/random_util.h: http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@128 PS9, Line 128: nit: add a comma; same elsewhere. http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@131 PS9, Line 131: std::vector CreateRandomIntegersInRange(const int num_values, Could you also add a small test for this in util/random_util-test.cc? http://gerrit.cloudera.org:8080/#/c/15043/9/src/kudu/util/random_util.h@146 PS9, Line 146: exception What exception is this? -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 9 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 07 Apr 2020 19:37:43 + Gerrit-HasComments: Yes
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has uploaded a new patch set (#7) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. encoding-test: Clean up bitshuffle tests a little Also adds functions in random number generator to generate 128-bit integers. Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 --- M src/kudu/cfile/encoding-test.cc M src/kudu/util/random.h M src/kudu/util/random_util.h 3 files changed, 109 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/15043/7 -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/15043/5/src/kudu/util/random.h File src/kudu/util/random.h: http://gerrit.cloudera.org:8080/#/c/15043/5/src/kudu/util/random.h@251 PS5, Line 251: IntType > UnsignedIntType This change won't be needed, reverting to PS3. Subtraction issue reported by ASAN is not an actual problem, so making an exception instead. http://gerrit.cloudera.org:8080/#/c/15043/5/src/kudu/util/random_util.h File src/kudu/util/random_util.h: http://gerrit.cloudera.org:8080/#/c/15043/5/src/kudu/util/random_util.h@130 PS5, Line 130: // in the [min_val, min_val + diff) range. > Same comment about signedness. Same as above. -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 01 Apr 2020 22:53:25 + Gerrit-HasComments: Yes
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has uploaded a new patch set (#6) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. encoding-test: Clean up bitshuffle tests a little Also adds functions in random number generator to generate 128-bit integers. Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 --- M src/kudu/cfile/encoding-test.cc M src/kudu/util/random.h M src/kudu/util/random_util.cc M src/kudu/util/random_util.h 4 files changed, 137 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/15043/6 -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/15043/5/src/kudu/util/random.h File src/kudu/util/random.h: http://gerrit.cloudera.org:8080/#/c/15043/5/src/kudu/util/random.h@251 PS5, Line 251: IntType UnsignedIntType Though I don't understand why these two functions differ on signedness: both UniformN() and NextN() return unsigned integers. http://gerrit.cloudera.org:8080/#/c/15043/5/src/kudu/util/random_util.h File src/kudu/util/random_util.h: http://gerrit.cloudera.org:8080/#/c/15043/5/src/kudu/util/random_util.h@130 PS5, Line 130: template Same comment about signedness. -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 01 Apr 2020 18:50:49 + Gerrit-HasComments: Yes
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has uploaded a new patch set (#5) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. encoding-test: Clean up bitshuffle tests a little Also adds functions in random number generator to generate 128-bit integers. Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 --- M src/kudu/cfile/encoding-test.cc M src/kudu/util/random.h M src/kudu/util/random_util.cc M src/kudu/util/random_util.h 4 files changed, 134 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/15043/5 -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has uploaded a new patch set (#4) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. encoding-test: Clean up bitshuffle tests a little Also adds functions in random number generator to generate 128-bit integers. Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 --- M src/kudu/cfile/encoding-test.cc M src/kudu/util/random.h M src/kudu/util/random_util.cc M src/kudu/util/random_util.h 4 files changed, 135 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/15043/4 -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. Patch Set 3: > Patch Set 3: Verified-1 > > Build Failed > > http://jenkins.kudu.apache.org/job/kudu-gerrit/21229/ : FAILURE ASAN failure seem legit, however unable to reproduce on ve0518. TestEncoding.TestBShufInt32BlockEncoder Working on the fix... -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 01 Apr 2020 13:42:00 + Gerrit-HasComments: No
[kudu-CR] encoding-test: Clean up bitshuffle tests a little
Bankim Bhavsar has uploaded a new patch set (#3) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: Clean up bitshuffle tests a little .. encoding-test: Clean up bitshuffle tests a little Also adds functions in random number generator to generate 128-bit integers. Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 --- M src/kudu/cfile/encoding-test.cc M src/kudu/util/random.h M src/kudu/util/random_util.h 3 files changed, 102 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/15043/3 -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] encoding-test: clean up bitshuffle tests a little
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15043 ) Change subject: encoding-test: clean up bitshuffle tests a little .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/15043/2/src/kudu/cfile/encoding-test.cc File src/kudu/cfile/encoding-test.cc: http://gerrit.cloudera.org:8080/#/c/15043/2/src/kudu/cfile/encoding-test.cc@722 PS2, Line 722: v = min_val + rng.Uniform64(max_val - min_val); The ASAN failure seem legit: /home/jenkins-slave/workspace/kudu-master/1/src/kudu/cfile/encoding-test.cc:722:41: runtime error: signed integer overflow: 2147483647 - -2147483648 cannot be represented in type 'int' -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Jan 2020 18:05:13 + Gerrit-HasComments: Yes
[kudu-CR] encoding-test: clean up bitshuffle tests a little
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15043 to look at the new patch set (#2). Change subject: encoding-test: clean up bitshuffle tests a little .. encoding-test: clean up bitshuffle tests a little Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 --- M src/kudu/cfile/encoding-test.cc 1 file changed, 39 insertions(+), 25 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/15043/2 -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] encoding-test: clean up bitshuffle tests a little
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/15043 )
Change subject: encoding-test: clean up bitshuffle tests a little
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15043/1/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
http://gerrit.cloudera.org:8080/#/c/15043/1/src/kudu/cfile/encoding-test.cc@719
PS1, Line 719: 1
> Do we explicitly want it to have the same seed every run?
Done
http://gerrit.cloudera.org:8080/#/c/15043/1/src/kudu/cfile/encoding-test.cc@721
PS1, Line 721: for (int i = 0; i < count; i++) {
> nit: maybe, use the same 'for (auto& e : ints) ...' construct here as well?
Done
http://gerrit.cloudera.org:8080/#/c/15043/1/src/kudu/cfile/encoding-test.cc@742
PS1, Line 742: int32_t
> int64_t ?
woops, good catch
--
To view, visit http://gerrit.cloudera.org:8080/15043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284
Gerrit-Change-Number: 15043
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Thu, 16 Jan 2020 06:24:07 +
Gerrit-HasComments: Yes
[kudu-CR] encoding-test: clean up bitshuffle tests a little
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/15043 )
Change subject: encoding-test: clean up bitshuffle tests a little
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15043/1/src/kudu/cfile/encoding-test.cc
File src/kudu/cfile/encoding-test.cc:
http://gerrit.cloudera.org:8080/#/c/15043/1/src/kudu/cfile/encoding-test.cc@719
PS1, Line 719: 1
Do we explicitly want it to have the same seed every run?
http://gerrit.cloudera.org:8080/#/c/15043/1/src/kudu/cfile/encoding-test.cc@721
PS1, Line 721: for (int i = 0; i < count; i++) {
nit: maybe, use the same 'for (auto& e : ints) ...' construct here as well?
http://gerrit.cloudera.org:8080/#/c/15043/1/src/kudu/cfile/encoding-test.cc@742
PS1, Line 742: int32_t
int64_t ?
Or this is not a typo and it's necessary to have int32 limits for this test?
--
To view, visit http://gerrit.cloudera.org:8080/15043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284
Gerrit-Change-Number: 15043
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 16 Jan 2020 02:46:37 +
Gerrit-HasComments: Yes
[kudu-CR] encoding-test: clean up bitshuffle tests a little
Hello Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/15043 to review the following change. Change subject: encoding-test: clean up bitshuffle tests a little .. encoding-test: clean up bitshuffle tests a little Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 --- M src/kudu/cfile/encoding-test.cc 1 file changed, 39 insertions(+), 25 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/15043/1 -- To view, visit http://gerrit.cloudera.org:8080/15043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I8b7bee1f952c46c09df5102ac1916141c6935284 Gerrit-Change-Number: 15043 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin
