pivotal-jbarrett commented on code in PR #7493:
URL: https://github.com/apache/geode/pull/7493#discussion_r857827040
##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/execute/PRClientServerRegionFunctionExecutionNoSingleHopDUnitTest.java:
##########
@@ -406,6 +412,76 @@ public void testBug40714() {
PRClientServerRegionFunctionExecutionDUnitTest::FunctionExecution_Inline_Bug40714);
}
+ /**
+ * This test case verifies that if the execution of a function handled
+ * by a Function Execution thread times out at the client, the
ServerConnection
+ * thread will eventually be released.
+ * In order to test this, a slow function will be executed by a client
+ * with a small time-out a number of times equal to the number of servers
+ * in the cluster * the max number of threads configured.
+ * After the function executions have timed-out, another request will be
+ * sent by the client to any server and it should be served timely.
+ * If the ServerConnection threads had not been released, this new
+ * request will never be served because there would be not ServerConnection
+ * threads available and the test case will time-out.
+ */
+ @Test
Review Comment:
Please rename test to current convention,
`PRClientServerRegionFunctionExecutionNoSingleHopDistributedTest`.
##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/execute/PRClientServerTestBase.java:
##########
@@ -368,6 +377,33 @@ private static void createNoSingleHopCacheClient(String
host, Integer port1, Int
assertNotNull(region);
}
+ private static void createNoSingleHopCacheClient(String host,
+ Integer port1, Integer port2, Integer port3, int connectTimeout) {
+ CacheServerTestUtil.disableShufflingOfEndpoints();
+
+ Pool p;
+ try {
+ PoolFactory factory = PoolManager.createFactory().addServer(host, port1)
+ .addServer(host, port2).addServer(host, port3).setPingInterval(2000)
+ .setSubscriptionEnabled(true).setReadTimeout(2000)
+ .setSocketBufferSize(1000).setRetryAttempts(0)
+ .setSocketConnectTimeout(connectTimeout)
+ .setPRSingleHopEnabled(false);
+
+ p = factory.create("PRClientServerTestBase");
+ } finally {
+ CacheServerTestUtil.enableShufflingOfEndpoints();
+ }
+ pool = (PoolImpl) p;
+ AttributesFactory factory = new AttributesFactory();
+ factory.setScope(Scope.LOCAL);
+ factory.setDataPolicy(DataPolicy.EMPTY);
+ factory.setPoolName(p.getName());
+ RegionAttributes attrs = factory.create();
Review Comment:
Please don't introduce more raw type usage. Preferably clean up all other
raw types in existing test.
##########
geode-core/src/main/java/org/apache/geode/internal/cache/execute/PartitionedRegionFunctionResultSender.java:
##########
@@ -81,31 +86,48 @@ public KnownVersion getClientVersion() {
return null;
}
+ public PartitionedRegionFunctionResultSender(DistributionManager dm,
PartitionedRegion pr,
+ long time, PartitionedRegionFunctionStreamingMessage msg,
+ Function function, int[] bucketArray) {
+ this(dm, pr, time, msg, function, bucketArray,
+ (x, y) -> FunctionStatsManager.getFunctionStats((String) x,
(InternalDistributedSystem) y));
+ }
+
/**
* Have to combine next two constructor in one and make a new class which
will send Results back.
*
*/
public PartitionedRegionFunctionResultSender(DistributionManager dm,
PartitionedRegion pr,
long time, PartitionedRegionFunctionStreamingMessage msg, Function
function,
- int[] bucketArray) {
+ int[] bucketArray, BiFunction functionStatsFunctionProvider) {
this.msg = msg;
this.dm = dm;
this.pr = pr;
this.time = time;
this.function = function;
this.bucketArray = bucketArray;
-
+ this.functionStatsFunctionProvider = functionStatsFunctionProvider;
Review Comment:
Have all overloaded constructs call a single initializing constructor.
##########
geode-core/src/main/java/org/apache/geode/internal/cache/execute/PartitionedRegionFunctionResultSender.java:
##########
@@ -81,31 +86,48 @@ public KnownVersion getClientVersion() {
return null;
}
+ public PartitionedRegionFunctionResultSender(DistributionManager dm,
PartitionedRegion pr,
+ long time, PartitionedRegionFunctionStreamingMessage msg,
+ Function function, int[] bucketArray) {
+ this(dm, pr, time, msg, function, bucketArray,
+ (x, y) -> FunctionStatsManager.getFunctionStats((String) x,
(InternalDistributedSystem) y));
+ }
+
/**
* Have to combine next two constructor in one and make a new class which
will send Results back.
*
*/
public PartitionedRegionFunctionResultSender(DistributionManager dm,
PartitionedRegion pr,
Review Comment:
If this new constructor is used only testing it should be package private.
Evaluate access level on each of these constructors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
