GEODE-9369: Command to copy region entries from a WAN site to another #6833

albertogpz · 2021-09-02T19:25:43Z

This PR contains the original commit of PR #6601 plus some improvements mainly on the testing as suggested by @kirklund .

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?

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.

geode-core/src/main/java/org/apache/geode/internal/cache/wan/GatewaySenderEventImpl.java

kirklund · 2021-09-07T21:42:15Z

geode-core/src/main/java/org/apache/geode/management/internal/functions/CliFunction.java

+ * An abstract function implementation to be extended by cli functions. Any cli function extending
+ * this class has to return a CliFunctionResult.
+ */
+public abstract class CliFunction<T> implements InternalFunction<T> {


Instead of copying CliFunction to geode-core, I would copy some build.gradle changes from geode-connectors to geode-wan.

The geode-connectors module is a good one to use as an example for how it adds GFSH commands modularly without changes to geode-gfsh or geode-core.

Basically, you would just add a geode-gfsh dependency to geode-wan's build.gradle just like it has in geode-connectors:

implementation(project(':geode-gfsh'))

And you probably need this also because gfsh extends spring-shell:

implementation('org.springframework.shell:spring-shell') { exclude module: 'aopalliance' exclude module: 'asm' exclude module: 'cglib' exclude module: 'guava' exclude module: 'spring-aop' exclude module: 'spring-context-support' exclude module: 'spring-core' ext.optional = true }

If you get any weird compilation failures with those added, study the geode-gfsh and geode-connectors build.gradle files to see if there are more dependencies that are needed.

kirklund · 2021-09-07T21:45:23Z

geode-gfsh/build.gradle


    api(project(':geode-core'))
    api(project(':geode-common'))
+    api(project(':geode-wan'))


If the dependency is flipped so that geode-wan depends on geode-gfsh, then you can remove this line.

kirklund · 2021-09-07T21:46:25Z

...sh/src/main/java/org/apache/geode/management/internal/cli/commands/WanCopyRegionCommand.java

+    final Object[] args = {regionName, senderId, isCancel, maxRate, batchSize};
+    ResultCollector<?, ?> resultCollector =
+        executeFunction(wanCopyRegionFunction, args, getAllNormalMembers());
+    @SuppressWarnings("unchecked")


Looks like you've added types, do you still need this suppression?

Not anymore.

kirklund · 2021-09-07T21:48:25Z

geode-wan/src/distributedTest/java/org/apache/geode/internal/cache/wan/WANTestBase.java


      fact.setOffHeap(offHeap);
-      Region r = fact.create(regionName);
+      Region<Object, Object> r = fact.create(regionName);


Region<?, ?> usually works in places like this, too. Either typing should be ok.

kirklund · 2021-09-07T22:17:05Z

...st/java/org/apache/geode/management/internal/cli/commands/WanCopyRegionCommandDUnitTest.java

+        .submit(wanCopyCommandFuture);
+
+    // Wait for the wan-copy command to start
+    wanCopyCommandStartLatch.await();


Add the GeodeAwaitility timeout.

Not applicable as there is no CountDownLatch anymore.

kirklund · 2021-09-07T22:19:12Z

...st/java/org/apache/geode/management/internal/cli/commands/WanCopyRegionCommandDUnitTest.java

+    asyncOps1.await();
+    asyncOps2.await();
+    asyncOps3.await();


Note: These are ok because AsyncInvocation is our code and await() actually uses the GeodeAwaitility timeout under the covers.

kirklund · 2021-09-07T22:19:26Z

...st/java/org/apache/geode/management/internal/cli/commands/WanCopyRegionCommandDUnitTest.java

+        .submit(wanCopyCommandFuture);
+
+    // Wait for the wan-copy command to start
+    wanCopyCommandStartLatch.await();


Add the GeodeAwaitility timeout.

Not applicable as there is no CountDownLatch anymore.

kirklund · 2021-09-07T22:20:25Z

...st/java/org/apache/geode/management/internal/cli/commands/WanCopyRegionCommandDUnitTest.java

+  }
+
+  public static void removeEntry(String regionName, long key) {
+    Region<?, ?> r = ClientCacheFactory.getAnyInstance().getRegion(SEPARATOR + regionName);


Singleton call.

kirklund · 2021-09-07T22:20:33Z

...st/java/org/apache/geode/management/internal/cli/commands/WanCopyRegionCommandDUnitTest.java

+  }
+
+  public void sendRandomOpsFromClient(String regionName, Set<Long> keySet, int iterations) {
+    Region<Long, Integer> r = ClientCacheFactory.getAnyInstance().getRegion(SEPARATOR + regionName);


Singleton call.

albertogpz · 2021-09-17T13:55:28Z

I've pointed out some things you can change to move all of your changes into geode-wan instead of having any code changes in geode-core and geode-gfsh.

I also think you should change the function to delegate execution tasks to the service.

Thanks for the suggestions. They are very good.

ghost

Build changes seem good.

kirklund · 2021-10-01T21:25:01Z

@albertogpz Quick note to let you I'm reviewing this.

kirklund · 2021-10-01T21:31:18Z

...est/java/org/apache/geode/cache/wan/internal/cli/commands/WanCopyRegionCommandDUnitTest.java

+      command.hasTableSection(ResultModel.MEMBER_STATUS_SECTION).hasColumn("Message")
+          .asList().haveExactly(1, exceptionError).haveExactly(2, senderNotPrimary);
+    }


[comment] Not a request for changes, but I generally try to format these chained APIs with linefeeds like:

command .hasTableSection(ResultModel.MEMBER_STATUS_SECTION) .hasColumn("Message") .asList() .haveExactly(1, exceptionError) .haveExactly(2, senderNotPrimary);

I'm not sure if it's easier to read for everyone, but it is for me.

kirklund · 2021-10-01T21:32:51Z

...est/java/org/apache/geode/cache/wan/internal/cli/commands/WanCopyRegionCommandDUnitTest.java

+    AsyncInvocation<Object> asyncOps1 =
+        client.invokeAsync(() -> sendRandomOpsFromClient(regionName, keySet, 10));
+    AsyncInvocation<Object> asyncOps2 =
+        client.invokeAsync(() -> sendRandomOpsFromClient(regionName, keySet, 10));
+    AsyncInvocation<Object> asyncOps3 =
+        client.invokeAsync(() -> sendRandomOpsFromClient(regionName, keySet, 10));


Since you don't actually need a return from these (you just call await() down below), you should just use the Void type: AsyncInvocation<Void>.

kirklund · 2021-10-01T21:35:13Z

...est/java/org/apache/geode/cache/wan/internal/cli/commands/WanCopyRegionCommandDUnitTest.java

+  }
+
+  public static void removeEntry(String regionName, long key) {
+    Region<?, ?> r = cache.getRegion(SEPARATOR + regionName);


You should go ahead and name any variables with a full word like region.

kirklund · 2021-10-01T21:42:19Z

...-wan/src/test/java/org/apache/geode/cache/wan/internal/WanCopyRegionFunctionServiceTest.java

+          try {
+            return service.execute(firstExecution, regionName, senderId);
+          } catch (Exception e) {
+            return null;


[comment] Returning null is probably ok. Or you could use an ErrorCollector JUnit rule here as well.

kirklund · 2021-10-01T21:44:47Z

...va/org/apache/geode/management/internal/cli/functions/WanCopyRegionFunctionDelegateTest.java

+  private EventCreator eventCreatorMock;
+
+  @Before
+  public void setUp() throws InterruptedException {


[comment] Some of these big methods are easier to read if you group all of the mocks together at the top, and then all of the stubbings (when statements) down lower.

kirklund · 2021-10-01T21:47:56Z

...va/org/apache/geode/management/internal/cli/functions/WanCopyRegionFunctionDelegateTest.java

+    Object[] options = new Object[] {"myRegion", "mySender", false, 1L, 2};
+    WanCopyRegionFunctionDelegate.ThreadSleeper sleeperMock =
+        mock(WanCopyRegionFunctionDelegate.ThreadSleeper.class);
+    doNothing().when(sleeperMock).sleep(anyLong());
+
+    executeWanCopyRegionFunction(options, sleeperMock);
+    ArgumentCaptor<Long> captor = ArgumentCaptor.forClass(Long.class);
+    verify(sleeperMock, times(1)).sleep(captor.capture());
+    assertThat(captor.getValue()).isGreaterThan(0).isLessThanOrEqualTo(2000);


[comment] Another way to organize these tests for (hopefully) easier reading is to group it in arrange/act/assert sections:

// arrange Object[] options = new Object[] {"myRegion", "mySender", false, 1L, 2}; WanCopyRegionFunctionDelegate.ThreadSleeper sleeperMock = mock(WanCopyRegionFunctionDelegate.ThreadSleeper.class); doNothing().when(sleeperMock).sleep(anyLong()); // act executeWanCopyRegionFunction(options, sleeperMock); // assert ArgumentCaptor<Long> captor = ArgumentCaptor.forClass(Long.class); verify(sleeperMock, times(1)).sleep(captor.capture()); assertThat(captor.getValue()).isGreaterThan(0).isLessThanOrEqualTo(2000);

kirklund · 2021-10-01T21:50:24Z

...va/org/apache/geode/management/internal/cli/functions/WanCopyRegionFunctionDelegateTest.java

+    assertThatThrownBy(
+        () -> function.wanCopyRegion(internalCacheMock, "member1", regionMock, gatewaySenderMock, 1,
+            10))
+                .isInstanceOf(NoAvailableServersException.class);


[comment] If you decide you like arrange/act/assert then you can use this syntax if you want:

// arrange ... // act Throwable thrown = catchThrowable(() -> { function.wanCopyRegion(internalCacheMock, "member1", regionMock, gatewaySenderMock, 1, 10); }); // assert assertThat(thrown).isInstanceOf(NoAvailableServersException.class);

kirklund · 2021-10-01T21:52:04Z

...-wan/src/test/java/org/apache/geode/cache/wan/internal/WanCopyRegionFunctionServiceTest.java

+public class WanCopyRegionFunctionServiceTest {
+
+  private WanCopyRegionFunctionService service;
+  InternalCache cache = mock(InternalCache.class);


private InternalCache cache = mock(InternalCache.class);

…apache#6601) * GEODE-9369: Command to copy region entries from a WAN site to another The command has been implemented as proposed in https://cwiki.apache.org/confluence/display/GEODE/Geode+Command+to+replicate+region+data+from+one+site+to+another+connected+via+WAN with some modifications with respect to the initial proposal. The command will get the entries of a region in a WAN site and will put them in batches that will be sent by a gateway sender to a remote WAN site. * GEODE-9369: Remove comments added to geode-gfsh/src/test/resources/expected-pom.xml * GEODE-9369: Changes after review * GEODE-9369: Update with Kirk's review comments * GEODE-9369: Update with boglesby's review comments * GEODE-9369: Changes after boglesby's review * GEODE-9369: Updated with davebarnes97's review comments. * GEODE-9369: More changes after davebarnes97's review * GEODE-9369: Updated with DonalEvan's comments * GEODE-9369: Small refactoring: Use CompletableFuture instead of Callable + FutureTask * GEODE-9369: Fix race condition added in previous commit * GEODE-9369: Small change of re-review from DonalEvans * GEODE-9369: Fix esporadic test failures due to error log. * GEODE-9369: Changes required after rebasing to develop

- Remove sleeps in unit tests that made some of them flaky, especially in windows - Remove use of DEEP_STUBS in mocks - Move new function to geode-wan - Handle the lifecycle of the executor used by the function in a new CacheService class in wan - Remove partial mocking in unit tests

In order to be able to use CliFunction in WanCopyRegion without adding a dependency to geode-gfsh, I created a CliFunction class inside geode-core which is a copy of that same class in geode-gfsh. At first I thought about moving the class from geode-gfsh to geode-core which seemed more logical but I realized it would break the compatibility of a public API.

kirklund

You should change any unit test that is validating that Thread.sleep throws InterruptedException to only test your class' interaction with ThreadSleeper. I inlined an example for one of the tests.

That Thread.sleep(1) is very prone to flakiness.

kirklund · 2021-10-04T22:54:18Z

geode-core/src/test/java/org/apache/geode/internal/cache/wan/GatewaySenderEventImplTest.java

+  public void testCreation_WithAfterUpdateWithGenerateCallbacks(boolean isGenerateCallbacks,
+      boolean isCallbackArgumentNull)
+      throws IOException {
+    LocalRegion region = mock(LocalRegion.class);


Please use InternalRegion instead of LocalRegion.

kirklund · 2021-10-04T22:59:36Z

...est/java/org/apache/geode/cache/wan/internal/cli/commands/WanCopyRegionCommandDUnitTest.java

+    await().until(
+        () -> getNumberOfCurrentExecutionsInServers(servers) == executionsExpected);
+  }


Anytime you're awaiting something with more than a binary value, I recommend using an assertion so that if it fails, you'll see the exact value:

await().untilAsserted( () -> assertThat(getNumberOfCurrentExecutionsInServers(servers)).isEqualTo(executionsExpected));

kirklund · 2021-10-04T23:03:09Z

...-wan/src/test/java/org/apache/geode/cache/wan/internal/WanCopyRegionFunctionServiceTest.java

+    }
+
+    // Wait for the functions to start execution
+    await().until(() -> service.getNumberOfCurrentExecutions() == executions);


Another one where I'd use untilAsserted with an assertion so you see the exact number if it fails.

kirklund · 2021-10-04T23:05:29Z

...-wan/src/test/java/org/apache/geode/cache/wan/internal/WanCopyRegionFunctionServiceTest.java

+    CountDownLatch latch = new CountDownLatch(executions);
+    for (int i = 0; i < executions; i++) {
+      Callable<CliFunctionResult> firstExecution = () -> {
+        latch.await();


Since CountDownLatch await waits forever by default, you should change it to:

latch.await(GeodeAwaitility.getTimeout().getSeconds(), TimeUnit.SECONDS);

kirklund · 2021-10-04T23:08:51Z

...va/org/apache/geode/management/internal/cli/functions/WanCopyRegionFunctionDelegateTest.java

+    AbstractGatewaySenderEventProcessor eventProcessorMock =
+        mock(AbstractGatewaySenderEventProcessor.class);
+    RegionAttributes<?, ?> attributesMock = mock(RegionAttributes.class);
+    Set<?> idsMock = mock(Set.class);


[comments] This is ok but Collections.emptySet() might be better to use since the general recommendation is to not mock classes from other libraries.

kirklund · 2021-10-04T23:14:56Z

...va/org/apache/geode/management/internal/cli/functions/WanCopyRegionFunctionDelegateTest.java

+    when(((AbstractGatewaySender) gatewaySenderMock).getEventProcessor())
+        .thenReturn(eventProcessorMock);


I think the interface InternalGatewaySender should work here instead of AbstractGatewaySender.

kirklund · 2021-10-04T23:20:21Z

...va/org/apache/geode/management/internal/cli/functions/WanCopyRegionFunctionDelegateTest.java

+    doAnswer(invocation -> {
+      Thread.sleep(1);
+      return null;
+    }).when(threadSleeperMock).sleep(anyLong());


This test is actually validating that Thread.sleep throws InterruptedException when interrupted, but you don't need (or want) to test the Java library, just your own classes. So this would be a place where it's better to just stub threadSleeperMock to throw InterruptedException:

@Test public void doPostSendBatchActions_ThrowInterruptedIfInterruptedTimeToSleepNotZero() throws InterruptedException { // arrange long maxRate = 1; long elapsedTime = 20L; InterruptedException thrownByThreadSleeper = new InterruptedException("thrownByThreadSleeper"); when(clockMock.millis()).thenReturn(startTime + elapsedTime); doThrow(thrownByThreadSleeper).when(threadSleeperMock).sleep(anyLong()); // act Throwable thrown = catchThrowable(() -> function.doPostSendBatchActions(startTime, entries, maxRate)); // assert assertThat(thrown).isInstanceOf(InterruptedException.class); }

kirklund

Looking really solid Alberto!

albertogpz · 2021-10-06T18:03:44Z

Looking really solid Alberto!

Thanks to your thorough review and your suggestions ;-)

…efactor

onichols-pivotal · 2021-10-14T15:41:27Z

Looks like these new tests are having some trouble on Windows: http://files.apachegeode-ci.info/builds/apache-develop-main/1.15.0-build.0583/test-results/distributedTest/1634209508/

Tip: running Windows PR checks on a PR is as simple as adding the windows label in GitHub (either when you create the PR, or before your next commit)

… another (apache#6833)" This reverts commit c106e01.

… another (#6833)" (#7005) This reverts commit c106e01.

albertogpz force-pushed the feature/GEODE-9369_2 branch 7 times, most recently from 8e59781 to a0b33b2 Compare September 6, 2021 16:45

albertogpz requested review from demery-pivotal and kirklund September 6, 2021 20:41

kirklund requested changes Sep 7, 2021

View reviewed changes

albertogpz force-pushed the feature/GEODE-9369_2 branch 3 times, most recently from f4c3255 to 39c33fd Compare September 14, 2021 15:24

albertogpz marked this pull request as ready for review September 14, 2021 17:36

albertogpz requested review from Bill, DonalEvans, agingade, boglesby, echobravopapa, gesterzhou, jchen21, jdeppe-pivotal, jinmeiliao, mhansonp, nabarunnag and onichols-pivotal as code owners September 14, 2021 17:36

albertogpz requested a review from a user September 14, 2021 17:36

onichols-pivotal approved these changes Sep 14, 2021

View reviewed changes

onichols-pivotal requested review from alb3rtobr and davebarnes97 September 14, 2021 17:41

albertogpz mentioned this pull request Sep 16, 2021

GEODE-9369: Fix test failure when run on Windows #6802

Closed

6 tasks

jinmeiliao approved these changes Sep 17, 2021

View reviewed changes

albertogpz requested a review from kirklund September 23, 2021 07:52

ghost approved these changes Sep 29, 2021

View reviewed changes

kirklund reviewed Oct 1, 2021

View reviewed changes

albertogpz added 7 commits October 4, 2021 09:41

GEODE-9369: Changes after klund's review

f141231

GEODE-9369: More changes after Kirk Lund's review

6e65065

GEODE-9369: Some more changes after Kirk's review

75f05c4

GEODE-9369: Changes required after rebasing

a4ff342

albertogpz force-pushed the feature/GEODE-9369_2 branch from 6bc0526 to a4ff342 Compare October 4, 2021 08:44

kirklund requested changes Oct 4, 2021

View reviewed changes

GEODE-9369: More changes after Kirk's review

74e35a2

kirklund approved these changes Oct 5, 2021

View reviewed changes

albertogpz force-pushed the feature/GEODE-9369_2 branch from aec89ad to 67fc8de Compare October 6, 2021 17:42

albertogpz force-pushed the feature/GEODE-9369_2 branch from 67fc8de to 84e574a Compare October 6, 2021 18:04

GEODE-9369: Update test case that was changed wrongly in a previous r…

a3bb6f1

…efactor

albertogpz force-pushed the feature/GEODE-9369_2 branch from 84e574a to a3bb6f1 Compare October 6, 2021 21:21

echobravopapa approved these changes Oct 8, 2021

View reviewed changes

boglesby approved these changes Oct 13, 2021

View reviewed changes

albertogpz merged commit c106e01 into apache:develop Oct 14, 2021

kamilla1201 added a commit to kamilla1201/geode that referenced this pull request Oct 14, 2021

Revert "GEODE-9369: Command to copy region entries from a WAN site to…

02349d5

… another (apache#6833)" This reverts commit c106e01.

kamilla1201 mentioned this pull request Oct 14, 2021

Revert "GEODE-9369: Command to copy region entries from a WAN site to… #7005

Merged

6 tasks

kamilla1201 added a commit that referenced this pull request Oct 15, 2021

Revert "GEODE-9369: Command to copy region entries from a WAN site to…

3a59870

… another (#6833)" (#7005) This reverts commit c106e01.

		when(((AbstractGatewaySender) gatewaySenderMock).getEventProcessor())
		.thenReturn(eventProcessorMock);

GEODE-9369: Command to copy region entries from a WAN site to another #6833

GEODE-9369: Command to copy region entries from a WAN site to another #6833

Uh oh!

Conversation

albertogpz commented Sep 2, 2021

For all changes:

Note:

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertogpz commented Sep 17, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

kirklund commented Oct 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirklund left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirklund left a comment

Choose a reason for hiding this comment

Uh oh!

albertogpz commented Oct 6, 2021

Uh oh!

onichols-pivotal commented Oct 14, 2021

Uh oh!