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 2016/09/06 20:29:03 UTC
[kudu-CR] KUDU-1590. Fix cache-test failure on some machines
Hello Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4321
to review the following change.
Change subject: KUDU-1590. Fix cache-test failure on some machines
......................................................................
KUDU-1590. Fix cache-test failure on some machines
CacheTest.TestEviction was failing on some machines, depending on the
number of cores. This is due to bfb6f2338cd57a1c7f8405c97b66c3050c8920ff
which changed the cache to determine the number of cache shards based on
the core count.
The number of shards changed the eviction behavior because the sharded
cache is actually only *approximately* LRU. Each shard itself is LRU,
but entries in different shards will not evict each other. So, with more
shards, the test wasn't evicting the element as expected.
The fix simply increases the amount of cache churn caused by the test so
that the eviction proceeds as expected.
Change-Id: I77362fc67ab5ba8420c21d1e3fb5c28ff9bfab1b
---
M src/kudu/util/cache-test.cc
1 file changed, 5 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/4321/1
--
To view, visit http://gerrit.cloudera.org:8080/4321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I77362fc67ab5ba8420c21d1e3fb5c28ff9bfab1b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] KUDU-1590. Fix cache-test failure on some machines
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1590. Fix cache-test failure on some machines
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/3238/
--
To view, visit http://gerrit.cloudera.org:8080/4321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I77362fc67ab5ba8420c21d1e3fb5c28ff9bfab1b
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: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1590. Fix cache-test failure on some machines
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1590. Fix cache-test failure on some machines
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4321/1/src/kudu/util/cache-test.cc
File src/kudu/util/cache-test.cc:
Line 190: for (int i = 0; i < kNumElems + 1000; i++) {
Just for my own curiosity (don't need to change anything), what exactly is the relationship between the number of insertions needed here and the number of shards such that 200 is guaranteed to be evicted? It's not intuitive to me, partly because when I ran the test on a 16 core box it passed, but on an 8 core box (my laptop) it failed.
--
To view, visit http://gerrit.cloudera.org:8080/4321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I77362fc67ab5ba8420c21d1e3fb5c28ff9bfab1b
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: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1590. Fix cache-test failure on some machines
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1590. Fix cache-test failure on some machines
......................................................................
KUDU-1590. Fix cache-test failure on some machines
CacheTest.TestEviction was failing on some machines, depending on the
number of cores. This is due to bfb6f2338cd57a1c7f8405c97b66c3050c8920ff
which changed the cache to determine the number of cache shards based on
the core count.
The number of shards changed the eviction behavior because the sharded
cache is actually only *approximately* LRU. Each shard itself is LRU,
but entries in different shards will not evict each other. So, with more
shards, the test wasn't evicting the element as expected.
The fix simply increases the amount of cache churn caused by the test so
that the eviction proceeds as expected.
Change-Id: I77362fc67ab5ba8420c21d1e3fb5c28ff9bfab1b
Reviewed-on: http://gerrit.cloudera.org:8080/4321
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/util/cache-test.cc
1 file changed, 5 insertions(+), 2 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I77362fc67ab5ba8420c21d1e3fb5c28ff9bfab1b
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1590. Fix cache-test failure on some machines
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1590. Fix cache-test failure on some machines
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4321/1/src/kudu/util/cache-test.cc
File src/kudu/util/cache-test.cc:
Line 190: for (int i = 0; i < kNumElems + 1000; i++) {
> Just for my own curiosity (don't need to change anything), what exactly is
Didn't really dig into this... my guess is that it was right "on the edge" here, and the number of shards just shuffled the elements we're touching here around differently such that sometimes '101' didn't get evicted, and sometimes it did.
--
To view, visit http://gerrit.cloudera.org:8080/4321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I77362fc67ab5ba8420c21d1e3fb5c28ff9bfab1b
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes