Skip to content

GEODE-4147: Add variability to client rebalance logic #1237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
PivotalSarge wants to merge 1 commit into from
Closed

GEODE-4147: Add variability to client rebalance logic #1237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion geode-core/src/main/java/org/apache/geode/cache/client/internal/PoolImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.Map.Entry;
import java.util.Properties;
import java.util.Set;
import java.util.SplittableRandom;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ThreadFactory;
Expand Down Expand Up @@ -159,6 +160,26 @@ public static PoolImpl create(PoolManagerImpl pm, String name, Pool attributes,
return pool;
}

/**
* Adds an arbitrary variance to a positive temporal interval. Where possible, 10% of the interval
* is added or subtracted from the interval. Otherwise, 1 is added or subtracted from the
* interval. For all positive intervals, the returned value will <bold>not</bold> equal the
* supplied interval.
*
* @param interval Positive temporal interval.
* @return Adjusted interval including the variance for positive intervals; the unmodified
* interval for non-positive intervals.
*/
static int addVarianceToInterval(int interval) {
if (1 <= interval) {
final SplittableRandom random = new SplittableRandom();
final int variance = (interval < 10) ? 1 : random.nextInt(interval / 10);
Copy link
Copy Markdown

@WireBaron WireBaron Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@PivotalSarge PivotalSarge Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 int sign = random.nextBoolean() ? 1 : -1;
return interval + (sign * variance);
Copy link
Copy Markdown

@WireBaron WireBaron Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

@PivotalSarge PivotalSarge Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To quote Inigo Montoya, perhaps that method does not do what I think it does... I'll make that change.

}
return interval;
}

public boolean isUsedByGateway() {
return usedByGateway;
}
Expand Down Expand Up @@ -186,7 +207,7 @@ protected PoolImpl(PoolManagerImpl pm, String name, Pool attributes,
this.name = name;
this.socketConnectTimeout = attributes.getSocketConnectTimeout();
this.freeConnectionTimeout = attributes.getFreeConnectionTimeout();
this.loadConditioningInterval = attributes.getLoadConditioningInterval();
this.loadConditioningInterval = addVarianceToInterval(attributes.getLoadConditioningInterval());
this.socketBufferSize = attributes.getSocketBufferSize();
this.threadLocalConnections = attributes.getThreadLocalConnections();
this.readTimeout = attributes.getReadTimeout();
Expand Down
13 changes: 12 additions & 1 deletion ...ore/src/test/java/org/apache/geode/cache/client/internal/ConnectionPoolImplJUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@

import static org.apache.geode.distributed.ConfigurationProperties.LOCATORS;
import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
import static org.junit.Assert.*;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import java.net.InetSocketAddress;
import java.net.SocketTimeoutException;
Expand Down Expand Up @@ -267,4 +270,12 @@ public boolean useThreadLocalConnection() {
assertEquals(location1, pool.executeOnQueuesAndReturnPrimaryResult(testOp));
}

@Test
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);
Copy link
Copy Markdown

@WireBaron WireBaron Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test will fail about 1 in 30000 times when it returns 300000. Admittedly rare, but could be annoying when it happens.

Copy link
Copy Markdown
Contributor

@kirklund kirklund Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I'm looking at older PRs] Would it be correct to just remove .isNotEqualTo(300000)?

Copy link
Copy Markdown
Contributor Author

@PivotalSarge PivotalSarge Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change for the previous comment should prevent that.

assertThat(PoolImpl.addVarianceToInterval(9)).as("Small value gets +/-1 variance")
.isNotEqualTo(9).isGreaterThanOrEqualTo(8).isLessThanOrEqualTo(10);
}
}