[GitHub] [geode-kafka-connector] nabarunnag opened a new pull request #1: Feature/restructuring

[GitBox]

nabarunnag opened a new pull request #1: Feature/restructuring
URL: https://github.com/apache/geode-kafka-connector/pull/1
 
 
   Cosmetic modification and new test runner for writing tests 


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [geode-kafka-connector] nabarunnag merged pull request #1: Feature/restructuring

[GitBox]

nabarunnag merged pull request #1: Feature/restructuring
URL: https://github.com/apache/geode-kafka-connector/pull/1
 
 
   


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [geode-kafka-connector] DonalEvans opened a new pull request #2: Added JsonPdxConverter

[GitBox]

DonalEvans opened a new pull request #2: Added JsonPdxConverter
URL: https://github.com/apache/geode-kafka-connector/pull/2
 
 
   - Allows PdxInstance objects to be converted to JSON bytes when sourced
   from a Geode region into a Kafka topic
   - Allows JSON bytes to be converted to PdxInstance objects when sinked
   into a Geode region from a Kafka topic
   - Added unit and DUnit tests for JsonPdxConverter
   - Added functionality to the test framework to specify custom key
   converter, custom value converter and configuration properties for each
   - Added TestObject class to allow validation of
   serialization/deserialization
   
   Authored-by: Donal Evans 


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [geode-kafka-connector] DonalEvans merged pull request #2: Added JsonPdxConverter

[GitBox]

DonalEvans merged pull request #2: Added JsonPdxConverter
URL: https://github.com/apache/geode-kafka-connector/pull/2
 
 
   


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [geode] mkevo commented on a change in pull request #4967: GEODE-7989: Improve backup exceptions logging

[GitBox]

mkevo commented on a change in pull request #4967:
URL: https://github.com/apache/geode/pull/4967#discussion_r411162666



##
File path: 
geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupService.java
##
@@ -74,6 +74,9 @@ public BackupService(InternalCache cache) {
 try {
   result = taskFuture.get();
 } catch (InterruptedException | ExecutionException e) {
+  if (e instanceof ExecutionException) {

Review comment:
   I think that it will be good to log both exceptions.
   Please remove this check.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jvarenina commented on a change in pull request #4967: GEODE-7989: Improve backup exceptions logging

[GitBox]

jvarenina commented on a change in pull request #4967:
URL: https://github.com/apache/geode/pull/4967#discussion_r411170968



##
File path: 
geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupService.java
##
@@ -74,6 +74,9 @@ public BackupService(InternalCache cache) {
 try {
   result = taskFuture.get();
 } catch (InterruptedException | ExecutionException e) {
+  if (e instanceof ExecutionException) {

Review comment:
   Done. Thanks!





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jujoramos opened a new pull request #4974: Revert "GEODE-7565: Allow gateway receivers with same host and port

[GitBox]

jujoramos opened a new pull request #4974:
URL: https://github.com/apache/geode/pull/4974


   This reverts commit dd23ee8200cba67cea82e57e2e4ccedcdf9e8266.
   
   Intermittent errors were observed while executing some internal tests and 
commit dd23ee8200cba67cea82e57e2e4ccedcdf9e8266 was determined to be 
responsible. As of yet, no local reproduction of the issue is available, but 
work is ongoing to provide a test that can be used to debug the issue.
   The below stack trace is an example of the issues we're seeing:
   
   ```
   [error 2020/04/18 23:44:22.758 PDT  tid=0x165] 
Unexpected error in pool task 

   org.apache.geode.InternalGemFireError: Unexpected message type PING
at 
org.apache.geode.cache.client.internal.AbstractOp.processAck(AbstractOp.java:264)
at 
org.apache.geode.cache.client.internal.PingOp$PingOpImpl.processResponse(PingOp.java:82)
at 
org.apache.geode.cache.client.internal.AbstractOp.processResponse(AbstractOp.java:222)
at 
org.apache.geode.cache.client.internal.AbstractOp.attemptReadResponse(AbstractOp.java:207)
at 
org.apache.geode.cache.client.internal.AbstractOp.attempt(AbstractOp.java:382)
at 
org.apache.geode.cache.client.internal.ConnectionImpl.execute(ConnectionImpl.java:268)
at 
org.apache.geode.cache.client.internal.pooling.PooledConnection.execute(PooledConnection.java:352)
at 
org.apache.geode.cache.client.internal.OpExecutorImpl.executeWithPossibleReAuthentication(OpExecutorImpl.java:753)
at 
org.apache.geode.cache.client.internal.OpExecutorImpl.executeOnServer(OpExecutorImpl.java:332)
at 
org.apache.geode.cache.client.internal.OpExecutorImpl.executeOn(OpExecutorImpl.java:303)
at 
org.apache.geode.cache.client.internal.PoolImpl.executeOn(PoolImpl.java:839)
at org.apache.geode.cache.client.internal.PingOp.execute(PingOp.java:38)
at 
org.apache.geode.cache.client.internal.LiveServerPinger$PingTask.run2(LiveServerPinger.java:90)
at 
org.apache.geode.cache.client.internal.PoolImpl$PoolTask.run(PoolImpl.java:1329)
at 
java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
at 
org.apache.geode.internal.ScheduledThreadPoolExecutorWithKeepAlive$DelegatingScheduledFuture.run(ScheduledThreadPoolExecutorWithKeepAlive.java:276)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)
   ```
   
   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 build` run cleanly?
   
   - [ ] Have you written or updated unit tests to verify your changes?
   
   - [ ] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)?
   
   ### Note:
   Please ensure that once the PR is submitted, check Concourse 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.
   



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jujoramos commented on issue #4974: Revert "GEODE-7565: Allow gateway receivers with same host and port

[GitBox]

jujoramos commented on issue #4974:
URL: https://github.com/apache/geode/pull/4974#issuecomment-616565367


   @alb3rtobr 



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jujoramos edited a comment on issue #4974: Revert "GEODE-7565: Allow gateway receivers with same host and port

[GitBox]

jujoramos edited a comment on issue #4974:
URL: https://github.com/apache/geode/pull/4974#issuecomment-616565367


   @alb3rtobr: I'm just tagging you as you are the author of the original 
commit so you're aware of the revert request, I'll try to reproduce the issue 
using a `DistributedTest` and will share it with you afterwards.
   



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jujoramos edited a comment on issue #4974: Revert "GEODE-7565: Allow gateway receivers with same host and port

[GitBox]

jujoramos edited a comment on issue #4974:
URL: https://github.com/apache/geode/pull/4974#issuecomment-616565367


   @alb3rtobr: I'm just tagging you as you are the author of the original 
commit so you're aware of the revert request, I'll try to reproduce the issue 
using a `DistributedTest` and will share it with you afterwards (let's use 
[GEODE-8004](https://issues.apache.org/jira/browse/GEODE-8004) for 
communication purposes).
   



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jujoramos edited a comment on issue #4974: Revert "GEODE-7565: Allow gateway receivers with same host and port

[GitBox]

jujoramos edited a comment on issue #4974:
URL: https://github.com/apache/geode/pull/4974#issuecomment-616565367


   @alb3rtobr: I'm just tagging you as you are the author of the original 
commit so you're aware of the revert request, I'll try to reproduce the issue 
using a `DistributedTest` and will share it with you afterwards (let's use 
[GEODE-8004](https://issues.apache.org/jira/browse/) for communication 
purposes).
   



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] kirklund commented on issue #4969: GEODE-7995: Creates concurrencyTest facet

[GitBox]

kirklund commented on issue #4969:
URL: https://github.com/apache/geode/pull/4969#issuecomment-616645957


   > What do we mean by facet in this context?
   
   I think it means (1) src set, (2) gradle target to execute tests in the src 
set, (3) pipeline job in CI and PR.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] alb3rtobr opened a new pull request #4975: Logging documentation fixes

[GitBox]

alb3rtobr opened a new pull request #4975:
URL: https://github.com/apache/geode/pull/4975


   



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] bschuchardt commented on issue #4974: Revert "GEODE-7565: Allow gateway receivers with same host and port

[GitBox]

bschuchardt commented on issue #4974:
URL: https://github.com/apache/geode/pull/4974#issuecomment-616668765


   The problem might be in changes to Ping.java.  It may send an error response 
back to the client with MessageType.PING, which maybe the client isn't equipped 
to deal with.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] pivotal-jbarrett commented on issue #4969: GEODE-7995: Creates concurrencyTest facet

[GitBox]

pivotal-jbarrett commented on issue #4969:
URL: https://github.com/apache/geode/pull/4969#issuecomment-616692247


   Very specifically we use a plugin called facet that creates all these things 
for us.
   https://plugins.gradle.org/plugin/nebula.facet



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jhuynh1 commented on a change in pull request #4928: GEODE-7971: Gw sender deliver TX events atomically to Gw receivers

[GitBox]

jhuynh1 commented on a change in pull request #4928:
URL: https://github.com/apache/geode/pull/4928#discussion_r411587074



##
File path: 
geode-core/src/main/java/org/apache/geode/internal/cache/wan/serial/SerialGatewaySenderQueue.java
##
@@ -410,6 +441,53 @@ public Object peek() throws CacheException {
 // so no need to worry about off-heap refCount.
   }
 
+  private void peekEventsFromIncompleteTransactions(List batch,

Review comment:
   This looks similar to the other peekEventsFromIncompleteTransactions, is 
there a way to reuse the code?  Although the classes probably already do a lot 
of duplication ...





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] dschneider-pivotal commented on a change in pull request #4973: improvement/GEODE-8002 Refactor: Extract class to encapsulate concurrent execution

[GitBox]

dschneider-pivotal commented on a change in pull request #4973:
URL: https://github.com/apache/geode/pull/4973#discussion_r411592696



##
File path: 
geode-redis/src/integrationTest/java/org/apache/geode/redis/general/LoopingThreads.java
##
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.general;
+
+import java.util.Arrays;
+import java.util.concurrent.CountDownLatch;
+import java.util.function.Function;
+import java.util.stream.Stream;
+
+public class LoopingThreads {
+  private final int iterationCount;
+  private final Function[] functions;
+
+  @SuppressWarnings("unchecked")
+  public LoopingThreads(int iterationCount,
+  Function... functions) {
+this.iterationCount = iterationCount;
+this.functions = (Function[]) functions;
+  }
+
+  public void run() {
+CountDownLatch latch = new CountDownLatch(1);
+Stream loopingThreadStream = Arrays
+.stream(functions)
+.map((r) -> new LoopingThread(r, iterationCount, latch))
+.map((t) -> {
+  t.start();
+  return t;
+});
+
+latch.countDown();
+
+loopingThreadStream.forEach(loopingThread -> {
+  try {
+loopingThread.join();
+  } catch (InterruptedException e) {
+throw new RuntimeException(e);
+  }
+});
+
+  }
+
+  private class LoopingRunnable implements Runnable {
+private final Function runnable;
+private final int iterationCount;
+private CountDownLatch startLatch;
+
+public LoopingRunnable(Function runnable, int 
iterationCount,
+CountDownLatch startLatch) {
+  this.runnable = runnable;
+  this.iterationCount = iterationCount;
+  this.startLatch = startLatch;
+}
+
+@Override
+public void run() {
+  try {
+startLatch.await();
+  } catch (InterruptedException e) {
+throw new RuntimeException(e);
+  }
+  for (int i = 0; i < iterationCount; i++) {
+runnable.apply(i);
+Thread.yield();

Review comment:
   Should the call of Thread.yield be optional? I'm not sure you would 
always want this





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] karensmolermiller opened a new pull request #4976: GEODE-7997: Document needed location of parallel gateway sender disk …

[GitBox]

karensmolermiller opened a new pull request #4976:
URL: https://github.com/apache/geode/pull/4976


   …stores
   
   Reviewers: I put the same information in 3 distinct locations. It is my hope 
that Geode-users who need this info will encounter it in one of those places 
when they look at the docs.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] DonalEvans commented on a change in pull request #4970: GEODE-7676: Add PR clear with expiration tests

[GitBox]

DonalEvans commented on a change in pull request #4970:
URL: https://github.com/apache/geode/pull/4970#discussion_r411591697



##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithExpirationDUnitTest.java
##
@@ -0,0 +1,535 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.apache.geode.cache.ExpirationAction.DESTROY;
+import static org.apache.geode.cache.ExpirationAction.INVALIDATE;
+import static org.apache.geode.cache.RegionShortcut.PARTITION;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_OVERFLOW;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT_OVERFLOW;
+import static org.apache.geode.internal.util.ArrayUtils.asList;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static org.apache.geode.test.dunit.VM.getVM;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.IntStream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import junitparams.naming.TestCaseName;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.ForcedDisconnectException;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheWriter;
+import org.apache.geode.cache.CacheWriterException;
+import org.apache.geode.cache.ExpirationAction;
+import org.apache.geode.cache.ExpirationAttributes;
+import org.apache.geode.cache.PartitionAttributes;
+import org.apache.geode.cache.PartitionAttributesFactory;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.cache.partition.PartitionRegionHelper;
+import org.apache.geode.cache.util.CacheWriterAdapter;
+import org.apache.geode.distributed.DistributedSystemDisconnectedException;
+import org.apache.geode.distributed.internal.DMStats;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import 
org.apache.geode.distributed.internal.membership.api.MembershipManagerHelper;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.DistributedDiskDirRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+
+/**
+ * Tests to verify that {@link PartitionedRegion#clear()} cancels all 
remaining expiration tasks
+ * on the {@link PartitionedRegion} once the operation is executed.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class PartitionedRegionClearWithExpirationDUnitTest implements 
Serializable {
+  private static final Integer BUCKETS = 13;
+  private static final Integer EXPIRATION_TIME = 30;
+  private static final String REGION_NAME = "PartitionedRegion";
+  private static final String TEST_CASE_NAME =
+  "[{index}] {method}(Coordinator:{0}, RegionType:{1}, 
ExpirationAction:{2})";
+
+  @Rule
+  public DistributedRule distributedRule = new DistributedRule(3);
+
+  @Rule
+  public CacheRule cacheRule = CacheRule.builder().createCacheInAll().build();
+
+  @Rule
+  public DistributedDiskDirRule distributedDiskDirRule = new 
DistributedDiskDirRule();
+
+  private VM accessor, server1, server2;
+
+  private enum TestVM {
+ACCESSOR(0), SERVER1(1), SERVER2(2);
+
+final int vmNumber;
+
+TestVM(int vmNumber) {
+  this.vmNumber = vmNumber;
+}
+  }
+
+  @SuppressWarnings("unused")
+  sta

[GitHub] [geode] prettyClouds commented on a change in pull request #4973: improvement/GEODE-8002 Refactor: Extract class to encapsulate concurrent execution

[GitBox]

prettyClouds commented on a change in pull request #4973:
URL: https://github.com/apache/geode/pull/4973#discussion_r411691444



##
File path: 
geode-redis/src/integrationTest/java/org/apache/geode/redis/general/LoopingThreads.java
##
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.general;
+
+import java.util.Arrays;
+import java.util.concurrent.CountDownLatch;
+import java.util.function.Function;
+import java.util.stream.Stream;
+
+public class LoopingThreads {
+  private final int iterationCount;
+  private final Function[] functions;
+
+  @SuppressWarnings("unchecked")
+  public LoopingThreads(int iterationCount,
+  Function... functions) {
+this.iterationCount = iterationCount;
+this.functions = (Function[]) functions;
+  }
+
+  public void run() {
+CountDownLatch latch = new CountDownLatch(1);
+Stream loopingThreadStream = Arrays
+.stream(functions)
+.map((r) -> new LoopingThread(r, iterationCount, latch))
+.map((t) -> {
+  t.start();
+  return t;
+});
+
+latch.countDown();
+
+loopingThreadStream.forEach(loopingThread -> {
+  try {
+loopingThread.join();
+  } catch (InterruptedException e) {
+throw new RuntimeException(e);
+  }
+});
+
+  }
+
+  private class LoopingRunnable implements Runnable {
+private final Function runnable;
+private final int iterationCount;
+private CountDownLatch startLatch;
+
+public LoopingRunnable(Function runnable, int 
iterationCount,
+CountDownLatch startLatch) {
+  this.runnable = runnable;
+  this.iterationCount = iterationCount;
+  this.startLatch = startLatch;
+}
+
+@Override
+public void run() {
+  try {
+startLatch.await();
+  } catch (InterruptedException e) {
+throw new RuntimeException(e);
+  }
+  for (int i = 0; i < iterationCount; i++) {
+runnable.apply(i);
+Thread.yield();

Review comment:
   That makes sense. I want to leave it like this as the default, though, 
and if someone wants to change the behavior they can add that functionality.  
How do you feel about that?  Or do you think not having this as an option is a 
dealbreaker.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jchen21 commented on a change in pull request #4970: GEODE-7676: Add PR clear with expiration tests

[GitBox]

jchen21 commented on a change in pull request #4970:
URL: https://github.com/apache/geode/pull/4970#discussion_r411699051



##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithExpirationDUnitTest.java
##
@@ -0,0 +1,535 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.apache.geode.cache.ExpirationAction.DESTROY;
+import static org.apache.geode.cache.ExpirationAction.INVALIDATE;
+import static org.apache.geode.cache.RegionShortcut.PARTITION;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_OVERFLOW;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT_OVERFLOW;
+import static org.apache.geode.internal.util.ArrayUtils.asList;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static org.apache.geode.test.dunit.VM.getVM;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.IntStream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import junitparams.naming.TestCaseName;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.ForcedDisconnectException;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheWriter;
+import org.apache.geode.cache.CacheWriterException;
+import org.apache.geode.cache.ExpirationAction;
+import org.apache.geode.cache.ExpirationAttributes;
+import org.apache.geode.cache.PartitionAttributes;
+import org.apache.geode.cache.PartitionAttributesFactory;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.cache.partition.PartitionRegionHelper;
+import org.apache.geode.cache.util.CacheWriterAdapter;
+import org.apache.geode.distributed.DistributedSystemDisconnectedException;
+import org.apache.geode.distributed.internal.DMStats;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import 
org.apache.geode.distributed.internal.membership.api.MembershipManagerHelper;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.DistributedDiskDirRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+
+/**
+ * Tests to verify that {@link PartitionedRegion#clear()} cancels all 
remaining expiration tasks
+ * on the {@link PartitionedRegion} once the operation is executed.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class PartitionedRegionClearWithExpirationDUnitTest implements 
Serializable {
+  private static final Integer BUCKETS = 13;
+  private static final Integer EXPIRATION_TIME = 30;
+  private static final String REGION_NAME = "PartitionedRegion";
+  private static final String TEST_CASE_NAME =
+  "[{index}] {method}(Coordinator:{0}, RegionType:{1}, 
ExpirationAction:{2})";
+
+  @Rule
+  public DistributedRule distributedRule = new DistributedRule(3);

Review comment:
   This rule is not used.

##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithExpirationDUnitTest.java
##
@@ -0,0 +1,535 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF

[GitHub] [geode] davebarnes97 commented on issue #4975: Logging documentation fixes

[GitBox]

davebarnes97 commented on issue #4975:
URL: https://github.com/apache/geode/pull/4975#issuecomment-616863201


   Thanks for this contribution, @alb3rtobr. 
   For the gfsh command reference writeup on change loglevel, I'd like to 
suggest a minor re-wording, which replaces future tense with present tense.
   
   Now:
   Changes the logging level on specified members. This command will take 
effect if the default <%=vars.product_name%> logging configuration is used.
   
   In case of using custom `Log4J` configuration, this command will not work 
unless the member whose logging level you want to change was started using the 
`--J=-Dgeode.LOG_LEVEL_UPDATE_OCCURS=ALWAYS` system property.
   
   Suggested revision:
   Changes the logging level on specified members. This command takes effect if 
the default <%=vars.product_name%> logging configuration is used.
   
   When using a custom `Log4J` configuration, this command takes effect only if 
the member whose logging level you want to change was started using the 
`--J=-Dgeode.LOG_LEVEL_UPDATE_OCCURS=ALWAYS` system property.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] demery-pivotal opened a new pull request #4977: GEODE-7851: Pulse refreshes expired access tokens

[GitBox]

demery-pivotal opened a new pull request #4977:
URL: https://github.com/apache/geode/pull/4977


   If a user's access token expires, Pulse attempts to refresh it. If the
   refresh fails, Pulse logs the user out and redirects the browser to
   /pulse/clusterLogout.
   
   Changes in Repository:
   - When OAuth is configured, before returning the user's cluster,
 getCluster() checks whether the user's access token has expired.
   - If the access token has expired, the repository attempts to refresh
 it.  If the refresh succeeds, the repository reconnects the user's
 cluster to JMX and returns it.
   - If the refresh fails, the repository disconnects the user's cluster
 from JMX, removes the cluster from the repository, and throws an
 authentication or authorization exception.
   
   Changes in PulseController:
   - If the service call throws an authentication or authorization
 exception, PulseController.  getPulseUpdate() returns a 401 status.
   
   Changes in pulsescript/common.js:
   - If a Pulse ajax call returns a 401 status, ajaxPost() redirects the
 browser to /pulse/clusterLogout to log the user out and request
 re-authorization.
   
   Co-authored-by: Joris Melchior 
   Co-authored-by: Dale Emery 
   
   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:
   - [X] Is there a JIRA ticket associated with this PR? Is it referenced in 
the commit message?
   
   - [X] Has your PR been rebased against the latest commit within the target 
branch (typically `develop`)?
   
   - [X] Is your initial contribution a single, squashed commit?
   
   - [X] Does `gradlew build` run cleanly?
   
   - [ ] Have you written or updated unit tests to verify your changes?
   
   - [ ] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)?
   
   ### Note:
   Please ensure that once the PR is submitted, check Concourse 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.
   



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] albertogpz commented on issue #4928: GEODE-7971: Gw sender deliver TX events atomically to Gw receivers

[GitBox]

albertogpz commented on issue #4928:
URL: https://github.com/apache/geode/pull/4928#issuecomment-616957705


   > Sorry, I haven't had time to dig deeply into this PR.
   > 
   > There were some concerns on the rfc for interleaving or out of ordering of 
events. I might have missed it, but were there some tests that can prove that 
we don't get out of order events on the receiving side?
   
   As pointed out in the RFC, with this feature there could be some reordering 
of events but it would happen on events so close in time that it could not even 
be assured that the original order in the queue was the right order.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] albertogpz commented on a change in pull request #4928: GEODE-7971: Gw sender deliver TX events atomically to Gw receivers

[GitBox]

albertogpz commented on a change in pull request #4928:
URL: https://github.com/apache/geode/pull/4928#discussion_r411879596



##
File path: 
geode-core/src/main/java/org/apache/geode/internal/cache/wan/serial/SerialGatewaySenderQueue.java
##
@@ -410,6 +441,53 @@ public Object peek() throws CacheException {
 // so no need to worry about off-heap refCount.
   }
 
+  private void peekEventsFromIncompleteTransactions(List batch,

Review comment:
   You are right, the code is very similar (just as it happens with the 
peek method) but the structures on which they operate are different:
   On the SerialGatewaySenderQueue it operates on a set of transactionIds while 
on the ParallelGatewaySenderQueue it operates on a map of .
   
   Besides, the difference between the Serial queue and the parallel queue 
makes it different the access to them. On the parallel you need to pass the 
bucketId and the partition region. On the serial one, you need to pass the last 
accessed key so that you do not go through the same elements over and over 
because those are not removed immediately when peeked from the queue as it 
happens with the parallel queue.
   
   As a consequence I did not find an easy way to reuse code that did end up 
making things more complex.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] albertogpz commented on a change in pull request #4928: GEODE-7971: Gw sender deliver TX events atomically to Gw receivers

[GitBox]

albertogpz commented on a change in pull request #4928:
URL: https://github.com/apache/geode/pull/4928#discussion_r411879596



##
File path: 
geode-core/src/main/java/org/apache/geode/internal/cache/wan/serial/SerialGatewaySenderQueue.java
##
@@ -410,6 +441,53 @@ public Object peek() throws CacheException {
 // so no need to worry about off-heap refCount.
   }
 
+  private void peekEventsFromIncompleteTransactions(List batch,

Review comment:
   You are right, the code is very similar (just as it happens with the 
peek method) but the structures on which they operate are different:
   On the SerialGatewaySenderQueue it operates on a set of transactionIds while 
on the ParallelGatewaySenderQueue it operates on a map of .
   
   Besides, the difference between the Serial queue and the parallel queue 
makes it different the access to them. On the parallel you need to pass the 
bucketId and the partition region. On the serial one, you need to pass the last 
accessed key so that you do not go through the same elements over and over 
because those are not removed immediately when peeked from the queue as it 
happens with the parallel queue.
   
   As a consequence I did not find an easy way to reuse code that did not end 
up making things more complex.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jujoramos commented on a change in pull request #4970: GEODE-7676: Add PR clear with expiration tests

[GitBox]

jujoramos commented on a change in pull request #4970:
URL: https://github.com/apache/geode/pull/4970#discussion_r411981569



##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithExpirationDUnitTest.java
##
@@ -0,0 +1,535 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.apache.geode.cache.ExpirationAction.DESTROY;
+import static org.apache.geode.cache.ExpirationAction.INVALIDATE;
+import static org.apache.geode.cache.RegionShortcut.PARTITION;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_OVERFLOW;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT_OVERFLOW;
+import static org.apache.geode.internal.util.ArrayUtils.asList;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static org.apache.geode.test.dunit.VM.getVM;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.IntStream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import junitparams.naming.TestCaseName;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.ForcedDisconnectException;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheWriter;
+import org.apache.geode.cache.CacheWriterException;
+import org.apache.geode.cache.ExpirationAction;
+import org.apache.geode.cache.ExpirationAttributes;
+import org.apache.geode.cache.PartitionAttributes;
+import org.apache.geode.cache.PartitionAttributesFactory;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.cache.partition.PartitionRegionHelper;
+import org.apache.geode.cache.util.CacheWriterAdapter;
+import org.apache.geode.distributed.DistributedSystemDisconnectedException;
+import org.apache.geode.distributed.internal.DMStats;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import 
org.apache.geode.distributed.internal.membership.api.MembershipManagerHelper;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.DistributedDiskDirRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+
+/**
+ * Tests to verify that {@link PartitionedRegion#clear()} cancels all 
remaining expiration tasks
+ * on the {@link PartitionedRegion} once the operation is executed.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class PartitionedRegionClearWithExpirationDUnitTest implements 
Serializable {
+  private static final Integer BUCKETS = 13;
+  private static final Integer EXPIRATION_TIME = 30;
+  private static final String REGION_NAME = "PartitionedRegion";
+  private static final String TEST_CASE_NAME =
+  "[{index}] {method}(Coordinator:{0}, RegionType:{1}, 
ExpirationAction:{2})";
+
+  @Rule
+  public DistributedRule distributedRule = new DistributedRule(3);
+
+  @Rule
+  public CacheRule cacheRule = CacheRule.builder().createCacheInAll().build();
+
+  @Rule
+  public DistributedDiskDirRule distributedDiskDirRule = new 
DistributedDiskDirRule();
+
+  private VM accessor, server1, server2;
+
+  private enum TestVM {
+ACCESSOR(0), SERVER1(1), SERVER2(2);
+
+final int vmNumber;
+
+TestVM(int vmNumber) {
+  this.vmNumber = vmNumber;
+}
+  }
+
+  @SuppressWarnings("unused")
+  stat

[GitHub] [geode] jujoramos commented on a change in pull request #4970: GEODE-7676: Add PR clear with expiration tests

[GitBox]

jujoramos commented on a change in pull request #4970:
URL: https://github.com/apache/geode/pull/4970#discussion_r411985789



##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithExpirationDUnitTest.java
##
@@ -0,0 +1,535 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.apache.geode.cache.ExpirationAction.DESTROY;
+import static org.apache.geode.cache.ExpirationAction.INVALIDATE;
+import static org.apache.geode.cache.RegionShortcut.PARTITION;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_OVERFLOW;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT_OVERFLOW;
+import static org.apache.geode.internal.util.ArrayUtils.asList;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static org.apache.geode.test.dunit.VM.getVM;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.IntStream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import junitparams.naming.TestCaseName;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.ForcedDisconnectException;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheWriter;
+import org.apache.geode.cache.CacheWriterException;
+import org.apache.geode.cache.ExpirationAction;
+import org.apache.geode.cache.ExpirationAttributes;
+import org.apache.geode.cache.PartitionAttributes;
+import org.apache.geode.cache.PartitionAttributesFactory;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.cache.partition.PartitionRegionHelper;
+import org.apache.geode.cache.util.CacheWriterAdapter;
+import org.apache.geode.distributed.DistributedSystemDisconnectedException;
+import org.apache.geode.distributed.internal.DMStats;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import 
org.apache.geode.distributed.internal.membership.api.MembershipManagerHelper;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.DistributedDiskDirRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+
+/**
+ * Tests to verify that {@link PartitionedRegion#clear()} cancels all 
remaining expiration tasks
+ * on the {@link PartitionedRegion} once the operation is executed.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class PartitionedRegionClearWithExpirationDUnitTest implements 
Serializable {
+  private static final Integer BUCKETS = 13;
+  private static final Integer EXPIRATION_TIME = 30;
+  private static final String REGION_NAME = "PartitionedRegion";
+  private static final String TEST_CASE_NAME =
+  "[{index}] {method}(Coordinator:{0}, RegionType:{1}, 
ExpirationAction:{2})";
+
+  @Rule
+  public DistributedRule distributedRule = new DistributedRule(3);
+
+  @Rule
+  public CacheRule cacheRule = CacheRule.builder().createCacheInAll().build();
+
+  @Rule
+  public DistributedDiskDirRule distributedDiskDirRule = new 
DistributedDiskDirRule();
+
+  private VM accessor, server1, server2;
+
+  private enum TestVM {
+ACCESSOR(0), SERVER1(1), SERVER2(2);
+
+final int vmNumber;
+
+TestVM(int vmNumber) {
+  this.vmNumber = vmNumber;
+}
+  }
+
+  @SuppressWarnings("unused")
+  stat

[GitHub] [geode] jujoramos commented on a change in pull request #4970: GEODE-7676: Add PR clear with expiration tests

[GitBox]

jujoramos commented on a change in pull request #4970:
URL: https://github.com/apache/geode/pull/4970#discussion_r411988106



##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithExpirationDUnitTest.java
##
@@ -0,0 +1,535 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.apache.geode.cache.ExpirationAction.DESTROY;
+import static org.apache.geode.cache.ExpirationAction.INVALIDATE;
+import static org.apache.geode.cache.RegionShortcut.PARTITION;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_OVERFLOW;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT_OVERFLOW;
+import static org.apache.geode.internal.util.ArrayUtils.asList;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static org.apache.geode.test.dunit.VM.getVM;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.IntStream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import junitparams.naming.TestCaseName;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.ForcedDisconnectException;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheWriter;
+import org.apache.geode.cache.CacheWriterException;
+import org.apache.geode.cache.ExpirationAction;
+import org.apache.geode.cache.ExpirationAttributes;
+import org.apache.geode.cache.PartitionAttributes;
+import org.apache.geode.cache.PartitionAttributesFactory;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.cache.partition.PartitionRegionHelper;
+import org.apache.geode.cache.util.CacheWriterAdapter;
+import org.apache.geode.distributed.DistributedSystemDisconnectedException;
+import org.apache.geode.distributed.internal.DMStats;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import 
org.apache.geode.distributed.internal.membership.api.MembershipManagerHelper;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.DistributedDiskDirRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+
+/**
+ * Tests to verify that {@link PartitionedRegion#clear()} cancels all 
remaining expiration tasks
+ * on the {@link PartitionedRegion} once the operation is executed.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class PartitionedRegionClearWithExpirationDUnitTest implements 
Serializable {
+  private static final Integer BUCKETS = 13;
+  private static final Integer EXPIRATION_TIME = 30;
+  private static final String REGION_NAME = "PartitionedRegion";
+  private static final String TEST_CASE_NAME =
+  "[{index}] {method}(Coordinator:{0}, RegionType:{1}, 
ExpirationAction:{2})";
+
+  @Rule
+  public DistributedRule distributedRule = new DistributedRule(3);
+
+  @Rule
+  public CacheRule cacheRule = CacheRule.builder().createCacheInAll().build();
+
+  @Rule
+  public DistributedDiskDirRule distributedDiskDirRule = new 
DistributedDiskDirRule();
+
+  private VM accessor, server1, server2;
+
+  private enum TestVM {
+ACCESSOR(0), SERVER1(1), SERVER2(2);
+
+final int vmNumber;
+
+TestVM(int vmNumber) {
+  this.vmNumber = vmNumber;
+}
+  }
+
+  @SuppressWarnings("unused")
+  stat

[GitHub] [geode] jujoramos commented on a change in pull request #4970: GEODE-7676: Add PR clear with expiration tests

[GitBox]

jujoramos commented on a change in pull request #4970:
URL: https://github.com/apache/geode/pull/4970#discussion_r411992120



##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithExpirationDUnitTest.java
##
@@ -0,0 +1,535 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.apache.geode.cache.ExpirationAction.DESTROY;
+import static org.apache.geode.cache.ExpirationAction.INVALIDATE;
+import static org.apache.geode.cache.RegionShortcut.PARTITION;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_OVERFLOW;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT_OVERFLOW;
+import static org.apache.geode.internal.util.ArrayUtils.asList;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static org.apache.geode.test.dunit.VM.getVM;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.IntStream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import junitparams.naming.TestCaseName;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.ForcedDisconnectException;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheWriter;
+import org.apache.geode.cache.CacheWriterException;
+import org.apache.geode.cache.ExpirationAction;
+import org.apache.geode.cache.ExpirationAttributes;
+import org.apache.geode.cache.PartitionAttributes;
+import org.apache.geode.cache.PartitionAttributesFactory;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.cache.partition.PartitionRegionHelper;
+import org.apache.geode.cache.util.CacheWriterAdapter;
+import org.apache.geode.distributed.DistributedSystemDisconnectedException;
+import org.apache.geode.distributed.internal.DMStats;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import 
org.apache.geode.distributed.internal.membership.api.MembershipManagerHelper;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.DistributedDiskDirRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+
+/**
+ * Tests to verify that {@link PartitionedRegion#clear()} cancels all 
remaining expiration tasks
+ * on the {@link PartitionedRegion} once the operation is executed.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class PartitionedRegionClearWithExpirationDUnitTest implements 
Serializable {
+  private static final Integer BUCKETS = 13;
+  private static final Integer EXPIRATION_TIME = 30;
+  private static final String REGION_NAME = "PartitionedRegion";
+  private static final String TEST_CASE_NAME =
+  "[{index}] {method}(Coordinator:{0}, RegionType:{1}, 
ExpirationAction:{2})";
+
+  @Rule
+  public DistributedRule distributedRule = new DistributedRule(3);

Review comment:
   It's used to launch the `DistributedTest` VMs. See 
[this](https://cwiki.apache.org/confluence/display/GEODE/Updating+Old+DUnit+Tests)
 for further details.





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.

For queries about this service, p

[GitHub] [geode] jujoramos commented on a change in pull request #4970: GEODE-7676: Add PR clear with expiration tests

[GitBox]

jujoramos commented on a change in pull request #4970:
URL: https://github.com/apache/geode/pull/4970#discussion_r411993686



##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithExpirationDUnitTest.java
##
@@ -0,0 +1,535 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.apache.geode.cache.ExpirationAction.DESTROY;
+import static org.apache.geode.cache.ExpirationAction.INVALIDATE;
+import static org.apache.geode.cache.RegionShortcut.PARTITION;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_OVERFLOW;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT_OVERFLOW;
+import static org.apache.geode.internal.util.ArrayUtils.asList;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static org.apache.geode.test.dunit.VM.getVM;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.IntStream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import junitparams.naming.TestCaseName;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.ForcedDisconnectException;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheWriter;
+import org.apache.geode.cache.CacheWriterException;
+import org.apache.geode.cache.ExpirationAction;
+import org.apache.geode.cache.ExpirationAttributes;
+import org.apache.geode.cache.PartitionAttributes;
+import org.apache.geode.cache.PartitionAttributesFactory;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.cache.partition.PartitionRegionHelper;
+import org.apache.geode.cache.util.CacheWriterAdapter;
+import org.apache.geode.distributed.DistributedSystemDisconnectedException;
+import org.apache.geode.distributed.internal.DMStats;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import 
org.apache.geode.distributed.internal.membership.api.MembershipManagerHelper;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.DistributedDiskDirRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+
+/**
+ * Tests to verify that {@link PartitionedRegion#clear()} cancels all 
remaining expiration tasks
+ * on the {@link PartitionedRegion} once the operation is executed.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class PartitionedRegionClearWithExpirationDUnitTest implements 
Serializable {
+  private static final Integer BUCKETS = 13;
+  private static final Integer EXPIRATION_TIME = 30;
+  private static final String REGION_NAME = "PartitionedRegion";
+  private static final String TEST_CASE_NAME =
+  "[{index}] {method}(Coordinator:{0}, RegionType:{1}, 
ExpirationAction:{2})";
+
+  @Rule
+  public DistributedRule distributedRule = new DistributedRule(3);
+
+  @Rule
+  public CacheRule cacheRule = CacheRule.builder().createCacheInAll().build();
+
+  @Rule
+  public DistributedDiskDirRule distributedDiskDirRule = new 
DistributedDiskDirRule();

Review comment:
   It's used to ensure all VMs use their own `disk-store`, specifically to 
avoid flakiness with the `PERSISTENT` regions. It works in conjunction with the 
`DistributedRule`.




--

[GitHub] [geode] jujoramos commented on a change in pull request #4970: GEODE-7676: Add PR clear with expiration tests

[GitBox]

jujoramos commented on a change in pull request #4970:
URL: https://github.com/apache/geode/pull/4970#discussion_r411996029



##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithExpirationDUnitTest.java
##
@@ -0,0 +1,535 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.apache.geode.cache.ExpirationAction.DESTROY;
+import static org.apache.geode.cache.ExpirationAction.INVALIDATE;
+import static org.apache.geode.cache.RegionShortcut.PARTITION;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_PERSISTENT_OVERFLOW;
+import static org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_OVERFLOW;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT;
+import static 
org.apache.geode.cache.RegionShortcut.PARTITION_REDUNDANT_PERSISTENT_OVERFLOW;
+import static org.apache.geode.internal.util.ArrayUtils.asList;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static org.apache.geode.test.dunit.VM.getVM;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.IntStream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import junitparams.naming.TestCaseName;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.ForcedDisconnectException;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheWriter;
+import org.apache.geode.cache.CacheWriterException;
+import org.apache.geode.cache.ExpirationAction;
+import org.apache.geode.cache.ExpirationAttributes;
+import org.apache.geode.cache.PartitionAttributes;
+import org.apache.geode.cache.PartitionAttributesFactory;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.cache.partition.PartitionRegionHelper;
+import org.apache.geode.cache.util.CacheWriterAdapter;
+import org.apache.geode.distributed.DistributedSystemDisconnectedException;
+import org.apache.geode.distributed.internal.DMStats;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import 
org.apache.geode.distributed.internal.membership.api.MembershipManagerHelper;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.DistributedDiskDirRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+
+/**
+ * Tests to verify that {@link PartitionedRegion#clear()} cancels all 
remaining expiration tasks
+ * on the {@link PartitionedRegion} once the operation is executed.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class PartitionedRegionClearWithExpirationDUnitTest implements 
Serializable {
+  private static final Integer BUCKETS = 13;
+  private static final Integer EXPIRATION_TIME = 30;
+  private static final String REGION_NAME = "PartitionedRegion";
+  private static final String TEST_CASE_NAME =
+  "[{index}] {method}(Coordinator:{0}, RegionType:{1}, 
ExpirationAction:{2})";
+
+  @Rule
+  public DistributedRule distributedRule = new DistributedRule(3);
+
+  @Rule
+  public CacheRule cacheRule = CacheRule.builder().createCacheInAll().build();
+
+  @Rule
+  public DistributedDiskDirRule distributedDiskDirRule = new 
DistributedDiskDirRule();
+
+  private VM accessor, server1, server2;
+
+  private enum TestVM {
+ACCESSOR(0), SERVER1(1), SERVER2(2);
+
+final int vmNumber;
+
+TestVM(int vmNumber) {
+  this.vmNumber = vmNumber;
+}
+  }
+
+  @SuppressWarnings("unused")
+  stat

[GitHub] [geode] alb3rtobr opened a new pull request #4978: Fix for regression introduced by GEODE-7565

[GitBox]

alb3rtobr opened a new pull request #4978:
URL: https://github.com/apache/geode/pull/4978


   This PR reverts the revert of GEODE-7565 and introduces a fix for the 
problem identified



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jujoramos opened a new pull request #4979: [DO NOT REVIEW]: GEM-2885

[GitBox]

jujoramos opened a new pull request #4979:
URL: https://github.com/apache/geode/pull/4979


   



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode-examples] metatype opened a new pull request #93: GEODE-8006: testing notifications

[GitBox]

metatype opened a new pull request #93:
URL: https://github.com/apache/geode-examples/pull/93


   



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] mhansonp opened a new pull request #4980: GEODE-7935: Awaiting for verification steps.

[GitBox]

mhansonp opened a new pull request #4980:
URL: https://github.com/apache/geode/pull/4980


   This test just needed some awaits added to allow the test to retry.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] dschneider-pivotal commented on a change in pull request #4973: improvement/GEODE-8002 Refactor: Extract class to encapsulate concurrent execution

[GitBox]

dschneider-pivotal commented on a change in pull request #4973:
URL: https://github.com/apache/geode/pull/4973#discussion_r412318830



##
File path: 
geode-redis/src/integrationTest/java/org/apache/geode/redis/general/LoopingThreads.java
##
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.general;
+
+import java.util.Arrays;
+import java.util.concurrent.CountDownLatch;
+import java.util.function.Function;
+import java.util.stream.Stream;
+
+public class LoopingThreads {
+  private final int iterationCount;
+  private final Function[] functions;
+
+  @SuppressWarnings("unchecked")
+  public LoopingThreads(int iterationCount,
+  Function... functions) {
+this.iterationCount = iterationCount;
+this.functions = (Function[]) functions;
+  }
+
+  public void run() {
+CountDownLatch latch = new CountDownLatch(1);
+Stream loopingThreadStream = Arrays
+.stream(functions)
+.map((r) -> new LoopingThread(r, iterationCount, latch))
+.map((t) -> {
+  t.start();
+  return t;
+});
+
+latch.countDown();
+
+loopingThreadStream.forEach(loopingThread -> {
+  try {
+loopingThread.join();
+  } catch (InterruptedException e) {
+throw new RuntimeException(e);
+  }
+});
+
+  }
+
+  private class LoopingRunnable implements Runnable {
+private final Function runnable;
+private final int iterationCount;
+private CountDownLatch startLatch;
+
+public LoopingRunnable(Function runnable, int 
iterationCount,
+CountDownLatch startLatch) {
+  this.runnable = runnable;
+  this.iterationCount = iterationCount;
+  this.startLatch = startLatch;
+}
+
+@Override
+public void run() {
+  try {
+startLatch.await();
+  } catch (InterruptedException e) {
+throw new RuntimeException(e);
+  }
+  for (int i = 0; i < iterationCount; i++) {
+runnable.apply(i);
+Thread.yield();

Review comment:
   not a deal breaker. I just merged in a new dunit test for concurrent 
testing of SADD. It would be nice to have this encapsulated concurrency code in 
that test before it becomes the pattern for subsequent tests





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] nabarunnag commented on a change in pull request #4976: GEODE-7997: Document needed location of parallel gateway sender disk …

[GitBox]

nabarunnag commented on a change in pull request #4976:
URL: https://github.com/apache/geode/pull/4976#discussion_r412320360



##
File path: geode-docs/managing/disk_storage/using_disk_stores.html.md.erb
##
@@ -170,7 +170,10 @@ Example of using a named disk store for PDX serialization 
metadata (cache.xml):
 
 Gateway sender queues are always overflowed and may be persisted. Assign them 
to overflow disk stores if you do not persist, and to persistence disk stores 
if you do.
 
-Example of using a named disk store for gateway sender queue persistence:
+Persisted data from a parallel gateway sender must go to the same disk
+store as used by the region.

Review comment:
   This is not required, but it would be nice to tell the user why this 
needs to be done. i.e. parallel gateway sender queues are  collocated with the 
parent region





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode-examples] metatype commented on issue #93: GEODE-8006: testing notifications

[GitBox]

metatype commented on issue #93:
URL: https://github.com/apache/geode-examples/pull/93#issuecomment-617279841


   Testing a comment.  This should be a comment in the JIRA.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode-examples] metatype commented on issue #93: GEODE-8006: testing notifications

[GitBox]

metatype commented on issue #93:
URL: https://github.com/apache/geode-examples/pull/93#issuecomment-617284504


   Test comment number 2.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] nabarunnag commented on a change in pull request #4976: GEODE-7997: Document needed location of parallel gateway sender disk …

[GitBox]

nabarunnag commented on a change in pull request #4976:
URL: https://github.com/apache/geode/pull/4976#discussion_r412320360



##
File path: geode-docs/managing/disk_storage/using_disk_stores.html.md.erb
##
@@ -170,7 +170,10 @@ Example of using a named disk store for PDX serialization 
metadata (cache.xml):
 
 Gateway sender queues are always overflowed and may be persisted. Assign them 
to overflow disk stores if you do not persist, and to persistence disk stores 
if you do.
 
-Example of using a named disk store for gateway sender queue persistence:
+Persisted data from a parallel gateway sender must go to the same disk
+store as used by the region.

Review comment:
   my  comment below  is not required, but it would be nice to tell the 
user why this needs to be done. i.e. parallel gateway sender queues are  
collocated with the parent region





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] prettyClouds commented on a change in pull request #4973: improvement/GEODE-8002 Refactor: Extract class to encapsulate concurrent execution

[GitBox]

prettyClouds commented on a change in pull request #4973:
URL: https://github.com/apache/geode/pull/4973#discussion_r412336050



##
File path: 
geode-redis/src/integrationTest/java/org/apache/geode/redis/general/LoopingThreads.java
##
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.general;
+
+import java.util.Arrays;
+import java.util.concurrent.CountDownLatch;
+import java.util.function.Function;
+import java.util.stream.Stream;
+
+public class LoopingThreads {
+  private final int iterationCount;
+  private final Function[] functions;
+
+  @SuppressWarnings("unchecked")
+  public LoopingThreads(int iterationCount,
+  Function... functions) {
+this.iterationCount = iterationCount;
+this.functions = (Function[]) functions;
+  }
+
+  public void run() {
+CountDownLatch latch = new CountDownLatch(1);
+Stream loopingThreadStream = Arrays
+.stream(functions)
+.map((r) -> new LoopingThread(r, iterationCount, latch))
+.map((t) -> {
+  t.start();
+  return t;
+});
+
+latch.countDown();
+
+loopingThreadStream.forEach(loopingThread -> {
+  try {
+loopingThread.join();
+  } catch (InterruptedException e) {
+throw new RuntimeException(e);
+  }
+});
+
+  }
+
+  private class LoopingRunnable implements Runnable {
+private final Function runnable;
+private final int iterationCount;
+private CountDownLatch startLatch;
+
+public LoopingRunnable(Function runnable, int 
iterationCount,
+CountDownLatch startLatch) {
+  this.runnable = runnable;
+  this.iterationCount = iterationCount;
+  this.startLatch = startLatch;
+}
+
+@Override
+public void run() {
+  try {
+startLatch.await();
+  } catch (InterruptedException e) {
+throw new RuntimeException(e);
+  }
+  for (int i = 0; i < iterationCount; i++) {
+runnable.apply(i);
+Thread.yield();

Review comment:
   alright.  it's a real PR now.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] kirklund commented on issue #4924: DRAFT: GEODE-7964: Upgrade Mockito to 3.3.3

[GitBox]

kirklund commented on issue #4924:
URL: https://github.com/apache/geode/pull/4924#issuecomment-617298733


   Another unrelated JMXMBeanReconnectDUnitTest failure.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode-examples] metatype opened a new pull request #94: GEODE-8006 Added .asf.yaml to control notifications

[GitBox]

metatype opened a new pull request #94:
URL: https://github.com/apache/geode-examples/pull/94


   



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] upthewaterspout commented on issue #4949: GEODE-7982: Close the client first in rolling upgrade test

[GitBox]

upthewaterspout commented on issue #4949:
URL: https://github.com/apache/geode/pull/4949#issuecomment-617301600


   JMXMBeanReconnectDUnitTest failed, but that is not related to this test.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] upthewaterspout commented on a change in pull request #4909: GEODE-7953: Restore Redundancy Internal API

[GitBox]

upthewaterspout commented on a change in pull request #4909:
URL: https://github.com/apache/geode/pull/4909#discussion_r412362589



##
File path: 
geode-core/src/main/java/org/apache/geode/cache/control/RestoreRedundancyResults.java
##
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.cache.control;
+
+import java.util.Map;
+
+import org.apache.geode.cache.partition.PartitionRebalanceInfo;
+import org.apache.geode.internal.cache.control.RegionRedundancyStatus;
+import 
org.apache.geode.internal.cache.partitioned.PartitionedRegionRebalanceOp;
+
+/**
+ * A class to collect the results of restore redundancy operations for one or 
more regions and
+ * determine the success of failure of the operation.
+ */
+public interface RestoreRedundancyResults {
+
+  /**
+   * {@link #SUCCESS} is defined as every included region having fully 
satisfied redundancy.
+   * {@link #FAILURE} is defined as at least one region that is configured to 
have redundant copies
+   * having fewer than its configured number of redundant copies.
+   * {@link #ERROR} is for cases when the restore redundancy operation was 
unable to begin or threw
+   * an exception.
+   */
+  enum Status {
+SUCCESS,
+FAILURE,
+ERROR
+  }
+
+  /**
+   * Adds the contents of another {@link RestoreRedundancyResults} object to 
this one, including
+   * both {@link RegionRedundancyStatus} objects and information on the number 
of primaries
+   * reassigned and the time taken to reassign them.
+   *
+   * @param results a {@link RestoreRedundancyResults} object whose contents 
will be added to this
+   *one.
+   */
+  void addRegionResults(RestoreRedundancyResults results);
+
+  /**
+   * Adds information regarding the number of primaries reassigned and the 
time taken to reassign
+   * them during a restore redundancy operation.
+   *
+   * @param details a {@link PartitionRebalanceInfo} generated by a
+   *{@link PartitionedRegionRebalanceOp} operation.
+   */
+  void addPrimaryReassignmentDetails(PartitionRebalanceInfo details);
+
+  /**
+   * Adds one {@link RegionRedundancyStatus} to the result set.
+   *
+   * @param regionResult The {@link RegionRedundancyStatus} to be added to the 
result set.
+   */
+  void addRegionResult(RegionRedundancyStatus regionResult);

Review comment:
   This is leaking an internal class into the public API. The public API 
should not refer to internal classes.
   
   In this case, I'm not clear why these mutation methods are part of the 
public API at all. Would it make more sense for RestoreRedundancyResults to be 
a read only view of the results?

##
File path: 
geode-core/src/main/java/org/apache/geode/cache/control/RestoreRedundancyResults.java
##
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.cache.control;
+
+import java.util.Map;
+
+import org.apache.geode.cache.partition.PartitionRebalanceInfo;
+import org.apache.geode.internal.cache.control.RegionRedundancyStatus;
+import 
org.apache.geode.internal.cache.partitioned.PartitionedRegionRebalanceOp;
+
+/**
+ * A class to collect the results of restore redundancy operations for one or 
more regions and
+ * determine the success of failure of the operation.
+ */
+public interface RestoreRedundancyResults {
+
+  /**
+   * {@link #SUCCESS} is defined as every included region having fully 
satisfied redundancy.
+   * {@link #FA

[GitHub] [geode] bschuchardt commented on a change in pull request #4959: GEODE-7852: Create test for running cache operations with a dropped …

[GitBox]

bschuchardt commented on a change in pull request #4959:
URL: https://github.com/apache/geode/pull/4959#discussion_r412381622



##
File path: 
geode-assembly/src/acceptanceTest/java/org/apache/geode/client/sni/ClientSNIDropProxyAcceptanceTest.java
##
@@ -0,0 +1,197 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.client.sni;
+
+import static 
com.palantir.docker.compose.execution.DockerComposeExecArgument.arguments;
+import static 
com.palantir.docker.compose.execution.DockerComposeExecOption.options;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_ENABLED_COMPONENTS;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_ENDPOINT_IDENTIFICATION_ENABLED;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE_TYPE;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_REQUIRE_AUTHENTICATION;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE_PASSWORD;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static 
org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.IOException;
+import java.net.URL;
+import java.util.Properties;
+
+import com.palantir.docker.compose.DockerComposeRule;
+import com.palantir.docker.compose.execution.DockerComposeRunArgument;
+import com.palantir.docker.compose.execution.DockerComposeRunOption;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestRule;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.client.ClientCacheFactory;
+import org.apache.geode.cache.client.ClientRegionShortcut;
+import org.apache.geode.cache.client.NoAvailableLocatorsException;
+import org.apache.geode.cache.client.proxy.ProxySocketFactories;
+import org.apache.geode.test.junit.rules.IgnoreOnWindowsRule;
+
+public class ClientSNIDropProxyAcceptanceTest {
+
+  private static final URL DOCKER_COMPOSE_PATH =
+  ClientSNIDropProxyAcceptanceTest.class.getResource("docker-compose.yml");
+
+  // Docker compose does not work on windows in CI. Ignore this test on windows
+  // Using a RuleChain to make sure we ignore the test before the rule comes 
into play
+  @ClassRule
+  public static TestRule ignoreOnWindowsRule = new IgnoreOnWindowsRule();
+
+  @Rule
+  public DockerComposeRule docker = DockerComposeRule.builder()
+  .file(DOCKER_COMPOSE_PATH.getPath())
+  .build();
+
+  private ClientCache cache;
+
+  private String trustStorePath;
+  private int proxyPort;
+
+  @Before
+  public void before() throws IOException, InterruptedException {
+trustStorePath =
+createTempFileFromResource(ClientSNIDropProxyAcceptanceTest.class,
+"geode-config/truststore.jks")
+.getAbsolutePath();
+docker.exec(options("-T"), "geode",
+arguments("gfsh", "run", "--file=/geode/scripts/geode-starter.gfsh"));
+  }
+
+  @After
+  public void after() {
+ensureCacheClosed();
+  }
+
+  @Test
+  public void performSimpleOperationsDropSNIProxy()
+  throws IOException,
+  InterruptedException {
+final Region region = getRegion();
+
+region.put("Roy Hobbs", 9);
+assertThat(region.get("Roy Hobbs")).isEqualTo(9);
+
+stopProxy();
+
+assertThatThrownBy(() -> region.get("Roy Hobbs"))
+.isInstanceOf(NoAvailableLocatorsException.class)
+.hasMessageContaining("Unable to connect to any locators in the list");
+
+
+restartProxy();
+
+await().untilAsserted(() -> assertThat(region.get("Roy 
Hobbs")).isEqualTo(9));
+
+region.put("Bennie Rodriquez", 30);
+assertThat(region.get("Bennie Rodriquez")).isEqualTo(30);
+
+region.put("Jake Taylor", 7);
+region.put("Crash Davis", 8);
+
+region.put("Ricky Vaughn", 99);
+region.put("Ebbie Calvin LaLoosh", 37);
+
+  }
+
+  pri

[GitHub] [geode] Bill commented on a change in pull request #4959: GEODE-7852: Create test for running cache operations with a dropped …

[GitBox]

Bill commented on a change in pull request #4959:
URL: https://github.com/apache/geode/pull/4959#discussion_r412386539



##
File path: 
geode-assembly/src/acceptanceTest/java/org/apache/geode/client/sni/ClientSNIDropProxyAcceptanceTest.java
##
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.client.sni;
+
+import static 
com.palantir.docker.compose.execution.DockerComposeExecArgument.arguments;
+import static 
com.palantir.docker.compose.execution.DockerComposeExecOption.options;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_ENABLED_COMPONENTS;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_ENDPOINT_IDENTIFICATION_ENABLED;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE_TYPE;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_REQUIRE_AUTHENTICATION;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE_PASSWORD;
+import static 
org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.IOException;
+import java.net.URL;
+import java.util.Properties;
+
+import com.palantir.docker.compose.DockerComposeRule;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestRule;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.client.ClientCacheFactory;
+import org.apache.geode.cache.client.ClientRegionShortcut;
+import org.apache.geode.cache.client.NoAvailableLocatorsException;
+import org.apache.geode.cache.client.proxy.ProxySocketFactories;
+import org.apache.geode.test.junit.rules.IgnoreOnWindowsRule;
+
+public class ClientSNIDropProxyAcceptanceTest {
+
+  private static final URL DOCKER_COMPOSE_PATH =
+  ClientSNIDropProxyAcceptanceTest.class.getResource("docker-compose.yml");
+
+  // Docker compose does not work on windows in CI. Ignore this test on windows
+  // Using a RuleChain to make sure we ignore the test before the rule comes 
into play
+  @ClassRule
+  public static TestRule ignoreOnWindowsRule = new IgnoreOnWindowsRule();
+
+  @Rule
+  public DockerComposeRule docker = DockerComposeRule.builder()
+  .file(DOCKER_COMPOSE_PATH.getPath())
+  .build();
+
+  private ClientCache cache;
+
+  private String trustStorePath;
+
+  @Before
+  public void before() throws IOException, InterruptedException {
+trustStorePath =
+createTempFileFromResource(ClientSNIDropProxyAcceptanceTest.class,
+"geode-config/truststore.jks")
+.getAbsolutePath();
+docker.exec(options("-T"), "geode",
+arguments("gfsh", "run", "--file=/geode/scripts/geode-starter.gfsh"));
+
+  }
+
+  @Test
+  public void performSimpleOperationsDropSNIProxy()
+  throws IOException,
+  InterruptedException {
+final Region region = getRegion();
+
+region.put("Roy Hobbs", 9);
+assertThat(region.get("Roy Hobbs")).isEqualTo(9);
+
+docker.containers()
+.container("haproxy")
+.stop();
+
+assertThatThrownBy(() -> region.get("Roy Hobbs"))
+.isInstanceOf(NoAvailableLocatorsException.class)
+.hasMessageContaining("Unable to connect to any locators in the list");
+
+docker.containers()
+.container("haproxy")
+.start();
+
+assertThat(region.get("Roy Hobbs")).isEqualTo(9);
+
+region.put("Bennie Rodriquez", 30);
+assertThat(region.get("Bennie Rodriquez")).isEqualTo(30);
+
+region.put("Jake Taylor", 7);
+region.put("Crash Davis", 8);
+
+region.put("Ricky Vaughn", 99);
+region.put("Ebbie Calvin LaLoosh", 37);
+
+  }
+
+  public Region getRegion() {
+Properties gemFireProps = new Properties();
+gemFireProps.setProperty(SSL_ENABLED_COMPONENTS, "all");
+gemFireProps.setProperty(SSL_KEYSTORE_TYPE, "jks");
+gemFireProps.setProperty(SSL_REQUIRE_AUTHENTICATION, "false")

[GitHub] [geode] Bill commented on a change in pull request #4959: GEODE-7852: Create test for running cache operations with a dropped …

[GitBox]

Bill commented on a change in pull request #4959:
URL: https://github.com/apache/geode/pull/4959#discussion_r412386248



##
File path: 
geode-assembly/src/acceptanceTest/resources/org/apache/geode/client/sni/docker-compose.yml
##
@@ -33,7 +33,7 @@ services:
 container_name: 'haproxy'
 image: 'haproxy:2.1'
 ports:
-  - "15443"
+  - "15443:15443"

Review comment:
   this has been addressed now @upthewaterspout 





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] Bill commented on a change in pull request #4959: GEODE-7852: Create test for running cache operations with a dropped …

[GitBox]

Bill commented on a change in pull request #4959:
URL: https://github.com/apache/geode/pull/4959#discussion_r412386787



##
File path: 
geode-assembly/src/acceptanceTest/java/org/apache/geode/client/sni/ClientSNIDropProxyAcceptanceTest.java
##
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.client.sni;
+
+import static 
com.palantir.docker.compose.execution.DockerComposeExecArgument.arguments;
+import static 
com.palantir.docker.compose.execution.DockerComposeExecOption.options;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_ENABLED_COMPONENTS;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_ENDPOINT_IDENTIFICATION_ENABLED;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE_TYPE;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_REQUIRE_AUTHENTICATION;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE_PASSWORD;
+import static 
org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.IOException;
+import java.net.URL;
+import java.util.Properties;
+
+import com.palantir.docker.compose.DockerComposeRule;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestRule;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.client.ClientCacheFactory;
+import org.apache.geode.cache.client.ClientRegionShortcut;
+import org.apache.geode.cache.client.NoAvailableLocatorsException;
+import org.apache.geode.cache.client.proxy.ProxySocketFactories;
+import org.apache.geode.test.junit.rules.IgnoreOnWindowsRule;
+
+public class ClientSNIDropProxyAcceptanceTest {
+
+  private static final URL DOCKER_COMPOSE_PATH =
+  ClientSNIDropProxyAcceptanceTest.class.getResource("docker-compose.yml");
+
+  // Docker compose does not work on windows in CI. Ignore this test on windows
+  // Using a RuleChain to make sure we ignore the test before the rule comes 
into play
+  @ClassRule
+  public static TestRule ignoreOnWindowsRule = new IgnoreOnWindowsRule();
+
+  @Rule
+  public DockerComposeRule docker = DockerComposeRule.builder()
+  .file(DOCKER_COMPOSE_PATH.getPath())
+  .build();
+
+  private ClientCache cache;
+
+  private String trustStorePath;
+
+  @Before
+  public void before() throws IOException, InterruptedException {
+trustStorePath =
+createTempFileFromResource(ClientSNIDropProxyAcceptanceTest.class,
+"geode-config/truststore.jks")
+.getAbsolutePath();
+docker.exec(options("-T"), "geode",
+arguments("gfsh", "run", "--file=/geode/scripts/geode-starter.gfsh"));
+
+  }
+
+  @Test
+  public void performSimpleOperationsDropSNIProxy()
+  throws IOException,
+  InterruptedException {
+final Region region = getRegion();
+
+region.put("Roy Hobbs", 9);
+assertThat(region.get("Roy Hobbs")).isEqualTo(9);
+
+docker.containers()
+.container("haproxy")
+.stop();
+
+assertThatThrownBy(() -> region.get("Roy Hobbs"))
+.isInstanceOf(NoAvailableLocatorsException.class)
+.hasMessageContaining("Unable to connect to any locators in the list");
+
+docker.containers()
+.container("haproxy")
+.start();
+

Review comment:
   we elected not to go this way (socket factory)





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] Bill commented on a change in pull request #4959: GEODE-7852: Create test for running cache operations with a dropped …

[GitBox]

Bill commented on a change in pull request #4959:
URL: https://github.com/apache/geode/pull/4959#discussion_r412391692



##
File path: 
geode-assembly/src/acceptanceTest/java/org/apache/geode/client/sni/ClientSNIDropProxyAcceptanceTest.java
##
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.client.sni;
+
+import static 
com.palantir.docker.compose.execution.DockerComposeExecArgument.arguments;
+import static 
com.palantir.docker.compose.execution.DockerComposeExecOption.options;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_ENABLED_COMPONENTS;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_ENDPOINT_IDENTIFICATION_ENABLED;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE_TYPE;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_REQUIRE_AUTHENTICATION;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE;
+import static 
org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE_PASSWORD;
+import static 
org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.IOException;
+import java.net.URL;
+import java.util.Properties;
+
+import com.palantir.docker.compose.DockerComposeRule;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestRule;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.client.ClientCacheFactory;
+import org.apache.geode.cache.client.ClientRegionShortcut;
+import org.apache.geode.cache.client.NoAvailableLocatorsException;
+import org.apache.geode.cache.client.proxy.ProxySocketFactories;
+import org.apache.geode.test.junit.rules.IgnoreOnWindowsRule;
+
+public class ClientSNIDropProxyAcceptanceTest {
+
+  private static final URL DOCKER_COMPOSE_PATH =
+  ClientSNIDropProxyAcceptanceTest.class.getResource("docker-compose.yml");
+
+  // Docker compose does not work on windows in CI. Ignore this test on windows
+  // Using a RuleChain to make sure we ignore the test before the rule comes 
into play
+  @ClassRule
+  public static TestRule ignoreOnWindowsRule = new IgnoreOnWindowsRule();

Review comment:
   agreed! it's coming…





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] DonalEvans commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

DonalEvans commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412366325



##
File path: 
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/ClearCommand.java
##
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static 
org.apache.geode.management.internal.cli.commands.DataCommandsUtils.callFunctionForRegion;
+
+import java.util.Set;
+
+import org.apache.commons.collections.CollectionUtils;
+import org.springframework.shell.core.annotation.CliCommand;
+import org.springframework.shell.core.annotation.CliOption;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.Region;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.cli.CliMetaData;
+import org.apache.geode.management.cli.ConverterHint;
+import org.apache.geode.management.cli.GfshCommand;
+import org.apache.geode.management.internal.cli.domain.DataCommandRequest;
+import org.apache.geode.management.internal.cli.domain.DataCommandResult;
+import org.apache.geode.management.internal.cli.functions.DataCommandFunction;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.management.internal.i18n.CliStrings;
+import org.apache.geode.security.ResourcePermission.Operation;
+import org.apache.geode.security.ResourcePermission.Resource;
+
+public class ClearCommand extends GfshCommand {
+  public static final String REGION_NOT_FOUND = "Region <%s> not found in any 
of the members";
+
+  @CliMetaData(relatedTopic = {CliStrings.TOPIC_GEODE_DATA, 
CliStrings.TOPIC_GEODE_REGION})
+  @CliCommand(value = {CliStrings.CLEAR}, help = CliStrings.CLEAR_HELP)
+  public ResultModel clear(
+  @CliOption(key = {CliStrings.CLEAR_REGION_NAME}, mandatory = true,
+  help = CliStrings.CLEAR_REGION_NAME_HELP,
+  optionContext = ConverterHint.REGION_PATH) String regionPath) {
+
+Cache cache = getCache();
+
+authorize(Resource.DATA, Operation.WRITE, regionPath);
+
+
+Region region = cache.getRegion(regionPath);
+DataCommandFunction clearfn = new DataCommandFunction();
+DataCommandResult dataResult;
+if (region == null) {
+  Set memberList = findAnyMembersForRegion(regionPath);
+
+  if (CollectionUtils.isEmpty(memberList)) {
+return new ResultModel().createError(String.format(REGION_NOT_FOUND, 
regionPath));
+  }
+
+  DataCommandRequest request = new DataCommandRequest();
+  request.setCommand(CliStrings.REMOVE);
+  request.setRemoveAllKeys("ALL");
+  request.setRegionName(regionPath);
+  dataResult = callFunctionForRegion(request, clearfn, memberList);
+} else {
+  dataResult = clearfn.remove(null, null, regionPath, "ALL",

Review comment:
   Shouldn't the clear command be using the clear function instead of 
remove?

##
File path: 
geode-core/src/main/java/org/apache/geode/management/internal/i18n/CliStrings.java
##
@@ -1914,9 +1921,10 @@
   public static final String REMOVE__MSG__KEY_EMPTY = "Key is Null";
   public static final String REMOVE__MSG__REGION_NOT_FOUND = "Region <{0}> Not 
Found";
   public static final String REMOVE__MSG__KEY_NOT_FOUND_REGION = "Key is not 
present in the region";
-  public static final String REMOVE__MSG__CLEARED_ALL_CLEARS = "Cleared all 
keys in the region";
-  public static final String 
REMOVE__MSG__CLEARALL_NOT_SUPPORTED_FOR_PARTITIONREGION =
-  "Option --" + REMOVE__ALL + " is not supported on partitioned region";
+  public static final String REMOVE__MSG__CLEARED_ALL_KEYS = "Cleared all keys 
in the region";

Review comment:
   It might be best to rename and move this constant to reflect that going 
forward, only the clear command should be using the message, since remove --all 
is now deprecated.

##
File path: 
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
##
@@ -313,22 +313,21 @@ public DataCommandResult remove(String key, String 
keyClass, String regionName,

[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #4909: GEODE-7953: Restore Redundancy Internal API

[GitBox]

pivotal-jbarrett commented on a change in pull request #4909:
URL: https://github.com/apache/geode/pull/4909#discussion_r412453438



##
File path: 
geode-core/src/main/java/org/apache/geode/cache/control/RestoreRedundancyResults.java
##
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.cache.control;
+
+import java.util.Map;
+
+import org.apache.geode.cache.partition.PartitionRebalanceInfo;
+import org.apache.geode.internal.cache.control.RegionRedundancyStatus;
+import 
org.apache.geode.internal.cache.partitioned.PartitionedRegionRebalanceOp;
+
+/**
+ * A class to collect the results of restore redundancy operations for one or 
more regions and
+ * determine the success of failure of the operation.
+ */
+public interface RestoreRedundancyResults {
+
+  /**
+   * {@link #SUCCESS} is defined as every included region having fully 
satisfied redundancy.
+   * {@link #FAILURE} is defined as at least one region that is configured to 
have redundant copies
+   * having fewer than its configured number of redundant copies.
+   * {@link #ERROR} is for cases when the restore redundancy operation was 
unable to begin or threw
+   * an exception.
+   */
+  enum Status {
+SUCCESS,
+FAILURE,
+ERROR
+  }
+
+  /**
+   * Adds the contents of another {@link RestoreRedundancyResults} object to 
this one, including
+   * both {@link RegionRedundancyStatus} objects and information on the number 
of primaries
+   * reassigned and the time taken to reassign them.
+   *
+   * @param results a {@link RestoreRedundancyResults} object whose contents 
will be added to this
+   *one.
+   */
+  void addRegionResults(RestoreRedundancyResults results);
+
+  /**
+   * Adds information regarding the number of primaries reassigned and the 
time taken to reassign
+   * them during a restore redundancy operation.
+   *
+   * @param details a {@link PartitionRebalanceInfo} generated by a
+   *{@link PartitionedRegionRebalanceOp} operation.

Review comment:
   If we had a proper API module these would be a compile time failure for 
any tests. 
   @upthewaterspout Seems like there should be a test in the static analyzer or 
other that should reject APIs like this. 

##
File path: 
geode-core/src/main/java/org/apache/geode/cache/control/RestoreRedundancyResults.java
##
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.cache.control;
+
+import java.util.Map;
+
+import org.apache.geode.cache.partition.PartitionRebalanceInfo;
+import org.apache.geode.internal.cache.control.RegionRedundancyStatus;
+import 
org.apache.geode.internal.cache.partitioned.PartitionedRegionRebalanceOp;
+
+/**
+ * A class to collect the results of restore redundancy operations for one or 
more regions and
+ * determine the success of failure of the operation.
+ */
+public interface RestoreRedundancyResults {
+
+  /**
+   * {@link #SUCCESS} is defined as every included region having fully 
satisfied redundancy.
+   * {@link #FAILURE} is defined as at least one region that is configured to 
have redundant copies
+   * having fewer than its configured number of redundant copies.
+   * {@link #ERROR} is for cases when the restore redundancy operation was 
unable to begin or threw
+   * an exception.
+   */
+  enum Status {
+SUCCESS,
+FAILURE,
+ERROR
+  }
+
+  /**
+   * Adds the contents of another {@link RestoreRedundancyR

[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #4909: GEODE-7953: Restore Redundancy Internal API

[GitBox]

pivotal-jbarrett commented on a change in pull request #4909:
URL: https://github.com/apache/geode/pull/4909#discussion_r412469554



##
File path: 
geode-core/src/main/java/org/apache/geode/cache/control/RestoreRedundancyResults.java
##
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.cache.control;
+
+import java.util.Map;
+
+import org.apache.geode.cache.partition.PartitionRebalanceInfo;
+import org.apache.geode.internal.cache.control.RegionRedundancyStatus;
+import 
org.apache.geode.internal.cache.partitioned.PartitionedRegionRebalanceOp;
+
+/**
+ * A class to collect the results of restore redundancy operations for one or 
more regions and
+ * determine the success of failure of the operation.
+ */
+public interface RestoreRedundancyResults {
+
+  /**
+   * {@link #SUCCESS} is defined as every included region having fully 
satisfied redundancy.
+   * {@link #FAILURE} is defined as at least one region that is configured to 
have redundant copies
+   * having fewer than its configured number of redundant copies.
+   * {@link #ERROR} is for cases when the restore redundancy operation was 
unable to begin or threw
+   * an exception.
+   */
+  enum Status {
+SUCCESS,
+FAILURE,
+ERROR
+  }
+
+  /**
+   * Adds the contents of another {@link RestoreRedundancyResults} object to 
this one, including
+   * both {@link RegionRedundancyStatus} objects and information on the number 
of primaries
+   * reassigned and the time taken to reassign them.
+   *
+   * @param results a {@link RestoreRedundancyResults} object whose contents 
will be added to this
+   *one.
+   */
+  void addRegionResults(RestoreRedundancyResults results);
+
+  /**
+   * Adds information regarding the number of primaries reassigned and the 
time taken to reassign
+   * them during a restore redundancy operation.
+   *
+   * @param details a {@link PartitionRebalanceInfo} generated by a
+   *{@link PartitionedRegionRebalanceOp} operation.

Review comment:
   Perhaps this is the opportunity to start a `geode-control-api.jar` that 
contains all these interfaces. We have to start somewhere and this seems like a 
good small start.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

BenjaminPerryRoss commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412471778



##
File path: 
geode-core/src/main/java/org/apache/geode/management/internal/i18n/CliStrings.java
##
@@ -1915,6 +1922,9 @@
   public static final String REMOVE__MSG__REGION_NOT_FOUND = "Region <{0}> Not 
Found";
   public static final String REMOVE__MSG__KEY_NOT_FOUND_REGION = "Key is not 
present in the region";
   public static final String REMOVE__MSG__CLEARED_ALL_CLEARS = "Cleared all 
keys in the region";
+  public static final String REMOVE__MSG__CLEARALL_DEPRECATION_WARNING =
+  "Warning: The --all option for the 'remove' command is deprecated. 
Please"
+  + " use the 'clear region' command instead.";
   public static final String 
REMOVE__MSG__CLEARALL_NOT_SUPPORTED_FOR_PARTITIONREGION =

Review comment:
   I've removed this message.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

BenjaminPerryRoss commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412472330



##
File path: 
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
##
@@ -314,17 +314,13 @@ public DataCommandResult remove(String key, String 
keyClass, String regionName,
 }
   } else {
 DataPolicy policy = region.getAttributes().getDataPolicy();
-if (!policy.withPartitioning()) {
-  region.clear();
-  if (logger.isDebugEnabled()) {
-logger.debug("Cleared all keys in the region - {}", regionName);
-  }
-  return DataCommandResult.createRemoveInfoResult(key, null, null,
-  CliStrings.format(CliStrings.REMOVE__MSG__CLEARED_ALL_CLEARS, 
regionName), true);
-} else {
-  return DataCommandResult.createRemoveInfoResult(key, null, null,
-  
CliStrings.REMOVE__MSG__CLEARALL_NOT_SUPPORTED_FOR_PARTITIONREGION, false);
+region.clear();

Review comment:
   I agree this could be confusing. I've added clear() as a helper method 
invoked by remove() when called with the 'ALL' flag to help make the 
relationship more visible.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

BenjaminPerryRoss commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412473139



##
File path: 
geode-core/src/main/java/org/apache/geode/management/internal/i18n/CliStrings.java
##
@@ -816,6 +816,13 @@
   public static final String CLEAR_DEFINED_INDEX__SUCCESS__MSG =
   "Index definitions successfully cleared";
 
+  /* clear region */
+  public static final String CLEAR_REGION = "clear region";

Review comment:
   I initially tried changing this to 'clear' as described in the ticket 
but this caused a conflict with the 'clear defined index' command. I've 
discussed it with Charlie and we decided that 'clear region' is appropriate 
(and it works around that issue) so I've updated the JIRA ticket/tracker story 
to reflect this.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

BenjaminPerryRoss commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412473711



##
File path: 
geode-gfsh/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ClearCommandDUnitTest.java
##
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.management.internal.cli.commands;
+
+import static 
org.apache.geode.management.internal.cli.commands.RemoveCommand.REGION_NOT_FOUND;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.Region;
+import org.apache.geode.management.internal.i18n.CliStrings;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.junit.rules.GfshCommandRule;
+import org.apache.geode.test.junit.rules.VMProvider;
+
+
+public class ClearCommandDUnitTest {
+  private static final String commandString = CliStrings.CLEAR_REGION;
+  private static final String REPLICATE_REGION_NAME = "replicateRegion";
+  private static final String PARTITIONED_REGION_NAME = "partitionedRegion";
+  private static final String EMPTY_STRING = "";
+
+  @Rule
+  public ClusterStartupRule clusterStartupRule = new ClusterStartupRule();
+
+  @Rule
+  public GfshCommandRule gfsh = new GfshCommandRule();
+
+  private MemberVM locator;
+  private MemberVM server1;
+  private MemberVM server2;
+
+  @Before
+  public void setup() throws Exception {
+locator = clusterStartupRule.startLocatorVM(0);
+server1 = clusterStartupRule.startServerVM(1, locator.getPort());
+server2 = clusterStartupRule.startServerVM(2, locator.getPort());
+
+gfsh.connectAndVerify(locator);
+gfsh.executeAndAssertThat("create region --name=" + REPLICATE_REGION_NAME 
+ " --type=REPLICATE")
+.statusIsSuccess();
+gfsh.executeAndAssertThat(
+"create region --name=" + PARTITIONED_REGION_NAME + " 
--type=PARTITION").statusIsSuccess();
+
+locator.waitUntilRegionIsReadyOnExactlyThisManyServers("/" + 
REPLICATE_REGION_NAME, 2);
+locator.waitUntilRegionIsReadyOnExactlyThisManyServers("/" + 
PARTITIONED_REGION_NAME, 2);
+
+VMProvider.invokeInEveryMember(ClearCommandDUnitTest::populateTestRegions, 
server1, server2);
+  }
+
+  private static void populateTestRegions() {
+Cache cache = CacheFactory.getAnyInstance();
+
+Region replicateRegion = 
cache.getRegion(REPLICATE_REGION_NAME);
+replicateRegion.put(EMPTY_STRING, "valueForEmptyKey");
+replicateRegion.put("key1", "value1");
+replicateRegion.put("key2", "value2");
+
+Region partitionedRegion = 
cache.getRegion(PARTITIONED_REGION_NAME);
+partitionedRegion.put("key1", "value1");
+partitionedRegion.put("key2", "value2");
+  }
+
+  @Test
+  public void clearInvalidRegion() {
+String command = commandString + " --region=NotAValidRegion";
+
+gfsh.executeAndAssertThat(command).statusIsError()
+.containsOutput(String.format(REGION_NOT_FOUND, "/NotAValidRegion"));
+  }
+
+  @Test
+  public void clearReplicateRegion() {
+String command = commandString + " --region=" + REPLICATE_REGION_NAME;
+
+gfsh.executeAndAssertThat("list regions").statusIsSuccess();

Review comment:
   I've gone ahead and removed this command/assertion since it's not really 
necessary. 





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

BenjaminPerryRoss commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412476686



##
File path: 
geode-core/src/main/java/org/apache/geode/management/internal/i18n/CliStrings.java
##
@@ -1914,9 +1921,10 @@
   public static final String REMOVE__MSG__KEY_EMPTY = "Key is Null";
   public static final String REMOVE__MSG__REGION_NOT_FOUND = "Region <{0}> Not 
Found";
   public static final String REMOVE__MSG__KEY_NOT_FOUND_REGION = "Key is not 
present in the region";
-  public static final String REMOVE__MSG__CLEARED_ALL_CLEARS = "Cleared all 
keys in the region";
-  public static final String 
REMOVE__MSG__CLEARALL_NOT_SUPPORTED_FOR_PARTITIONREGION =
-  "Option --" + REMOVE__ALL + " is not supported on partitioned region";
+  public static final String REMOVE__MSG__CLEARED_ALL_KEYS = "Cleared all keys 
in the region";

Review comment:
   I agree, this could easily be corrected here.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

BenjaminPerryRoss commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412480534



##
File path: 
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/ClearCommand.java
##
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static 
org.apache.geode.management.internal.cli.commands.DataCommandsUtils.callFunctionForRegion;
+
+import java.util.Set;
+
+import org.apache.commons.collections.CollectionUtils;
+import org.springframework.shell.core.annotation.CliCommand;
+import org.springframework.shell.core.annotation.CliOption;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.Region;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.cli.CliMetaData;
+import org.apache.geode.management.cli.ConverterHint;
+import org.apache.geode.management.cli.GfshCommand;
+import org.apache.geode.management.internal.cli.domain.DataCommandRequest;
+import org.apache.geode.management.internal.cli.domain.DataCommandResult;
+import org.apache.geode.management.internal.cli.functions.DataCommandFunction;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.management.internal.i18n.CliStrings;
+import org.apache.geode.security.ResourcePermission.Operation;
+import org.apache.geode.security.ResourcePermission.Resource;
+
+public class ClearCommand extends GfshCommand {
+  public static final String REGION_NOT_FOUND = "Region <%s> not found in any 
of the members";
+
+  @CliMetaData(relatedTopic = {CliStrings.TOPIC_GEODE_DATA, 
CliStrings.TOPIC_GEODE_REGION})
+  @CliCommand(value = {CliStrings.CLEAR}, help = CliStrings.CLEAR_HELP)
+  public ResultModel clear(
+  @CliOption(key = {CliStrings.CLEAR_REGION_NAME}, mandatory = true,
+  help = CliStrings.CLEAR_REGION_NAME_HELP,
+  optionContext = ConverterHint.REGION_PATH) String regionPath) {
+
+Cache cache = getCache();
+
+authorize(Resource.DATA, Operation.WRITE, regionPath);
+
+
+Region region = cache.getRegion(regionPath);
+DataCommandFunction clearfn = new DataCommandFunction();
+DataCommandResult dataResult;
+if (region == null) {
+  Set memberList = findAnyMembersForRegion(regionPath);
+
+  if (CollectionUtils.isEmpty(memberList)) {
+return new ResultModel().createError(String.format(REGION_NOT_FOUND, 
regionPath));
+  }
+
+  DataCommandRequest request = new DataCommandRequest();
+  request.setCommand(CliStrings.REMOVE);
+  request.setRemoveAllKeys("ALL");
+  request.setRegionName(regionPath);
+  dataResult = callFunctionForRegion(request, clearfn, memberList);
+} else {
+  dataResult = clearfn.remove(null, null, regionPath, "ALL",

Review comment:
   The remove function already has logic for 'clearing' a region (since you 
can remove --all) so a clear is just a remove with the all flag set. Anil and I 
discussed this as well and I refactored the remove all code into a 'clear()' 
method on the function class so it was more obvious where the code for clear 
was being performed but there were challenges in implementing a new clear 
function and it would also have involved duplicate code.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

BenjaminPerryRoss commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412487003



##
File path: 
geode-gfsh/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ClearCommandDUnitTest.java
##
@@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.management.internal.cli.commands;
+
+import static 
org.apache.geode.management.internal.cli.commands.RemoveCommand.REGION_NOT_FOUND;
+import static 
org.apache.geode.management.internal.i18n.CliStrings.REMOVE__MSG__CLEARED_ALL_KEYS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.Region;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.management.internal.i18n.CliStrings;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.junit.rules.GfshCommandRule;
+import org.apache.geode.test.junit.rules.VMProvider;
+
+
+public class ClearCommandDUnitTest {
+  private static final String REPLICATE_REGION_NAME = "replicateRegion";
+  private static final String PARTITIONED_REGION_NAME = "partitionedRegion";
+  private static final String EMPTY_STRING = "";
+  private static final int NUM_ENTRIES = 200;
+
+  @Rule
+  public ClusterStartupRule clusterStartupRule = new ClusterStartupRule();
+
+  @Rule
+  public GfshCommandRule gfsh = new GfshCommandRule();
+
+  private MemberVM locator;
+  private MemberVM server1;
+  private MemberVM server2;
+
+  @Before
+  public void setup() throws Exception {
+locator = clusterStartupRule.startLocatorVM(0);
+server1 = clusterStartupRule.startServerVM(1, locator.getPort());
+server2 = clusterStartupRule.startServerVM(2, locator.getPort());
+
+gfsh.connectAndVerify(locator);
+gfsh.executeAndAssertThat("create region --name=" + REPLICATE_REGION_NAME 
+ " --type=REPLICATE")
+.statusIsSuccess();
+gfsh.executeAndAssertThat(
+"create region --name=" + PARTITIONED_REGION_NAME + " 
--type=PARTITION").statusIsSuccess();
+
+locator.waitUntilRegionIsReadyOnExactlyThisManyServers("/" + 
REPLICATE_REGION_NAME, 2);
+locator.waitUntilRegionIsReadyOnExactlyThisManyServers("/" + 
PARTITIONED_REGION_NAME, 2);
+
+VMProvider.invokeInEveryMember(ClearCommandDUnitTest::populateTestRegions, 
server1, server2);
+  }
+
+  private static void populateTestRegions() {
+Cache cache = CacheFactory.getAnyInstance();
+
+Region replicateRegion = 
cache.getRegion(REPLICATE_REGION_NAME);
+replicateRegion.put(EMPTY_STRING, "valueForEmptyKey");

Review comment:
   I was following the pattern set up by the Remove command (since they're 
very similar) for a lot of these choices and this is the pattern that was used 
in the DUnit tests for that command. With the value specifically I'm not sure 
it matters whether it's empty or not but it's easy to change if we think 
there's value.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

BenjaminPerryRoss commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412492480



##
File path: 
geode-gfsh/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ClearCommandDUnitTest.java
##
@@ -70,43 +72,49 @@ private static void populateTestRegions() {
 
 Region replicateRegion = 
cache.getRegion(REPLICATE_REGION_NAME);
 replicateRegion.put(EMPTY_STRING, "valueForEmptyKey");
-replicateRegion.put("key1", "value1");
-replicateRegion.put("key2", "value2");
+for(int i = 0; i < NUM_ENTRIES; i++) {
+  replicateRegion.put("key" + i, "value" + i);
+}
 
 Region partitionedRegion = 
cache.getRegion(PARTITIONED_REGION_NAME);
-partitionedRegion.put("key1", "value1");
-partitionedRegion.put("key2", "value2");
+replicateRegion.put(EMPTY_STRING, "valueForEmptyKey");
+for(int i = 0; i < NUM_ENTRIES; i++) {
+  partitionedRegion.put("key" + i, "value" + i);
+}
   }
 
   @Test
-  public void clearInvalidRegion() {
-String command = commandString + " --region=NotAValidRegion";
-
+  public void clearFailsWhenRegionIsNotFound() {
+String invalidRegionName = "NotAValidRegion";
+String command = new CommandStringBuilder(CliStrings.CLEAR)
+.addOption(CliStrings.CLEAR_REGION_NAME, 
invalidRegionName).getCommandString();
 gfsh.executeAndAssertThat(command).statusIsError()
-.containsOutput(String.format(REGION_NOT_FOUND, "/NotAValidRegion"));
+.containsOutput(String.format(REGION_NOT_FOUND, "/" + 
invalidRegionName));
   }
 
   @Test
-  public void clearReplicateRegion() {
-String command = commandString + " --region=" + REPLICATE_REGION_NAME;
+  public void clearSucceedsWithValidReplicateRegion() {
+String command = new CommandStringBuilder(CliStrings.CLEAR)
+.addOption(CliStrings.CLEAR_REGION_NAME, 
REPLICATE_REGION_NAME).getCommandString();
 
-gfsh.executeAndAssertThat("list regions").statusIsSuccess();
+gfsh.executeAndAssertThat("list regions");
 gfsh.executeAndAssertThat(command).statusIsSuccess();
 
-assertThat(gfsh.getGfshOutput()).contains("Cleared all keys in the 
region");
+assertThat(gfsh.getGfshOutput()).contains(REMOVE__MSG__CLEARED_ALL_CLEARS);
 
 server1.invoke(() -> verifyAllKeysAreRemoved(REPLICATE_REGION_NAME));
 server2.invoke(() -> verifyAllKeysAreRemoved(REPLICATE_REGION_NAME));
   }
 
 
   @Test
-  public void clearPartitionedRegion() {
-String command = commandString + " --region=" + PARTITIONED_REGION_NAME;
+  public void clearSucceedsWithValidPartitionedRegion() {
+String command = new CommandStringBuilder(CliStrings.CLEAR)
+.addOption(CliStrings.CLEAR_REGION_NAME, 
PARTITIONED_REGION_NAME).getCommandString();
 
-gfsh.executeAndAssertThat(command).statusIsSuccess();
+gfsh.executeAndAssertThat(command);

Review comment:
   This was removed by mistake, it should be back in there now.

##
File path: 
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
##
@@ -276,7 +276,7 @@ public DataCommandResult remove(String key, String 
keyClass, String regionName,
 
 if (StringUtils.isEmpty(removeAllKeys) && (key == null)) {
   return DataCommandResult.createRemoveResult(null, null, null,
-  CliStrings.REMOVE__MSG__KEY_EMPTY, false);
+  "BR" + CliStrings.REMOVE__MSG__KEY_EMPTY, false);

Review comment:
   Done





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

BenjaminPerryRoss commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412492948



##
File path: 
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java
##
@@ -313,22 +313,21 @@ public DataCommandResult remove(String key, String 
keyClass, String regionName,
   CliStrings.REMOVE__MSG__KEY_NOT_FOUND_REGION, false);
 }
   } else {
-DataPolicy policy = region.getAttributes().getDataPolicy();
-if (!policy.withPartitioning()) {
-  region.clear();
-  if (logger.isDebugEnabled()) {
-logger.debug("Cleared all keys in the region - {}", regionName);
-  }
-  return DataCommandResult.createRemoveInfoResult(key, null, null,
-  CliStrings.format(CliStrings.REMOVE__MSG__CLEARED_ALL_CLEARS, 
regionName), true);
-} else {
-  return DataCommandResult.createRemoveInfoResult(key, null, null,
-  
CliStrings.REMOVE__MSG__CLEARALL_NOT_SUPPORTED_FOR_PARTITIONREGION, false);
-}
+return clear(region, regionName);
   }
 }
   }
 
+  public DataCommandResult clear(Region region, String regionName) {
+DataPolicy policy = region.getAttributes().getDataPolicy();

Review comment:
   I've removed it.

##
File path: 
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/ClearCommand.java
##
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static 
org.apache.geode.management.internal.cli.commands.DataCommandsUtils.callFunctionForRegion;
+
+import java.util.Set;
+
+import org.apache.commons.collections.CollectionUtils;
+import org.springframework.shell.core.annotation.CliCommand;
+import org.springframework.shell.core.annotation.CliOption;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.Region;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.cli.CliMetaData;
+import org.apache.geode.management.cli.ConverterHint;
+import org.apache.geode.management.cli.GfshCommand;
+import org.apache.geode.management.internal.cli.domain.DataCommandRequest;
+import org.apache.geode.management.internal.cli.domain.DataCommandResult;
+import org.apache.geode.management.internal.cli.functions.DataCommandFunction;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.management.internal.i18n.CliStrings;
+import org.apache.geode.security.ResourcePermission.Operation;
+import org.apache.geode.security.ResourcePermission.Resource;
+
+public class ClearCommand extends GfshCommand {
+  public static final String REGION_NOT_FOUND = "Region <%s> not found in any 
of the members";
+
+  @CliMetaData(relatedTopic = {CliStrings.TOPIC_GEODE_DATA, 
CliStrings.TOPIC_GEODE_REGION})
+  @CliCommand(value = {CliStrings.CLEAR}, help = CliStrings.CLEAR_HELP)
+  public ResultModel clear(
+  @CliOption(key = {CliStrings.CLEAR_REGION_NAME}, mandatory = true,
+  help = CliStrings.CLEAR_REGION_NAME_HELP,
+  optionContext = ConverterHint.REGION_PATH) String regionPath) {
+
+Cache cache = getCache();
+
+authorize(Resource.DATA, Operation.WRITE, regionPath);
+
+
+Region region = cache.getRegion(regionPath);
+DataCommandFunction clearfn = new DataCommandFunction();
+DataCommandResult dataResult;
+if (region == null) {
+  Set memberList = findAnyMembersForRegion(regionPath);
+
+  if (CollectionUtils.isEmpty(memberList)) {
+return new ResultModel().createError(String.format(REGION_NOT_FOUND, 
regionPath));

Review comment:
   I've removed this.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

BenjaminPerryRoss commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412493249



##
File path: 
geode-gfsh/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ClearCommandDUnitTest.java
##
@@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.management.internal.cli.commands;
+
+import static 
org.apache.geode.management.internal.cli.commands.RemoveCommand.REGION_NOT_FOUND;
+import static 
org.apache.geode.management.internal.i18n.CliStrings.REMOVE__MSG__CLEARED_ALL_KEYS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.Region;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.management.internal.i18n.CliStrings;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.junit.rules.GfshCommandRule;
+import org.apache.geode.test.junit.rules.VMProvider;
+
+
+public class ClearCommandDUnitTest {
+  private static final String REPLICATE_REGION_NAME = "replicateRegion";
+  private static final String PARTITIONED_REGION_NAME = "partitionedRegion";
+  private static final String EMPTY_STRING = "";
+  private static final int NUM_ENTRIES = 200;
+
+  @Rule
+  public ClusterStartupRule clusterStartupRule = new ClusterStartupRule();
+
+  @Rule
+  public GfshCommandRule gfsh = new GfshCommandRule();
+
+  private MemberVM locator;
+  private MemberVM server1;
+  private MemberVM server2;
+
+  @Before
+  public void setup() throws Exception {
+locator = clusterStartupRule.startLocatorVM(0);
+server1 = clusterStartupRule.startServerVM(1, locator.getPort());
+server2 = clusterStartupRule.startServerVM(2, locator.getPort());
+
+gfsh.connectAndVerify(locator);
+gfsh.executeAndAssertThat("create region --name=" + REPLICATE_REGION_NAME 
+ " --type=REPLICATE")
+.statusIsSuccess();
+gfsh.executeAndAssertThat(
+"create region --name=" + PARTITIONED_REGION_NAME + " 
--type=PARTITION").statusIsSuccess();
+
+locator.waitUntilRegionIsReadyOnExactlyThisManyServers("/" + 
REPLICATE_REGION_NAME, 2);
+locator.waitUntilRegionIsReadyOnExactlyThisManyServers("/" + 
PARTITIONED_REGION_NAME, 2);
+
+VMProvider.invokeInEveryMember(ClearCommandDUnitTest::populateTestRegions, 
server1, server2);
+  }
+
+  private static void populateTestRegions() {
+Cache cache = CacheFactory.getAnyInstance();
+
+Region replicateRegion = 
cache.getRegion(REPLICATE_REGION_NAME);
+replicateRegion.put(EMPTY_STRING, "valueForEmptyKey");
+for (int i = 0; i < NUM_ENTRIES; i++) {
+  replicateRegion.put("key" + i, "value" + i);
+}
+
+Region partitionedRegion = 
cache.getRegion(PARTITIONED_REGION_NAME);
+replicateRegion.put(EMPTY_STRING, "valueForEmptyKey");
+for (int i = 0; i < NUM_ENTRIES; i++) {
+  partitionedRegion.put("key" + i, "value" + i);
+}
+  }
+
+  @Test
+  public void clearFailsWhenRegionIsNotFound() {
+String invalidRegionName = "NotAValidRegion";
+String command = new CommandStringBuilder(CliStrings.CLEAR)
+.addOption(CliStrings.CLEAR_REGION_NAME, 
invalidRegionName).getCommandString();
+gfsh.executeAndAssertThat(command).statusIsError()
+.containsOutput(String.format(REGION_NOT_FOUND, "/" + 
invalidRegionName));
+  }
+
+  @Test
+  public void clearSucceedsWithValidReplicateRegion() {
+String command = new CommandStringBuilder(CliStrings.CLEAR)
+.addOption(CliStrings.CLEAR_REGION_NAME, 
REPLICATE_REGION_NAME).getCommandString();
+
+gfsh.executeAndAssertThat(command).statusIsSuccess();
+
+assertThat(gfsh.getGfshOutput()).contains(REMOVE__MSG__CLEARED_ALL_KEYS);
+
+server1.invoke(() -> verifyAllKeysAreRemoved(REPLICATE_REGION_NAME));
+server2.invoke(() -> verifyAllKeysAreRemoved(REPLICATE_REGION_NAME));
+  }
+
+
+  @Test
+  public void clearSucceedsWithValidPartitionedRegion() {
+String command

[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

BenjaminPerryRoss commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412493711



##
File path: 
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java
##
@@ -381,6 +397,12 @@ public ResultModel toResultModel() {
 }
 
 ResultModel result = new ResultModel();
+
+if (warningMessage != null && !warningMessage.isEmpty()) {
+  InfoResultModel info = result.addInfo(HEADER_INFO_SECTION);

Review comment:
   I'm reluctant to change the addInfo to setHeader because I want the 
structure of the output to match remove as closely as possible so I opted to 
change the name to WARNING_INFO_SECTION as this section isn't used in any other 
context.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

BenjaminPerryRoss commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412495880



##
File path: 
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/RemoveCommand.java
##
@@ -90,7 +90,14 @@ public ResultModel remove(
 }
 
 dataResult.setKeyClass(keyClass);
+ResultModel result = null;

Review comment:
   I've removed the redundant initialization.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] BenjaminPerryRoss commented on a change in pull request #4818: GEODE-7667: Add a 'clear' gfsh command for PR and RR clear

[GitBox]

BenjaminPerryRoss commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r412496115



##
File path: 
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java
##
@@ -240,6 +242,16 @@ public static DataCommandResult createRemoveResult(Object 
inputKey, Object value
 return result;
   }
 
+  public static DataCommandResult createClearResult(Throwable error, String 
errorString,

Review comment:
   It was left in by mistake after making and then reverting some changes 
based on PR feedback. I'll remove this.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] kirklund removed a comment on issue #4924: DRAFT: GEODE-7964: Upgrade Mockito to 3.3.3

[GitBox]

kirklund removed a comment on issue #4924:
URL: https://github.com/apache/geode/pull/4924#issuecomment-614181221







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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] kirklund removed a comment on issue #4924: DRAFT: GEODE-7964: Upgrade Mockito to 3.3.3

[GitBox]

kirklund removed a comment on issue #4924:
URL: https://github.com/apache/geode/pull/4924#issuecomment-617298733


   Another unrelated JMXMBeanReconnectDUnitTest failure.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] kirklund removed a comment on issue #4924: DRAFT: GEODE-7964: Upgrade Mockito to 3.3.3

[GitBox]

kirklund removed a comment on issue #4924:
URL: https://github.com/apache/geode/pull/4924#issuecomment-614809461


   The Concurrent Tests that don't use Mockito still seem to hang in 
StressNewTest (I removed `@Ignore` so they now need to pass StressNewTest). 



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] upthewaterspout commented on issue #4963: GEODE-7503: Block Cache.close() until everything is disconnected

[GitBox]

upthewaterspout commented on issue #4963:
URL: https://github.com/apache/geode/pull/4963#issuecomment-617442287


   The combination of a `ThreadLocal`, a `CountDownLatch`, and 
`synchronized()` seems a bit complicated.
   
   The whole close block is already inside of  
`synchronized(GemFireCacheImpl.class) {}`. It seems like the race condition is 
just with the early out at the beginning of close:
   
   ```
   if (isClosed()) {
 return;
   }
   ```
   
   Couldn't that just be changed to 
   
   ```
   if (!skipWait && isClosed()) {
 return;
   }
   ```
   
   To handle thread reentering cache.close, just don't add a skipWait check 
inside the synchronized block here:
   
   ```
   synchronized (GemFireCacheImpl.class) {
 // ALL CODE FOR CLOSE SHOULD NOW BE UNDER STATIC SYNCHRONIZATION OF 
GemFireCacheImpl.class
 // static synchronization is necessary due to static resources
 if (isClosed()) {
   
   > Remove the below check
   if (!skipAwait...)
   ```
   
   Any code that gets into the synchronized block is guaranteed that either (a) 
the cache has not been closed by another thread (b) the cache is completely 
closed already or (c) the thread is reentering cache close, in which case it 
should early out.
   
   There is probably some wrinkle that I'm missing here...



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] gesterzhou commented on a change in pull request #4970: GEODE-7676: Add PR clear with expiration tests

[GitBox]

gesterzhou commented on a change in pull request #4970:
URL: https://github.com/apache/geode/pull/4970#discussion_r412539205



##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithExpirationDUnitTest.java
##
@@ -0,0 +1,537 @@
+/*

Review comment:
   The test needs 30 minutes to finish all combination. And many tests took 
more than 30 seconds each. 
   
   Many combinations are unnecessary, for example we want to see the expiration 
tasks are cancelled, we don't care what expiration. Only when we want to see an 
expiration to be triggered, then we need some (not all) expiration types. In my 
opinion, we can hard code to use DESTROY expiration type only in this test.
   
   We don't have to verify clear from accessor or server. Some other tests have 
verified that. You can just use server to do clear. 
   
   Region types can also be reduced to a few. We have other tests  to verify 
that all the combination of region type can do clear successfully. We only need 
to verify the expiration tasks are cleared in this test.
   





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] DonalEvans commented on a change in pull request #4909: GEODE-7953: Restore Redundancy Internal API

[GitBox]

DonalEvans commented on a change in pull request #4909:
URL: https://github.com/apache/geode/pull/4909#discussion_r412566753



##
File path: 
geode-core/src/main/java/org/apache/geode/cache/control/RestoreRedundancyResults.java
##
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.cache.control;
+
+import java.util.Map;
+
+import org.apache.geode.cache.partition.PartitionRebalanceInfo;
+import org.apache.geode.internal.cache.control.RegionRedundancyStatus;
+import 
org.apache.geode.internal.cache.partitioned.PartitionedRegionRebalanceOp;
+
+/**
+ * A class to collect the results of restore redundancy operations for one or 
more regions and
+ * determine the success of failure of the operation.
+ */
+public interface RestoreRedundancyResults {
+
+  /**
+   * {@link #SUCCESS} is defined as every included region having fully 
satisfied redundancy.
+   * {@link #FAILURE} is defined as at least one region that is configured to 
have redundant copies
+   * having fewer than its configured number of redundant copies.
+   * {@link #ERROR} is for cases when the restore redundancy operation was 
unable to begin or threw
+   * an exception.
+   */
+  enum Status {
+SUCCESS,
+FAILURE,
+ERROR
+  }
+
+  /**
+   * Adds the contents of another {@link RestoreRedundancyResults} object to 
this one, including
+   * both {@link RegionRedundancyStatus} objects and information on the number 
of primaries
+   * reassigned and the time taken to reassign them.
+   *
+   * @param results a {@link RestoreRedundancyResults} object whose contents 
will be added to this
+   *one.
+   */
+  void addRegionResults(RestoreRedundancyResults results);
+
+  /**
+   * Adds information regarding the number of primaries reassigned and the 
time taken to reassign
+   * them during a restore redundancy operation.
+   *
+   * @param details a {@link PartitionRebalanceInfo} generated by a
+   *{@link PartitionedRegionRebalanceOp} operation.
+   */
+  void addPrimaryReassignmentDetails(PartitionRebalanceInfo details);
+
+  /**
+   * Adds one {@link RegionRedundancyStatus} to the result set.
+   *
+   * @param regionResult The {@link RegionRedundancyStatus} to be added to the 
result set.
+   */
+  void addRegionResult(RegionRedundancyStatus regionResult);

Review comment:
   Good call. I've removed the mutators from the interface and moved 
`RegionRedundancyStatus` to no longer be internal.

##
File path: 
geode-core/src/main/java/org/apache/geode/internal/cache/control/RegionRedundancyStatus.java
##
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the 
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software 
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
KIND, either express
+ * or implied. See the License for the specific language governing permissions 
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.control;
+
+import java.io.Serializable;
+
+import org.apache.geode.internal.cache.PartitionedRegion;
+
+/**
+ * Used to calculate and store the redundancy status for a {@link 
PartitionedRegion}.
+ */
+public class RegionRedundancyStatus implements Serializable {

Review comment:
   Done.

##
File path: 
geode-core/src/main/java/org/apache/geode/internal/cache/control/RestoreRedundancyBuilderImpl.java
##
@@ -0,0 +1,177 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more 
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional 
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache 
License, Version 2.0 (the
+ * "Licen

[GitHub] [geode] DonalEvans commented on issue #4909: GEODE-7953: Restore Redundancy Internal API

[GitBox]

DonalEvans commented on issue #4909:
URL: https://github.com/apache/geode/pull/4909#issuecomment-617470775


   > it seems like there are several classes that are a tweaked copy of 
existing classes - RestoreRedundancyBuilderImpl, RestoreRedundancyDirector, 
RegionRedundancyStatus. Is there a way we could extend/generalize the existing 
classes rather than copy/paste/tweak?
   
   I've modified `CompositeDirector` slightly to avoid the need for the 
`RestoreRedundancyDirector` class, but as described in my comment above, doing 
the same for `RestoreRedundancyBuilderImpl` would be very difficult at this 
point. As for `RegionRedundancyStatus`; it's fundamentally very different from 
`RebalanceResults` in terms of the information it contains, since it represents 
a snapshot of the state of a region rather than the work done to get it to that 
state (unless I've missed something here and there is another class that 
already exists and is similar to `RegionRedundancyStatus`).



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] dschneider-pivotal opened a new pull request #4981: GEODE-7981: Redis default should be PARTITION_REDUNDANT

[GitBox]

dschneider-pivotal opened a new pull request #4981:
URL: https://github.com/apache/geode/pull/4981


   Fixed docs to describe the new default.
   Also if the system property it set to an unknown shortcut
   then it now defaults to PARTITION_REDUNDANT instead of PARTITION.
   The test now validates this case and now uses assertThat instead of assert.
   
   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 build` run cleanly?
   
   - [ ] Have you written or updated unit tests to verify your changes?
   
   - [ ] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)?
   
   ### Note:
   Please ensure that once the PR is submitted, check Concourse 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.
   



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] mhansonp opened a new pull request #4982: GEODE-7935: Awaiting for verification steps.

[GitBox]

mhansonp opened a new pull request #4982:
URL: https://github.com/apache/geode/pull/4982


   This test just needed some awaits added to allow the test to retry.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] dschneider-pivotal opened a new pull request #4983: GEODE-8010: message is now debug instead of info

[GitBox]

dschneider-pivotal opened a new pull request #4983:
URL: https://github.com/apache/geode/pull/4983


   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 build` run cleanly?
   
   - [ ] Have you written or updated unit tests to verify your changes?
   
   - [ ] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)?
   
   ### Note:
   Please ensure that once the PR is submitted, check Concourse 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.
   



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jujoramos commented on issue #4978: Fix for regression introduced by GEODE-7565

[GitBox]

jujoramos commented on issue #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-617642403


   @bschuchardt @alb3rtobr : I'm still analysing the issue and seeing whether I 
can reproduce it consistently, so far it's proving quite elusive as it only 
fails once or twice in around 100 runs of my test. Will keep you posted.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jujoramos commented on a change in pull request #4970: GEODE-7676: Add PR clear with expiration tests

[GitBox]

jujoramos commented on a change in pull request #4970:
URL: https://github.com/apache/geode/pull/4970#discussion_r412794182



##
File path: 
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithExpirationDUnitTest.java
##
@@ -0,0 +1,537 @@
+/*

Review comment:
   > The test needs 30 minutes to finish all combination. And many tests 
took more than 30 seconds each.
   
   This is not a bad thing per se, I wanted to test all possible combinations 
so it's expected for the distributed test to take some time to finish. I'll 
remove some combinations, though, so the overall time will be reduced.
   
   > Many combinations are unnecessary, for example we want to see the 
expiration tasks are cancelled, we don't care what expiration. Only when we 
want to see an expiration to be triggered, then we need some (not all) 
expiration types. In my opinion, we can hard code to use DESTROY expiration 
type only in this test.
   We don't have to verify clear from accessor or server. Some other tests have 
verified that. You can just use server to do clear.
   Region types can also be reduced to a few. We have other tests to verify 
that all the combination of region type can do clear successfully. We only need 
to verify the expiration tasks are cleared in this test.
   
   Having tests to verify several possible combinations is better than having 
no tests at all, specially for the region types as we might change the 
implementation class in the future... if we do, this test might be able to 
catch possible regressions introduced,  so I'd prefer to keep them all. 
Regarding the `ExpirationAction`, I agree, will remove the `INVALIDATE` one and 
just use `DESTROY`.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jujoramos commented on a change in pull request #4978: Fix for regression introduced by GEODE-7565

[GitBox]

jujoramos commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r412828019



##
File path: 
geode-core/src/main/java/org/apache/geode/cache/client/internal/PingOp.java
##
@@ -43,12 +45,16 @@ private PingOp() {
   static class PingOpImpl extends AbstractOp {
 
 private long startTime;
+private ServerLocation location;

Review comment:
   The above fields are not used anywhere, can we just remove them?, or am 
I missing something?.

##
File path: 
geode-core/src/main/java/org/apache/geode/cache/client/internal/PingOp.java
##
@@ -65,8 +71,9 @@ protected boolean needsUserId() {
 @Override
 protected void sendMessage(Connection cnx) throws Exception {
   getMessage().clearMessageHasSecurePartFlag();
-  this.startTime = System.currentTimeMillis();
-  getMessage().send(false);
+  getMessage().setNumberOfParts(1);
+  getMessage().addObjPart(serverID);
+  getMessage().send(true);

Review comment:
   These operations can be directly executed within the `PingOpImpl` 
constructor, as we do with the rest of the messages. I've also noted that you 
changed the `clearMessage` flag from `false` to `true` as well, any reasons 
behind that?.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jdeppe-pivotal commented on issue #4861: GEODE-7905: RedisDistDUnitTest failing intermittently in CI

[GitBox]

jdeppe-pivotal commented on issue #4861:
URL: https://github.com/apache/geode/pull/4861#issuecomment-617771032


   Since we're actively working on various approaches to this, and it's 
probably still going to take a while, I'm going to close this PR for now.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] jujoramos commented on issue #4978: Fix for regression introduced by GEODE-7565

[GitBox]

jujoramos commented on issue #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-617814149


   One additional comment regarding the following waning message:
   
   ```
   [warn 2020/04/18 23:44:22.757 PDT  
tid=0x298] Unable to ping non-member 
rs-FullRegression19040559a2i32xlarge-hydra-client-63(bridgegemfire1_host1_4749:4749):41003
 for client 
identity(rs-FullRegression19040559a2i32xlarge-hydra-client-63(edgegemfire3_host1_1071:1071:loner):50046:5a182991:edgegemfire3_host1_1071,connection=2
   ```
   
   The above is logged by member `bridgegemfire1_host1_4749` in the tests, 
exactly the member to which the `ping` command was sent... this warning is 
logged within the `pingCorrectServer` method which, unless I'm missing 
something, shouldn't be invoked at all if this member is the actual recipient 
for the `ping` message. Maybe `!myID.equals(targetServer)` is not working as 
expected?.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] prettyClouds opened a new pull request #4984: Refactor: Split SetsIntegrationTest into multiple files

[GitBox]

prettyClouds opened a new pull request #4984:
URL: https://github.com/apache/geode/pull/4984


   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 build` run cleanly?
   
   - [ ] Have you written or updated unit tests to verify your changes?
   
   - [ ] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)?
   
   ### Note:
   Please ensure that once the PR is submitted, check Concourse 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.
   



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [geode] alb3rtobr commented on issue #4978: Fix for regression introduced by GEODE-7565

[GitBox]

alb3rtobr commented on issue #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-617861442


   > One additional comment regarding the following waning message:
   > 
   > ```
   > [warn 2020/04/18 23:44:22.757 PDT  tid=0x298] Unable to ping non-member 
rs-FullRegression19040559a2i32xlarge-hydra-client-63(bridgegemfire1_host1_4749:4749):41003
 for client 
identity(rs-FullRegression19040559a2i32xlarge-hydra-client-63(edgegemfire3_host1_1071:1071:loner):50046:5a182991:edgegemfire3_host1_1071,connection=2
   > ```
   > 
   > The above is logged by member `bridgegemfire1_host1_4749` in the tests, 
exactly the member to which the `ping` command was sent... this warning is 
logged within the `pingCorrectServer` method which, unless I'm missing 
something, shouldn't be invoked at all if this member is the actual recipient 
for the `ping` message. Maybe `!myID.equals(targetServer)` is not working as 
expected?.
   
   I have added a log right after the comparision to check which values were 
considered different. I have created two clusters with two servers, and in my 
case I dont see the comparision is not working, this is an example:
   ```
   [info 2020/04/22 15:05:01.338 GMT  
tid=0x56] ALBERTO - MyID=172.17.0.3(server-0:85):41000 - 
target=172.17.0.9(server-1:84):41000
   ```
   Could you add the same log to your code?



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org