kirklund commented on a change in pull request #6909:
URL: https://github.com/apache/geode/pull/6909#discussion_r737920330
##########
File path:
geode-wan/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/AlterRegionCommandWithRemoteLocator.java
##########
@@ -208,7 +208,36 @@ public void
whenAlteringMultipleRegionWithAlterCommandToAddGatewaySendersThenItS
return true;
});
+ }
+
+
+ /**
+ * Test execution of alter region command, when adding gw sender to already
initialized region is
+ * not taking too much time.
+ * Verify that execution of command will not be halted, due to
PRShadowRegion not available
+ * immediately in all servers.
+ */
+ @Test
+ public void
alterInitializedRegionWithGwSenderOnManyServersDoesNotTakeTooLong() {
+ gfsh.executeAndAssertThat("create disk-store --name=data
--max-oplog-size=10 --dir=.")
+ .statusIsSuccess();
+
+ gfsh.executeAndAssertThat(
+ "create region --name=Positions --type=PARTITION_REDUNDANT_PERSISTENT
--disk-store=data --total-num-buckets=13")
+ .statusIsSuccess();
+ gfsh.executeAndAssertThat(
+ "query --query=\"select key,value from " + SEPARATOR +
"Positions.entries\"")
+ .statusIsSuccess();
+
+ gfsh.executeAndAssertThat(
+ "create gateway-sender --id=parallelPositions
--remote-distributed-system-id=1 --enable-persistence=true
--disk-store-name=data --parallel=true")
+ .statusIsSuccess();
+
+ GeodeAwaitility.await().atMost(15, TimeUnit.SECONDS).until(() -> {
Review comment:
Please use the default timeout in `GeodeAwaitility` by removing the
`atMost`. This low of a timeout can intermittently fail on a machine that gets
an unexpected CPU pause.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/CreateMissingBucketsTask.java
##########
@@ -43,19 +71,70 @@ public void run2() {
protected void createMissingBuckets(PartitionedRegion region) {
PartitionedRegion parentRegion =
ColocationHelper.getColocatedRegion(region);
- if (parentRegion == null) {
+ if (parentRegion == null)
return;
- }
+
// Make sure the parent region has created missing buckets
// before we create missing buckets for this child region.
createMissingBuckets(parentRegion);
for (int i = 0; i < region.getTotalNumberOfBuckets(); i++) {
+ if (region.isClosed || region.isLocallyDestroyed)
+ return;
if
(parentRegion.getRegionAdvisor().getBucketAdvisor(i).getBucketRedundancy() !=
region
.getRegionAdvisor().getBucketAdvisor(i).getBucketRedundancy()) {
region.getRedundancyProvider().createBucketAtomically(i, 0, true,
null);
}
}
}
+
+ /**
+ * Wait for Colocation to complete. Wait all nodes to Register this
PartitionedRegion.
+ */
+ protected boolean waitForColocationCompleted(PartitionedRegion
partitionedRegion) {
+ int sleepInterval =
PartitionedRegionHelper.DEFAULT_WAIT_PER_RETRY_ITERATION;
+
+ while (!ColocationHelper.isColocationComplete(partitionedRegion)
+ && (retryCount < MAX_NUMBER_INTERVALS)) {
+
+ // Didn't time out. Sleep a bit and then continue
+ boolean interrupted = Thread.interrupted();
+ try {
+ logger.info("Waiting for collocation to complete, retry number {}",
retryCount);
+ Thread.sleep(sleepInterval);
+ } catch (InterruptedException ignore) {
+ interrupted = true;
+ } finally {
+ if (interrupted) {
+ Thread.currentThread().interrupt();
+ }
+ }
+
+ if (partitionedRegion.isLocallyDestroyed || partitionedRegion.isClosed)
+ return false;
+
+ retryCount++;
+ if (retryCount == SMALL_200MS_INTERVALS) {
+ sleepInterval = 2 *
PartitionedRegionHelper.DEFAULT_WAIT_PER_RETRY_ITERATION;
+ } else if (retryCount == SMALL_500MS_INTERVALS) {
+ sleepInterval = 5 *
PartitionedRegionHelper.DEFAULT_WAIT_PER_RETRY_ITERATION;
+ } else if (retryCount == MEDIUM_1SEC_INTERVALS) {
+ sleepInterval = 10 *
PartitionedRegionHelper.DEFAULT_WAIT_PER_RETRY_ITERATION;
+ } else if (retryCount == MEDIUM_2SEC_INTERVALS) {
+ sleepInterval = 20 *
PartitionedRegionHelper.DEFAULT_WAIT_PER_RETRY_ITERATION;
+ } else if (retryCount == LARGE_5SEC_INTERVALS) {
+ sleepInterval = 50 *
PartitionedRegionHelper.DEFAULT_WAIT_PER_RETRY_ITERATION;
+ }
+
+ }
+ return ColocationHelper.isColocationComplete(partitionedRegion);
+
+ }
+
+ @VisibleForTesting
+ public int getRetryCount() {
+ return retryCount;
+ }
Review comment:
The only callers are in the same package, so you should make this
package-private to minimize the scope:
```
@VisibleForTesting
int getRetryCount() {
return retryCount;
}
```
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/CreateMissingBucketsTask.java
##########
@@ -43,19 +71,70 @@ public void run2() {
protected void createMissingBuckets(PartitionedRegion region) {
PartitionedRegion parentRegion =
ColocationHelper.getColocatedRegion(region);
- if (parentRegion == null) {
+ if (parentRegion == null)
return;
- }
+
Review comment:
We typically keep or even add the missing `{` and `}` brackets instead
of deleting them..
--
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]
