You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2017/08/04 23:41:54 UTC
[Impala-ASF-CR] IMPALA-5031: unsafe random number generation in buffer-pool-test
Tim Armstrong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7596
Change subject: IMPALA-5031: unsafe random number generation in buffer-pool-test
......................................................................
IMPALA-5031: unsafe random number generation in buffer-pool-test
The bug is that ConcurrentRegistration shares one random number
generator between all the threads. This isn't safe and UBSAN was
unhappy with it.
The fix is to create one RNG per thread. We already do that in a
different test so the code is factored out into a utility function.
Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
---
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/testutil/rand-util.h
2 files changed, 21 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/96/7596/1
--
To view, visit http://gerrit.cloudera.org:8080/7596
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: unsafe random number generation in buffer-pool-test
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5031: unsafe random number generation in buffer-pool-test
......................................................................
Patch Set 1:
Added a testing section to clarify Jim's question.
--
To view, visit http://gerrit.cloudera.org:8080/7596
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: unsafe random number generation in buffer-pool-test
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5031: unsafe random number generation in buffer-pool-test
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7596/1/be/src/testutil/rand-util.h
File be/src/testutil/rand-util.h:
PS1, Line 34: static void SeedRng(const char* env_var, std::mt19937* rng) {
: const char* seed_str = getenv(env_var);
: int64_t seed;
: if (seed_str != nullptr) {
: seed = atoi(seed_str);
: } else {
: seed = time(nullptr);
: }
: LOG(INFO) << "Random seed (overridable with " << env_var << "): " << seed;
: rng->seed(seed);
: }
> Why not change this to use std::random_device instead?
Done
--
To view, visit http://gerrit.cloudera.org:8080/7596
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: unsafe random number generation in buffer-pool-test
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5031: unsafe random number generation in buffer-pool-test
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7596
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: unsafe random number generation in buffer-pool-test
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5031: unsafe random number generation in buffer-pool-test
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7596/1/be/src/testutil/rand-util.h
File be/src/testutil/rand-util.h:
PS1, Line 34: static void SeedRng(const char* env_var, std::mt19937* rng) {
: const char* seed_str = getenv(env_var);
: int64_t seed;
: if (seed_str != nullptr) {
: seed = atoi(seed_str);
: } else {
: seed = time(nullptr);
: }
: LOG(INFO) << "Random seed (overridable with " << env_var << "): " << seed;
: rng->seed(seed);
: }
Why not change this to use std::random_device instead?
See https://github.com/apache/incubator-impala/blob/master/be/src/rpc/authentication.cc#L493-L494
--
To view, visit http://gerrit.cloudera.org:8080/7596
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: unsafe random number generation in buffer-pool-test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5031: unsafe random number generation in buffer-pool-test
......................................................................
IMPALA-5031: unsafe random number generation in buffer-pool-test
The bug is that ConcurrentRegistration shares one random number
generator between all the threads. This isn't safe and UBSAN was
unhappy with it.
The fix is to create one RNG per thread. We already do that in a
different test so the code is factored out into a utility function.
Testing:
Ran buffer-pool-test under UBSAN
Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Reviewed-on: http://gerrit.cloudera.org:8080/7596
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/testutil/rand-util.h
2 files changed, 22 insertions(+), 11 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Sailesh Mukil: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7596
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: unsafe random number generation in buffer-pool-test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5031: unsafe random number generation in buffer-pool-test
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/992/
--
To view, visit http://gerrit.cloudera.org:8080/7596
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: unsafe random number generation in buffer-pool-test
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5031: unsafe random number generation in buffer-pool-test
......................................................................
Patch Set 1: Code-Review+2
You tested with UBSan?
--
To view, visit http://gerrit.cloudera.org:8080/7596
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: unsafe random number generation in buffer-pool-test
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Jim Apple,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7596
to look at the new patch set (#3).
Change subject: IMPALA-5031: unsafe random number generation in buffer-pool-test
......................................................................
IMPALA-5031: unsafe random number generation in buffer-pool-test
The bug is that ConcurrentRegistration shares one random number
generator between all the threads. This isn't safe and UBSAN was
unhappy with it.
The fix is to create one RNG per thread. We already do that in a
different test so the code is factored out into a utility function.
Testing:
Ran buffer-pool-test under UBSAN
Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
---
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/testutil/rand-util.h
2 files changed, 22 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/96/7596/3
--
To view, visit http://gerrit.cloudera.org:8080/7596
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: unsafe random number generation in buffer-pool-test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5031: unsafe random number generation in buffer-pool-test
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7596
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: unsafe random number generation in buffer-pool-test
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Jim Apple,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7596
to look at the new patch set (#2).
Change subject: IMPALA-5031: unsafe random number generation in buffer-pool-test
......................................................................
IMPALA-5031: unsafe random number generation in buffer-pool-test
The bug is that ConcurrentRegistration shares one random number
generator between all the threads. This isn't safe and UBSAN was
unhappy with it.
The fix is to create one RNG per thread. We already do that in a
different test so the code is factored out into a utility function.
Testing:
Ran buffer-pool-test under UBSAN
Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
---
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/testutil/rand-util.h
2 files changed, 21 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/96/7596/2
--
To view, visit http://gerrit.cloudera.org:8080/7596
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>