GEODE-4147: Add variability to client rebalance logic#1237
GEODE-4147: Add variability to client rebalance logic#1237PivotalSarge wants to merge 1 commit intoapache:developfrom
Conversation
bschuchardt
left a comment
bschuchardt
left a comment
There was a problem hiding this comment.
LGTM
I'd like to learn how to use that style of assertions
| static int addVarianceToInterval(int interval) { | ||
| if (1 <= interval) { | ||
| final SplittableRandom random = new SplittableRandom(); | ||
| final int variance = (interval < 10) ? 1 : random.nextInt(interval / 10); |
There was a problem hiding this comment.
For a value less than 10 this will give (value - 1) 50% of the time and (value + 1) 50%...it seems it should also have an equal chance to return the unmodified value.
There was a problem hiding this comment.
Because the intent behind this change is to avoid having a lot of activity happen at the same time, I consciously excluded the case where the interval was not changed.
| final SplittableRandom random = new SplittableRandom(); | ||
| final int variance = (interval < 10) ? 1 : random.nextInt(interval / 10); | ||
| final int sign = random.nextBoolean() ? 1 : -1; | ||
| return interval + (sign * variance); |
There was a problem hiding this comment.
Not sure if this matter, but this will also return the original value twice as often as any other value (i.e. if the interval is 10, this will return 9 1/4 of the time, 11 1/4 of the time, and 10 half the time).
There was a problem hiding this comment.
To quote Inigo Montoya, perhaps that method does not do what I think it does... I'll make that change.
| public void testAddVarianceToInterval() { | ||
| assertThat(PoolImpl.addVarianceToInterval(0)).as("Zero gets zero variance").isEqualTo(0); | ||
| assertThat(PoolImpl.addVarianceToInterval(300000)).as("Large value gets +/-10% variance") | ||
| .isNotEqualTo(300000).isGreaterThanOrEqualTo(270000).isLessThanOrEqualTo(330000); |
There was a problem hiding this comment.
This test will fail about 1 in 30000 times when it returns 300000. Admittedly rare, but could be annoying when it happens.
There was a problem hiding this comment.
[I'm looking at older PRs] Would it be correct to just remove .isNotEqualTo(300000)?
There was a problem hiding this comment.
The change for the previous comment should prevent that.
PivotalSarge
force-pushed
the
feature/GEODE-4147
branch
from
a4b7e1f to
010e3da
Compare
PivotalSarge
force-pushed
the
feature/GEODE-4147
branch
from
010e3da to
1f0db2a
Compare
upthewaterspout
left a comment
upthewaterspout
left a comment
There was a problem hiding this comment.
I'm seeing tests fail with this PR
ConnectionPoolFactoryJUnitTest and all of the CacheXml* tests.
|
Encountering problems with tests; recalling these changes until that can be sorted. |
5 participants
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
[n/a] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
@upthewaterspout @bschuchardt