[jira] [Commented] (GEODE-8878) PR clear and create index on secondary member would result in EntryDestroyedException

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GEODE-8878?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17275399#comment-17275399 ] 

ASF GitHub Bot commented on GEODE-8878:
---------------------------------------

kirklund commented on a change in pull request #5950:
URL: https://github.com/apache/geode/pull/5950#discussion_r567127213



##########
File path: geode-core/src/distributedTest/java/org/apache/geode/cache/query/partitioned/PRClearCreateIndexDUnitTest.java
##########
@@ -0,0 +1,264 @@
+/*
+ * 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.query.partitioned;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.Serializable;
+import java.util.stream.IntStream;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.client.ClientRegionShortcut;
+import org.apache.geode.cache.query.QueryService;
+import org.apache.geode.distributed.internal.ClusterDistributionManager;
+import org.apache.geode.distributed.internal.DistributionMessage;
+import org.apache.geode.distributed.internal.DistributionMessageObserver;
+import org.apache.geode.internal.cache.DistributedClearOperation;
+import org.apache.geode.internal.cache.DistributedClearOperation.ClearRegionMessage;
+import org.apache.geode.internal.cache.PartitionedRegionClearMessage;
+import org.apache.geode.test.dunit.AsyncInvocation;
+import org.apache.geode.test.dunit.rules.ClientVM;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+
+public class PRClearCreateIndexDUnitTest implements Serializable {
+  @Rule
+  public ClusterStartupRule cluster = new ClusterStartupRule(4, true);
+
+  private MemberVM primary, secondary;
+  private ClientVM client;
+
+  @Before
+  public void before() throws Exception {
+    int locatorPort = ClusterStartupRule.getDUnitLocatorPort();
+    primary = cluster.startServerVM(0, locatorPort);
+    secondary = cluster.startServerVM(1, locatorPort);
+
+    // create region on server1 first, making sure server1 has the primary bucket
+    primary.invoke(() -> {
+      DistributionMessageObserver.setInstance(new MessageObserver());
+      Region<Object, Object> region =
+          ClusterStartupRule.memberStarter.createPartitionRegion("regionA",
+              f -> f.setTotalNumBuckets(1).setRedundantCopies(2));
+      IntStream.range(0, 100).forEach(i -> region.put(i, "value" + i));
+    });
+
+    // server2 has the secondary bucket
+    secondary.invoke(() -> {
+      DistributionMessageObserver.setInstance(new MessageObserver());
+      ClusterStartupRule.memberStarter.createPartitionRegion("regionA",
+          f -> f.setTotalNumBuckets(1).setRedundantCopies(2));
+    });
+  }
+
+  @After
+  public void after() throws Exception {
+    primary.invoke(() -> {
+      DistributionMessageObserver.setInstance(null);
+    });
+    secondary.invoke(() -> {
+      DistributionMessageObserver.setInstance(null);
+    });
+  }
+
+  // All tests create index on secondary members. These tests are making sure we are requesting
+  // locks for clear on secondary members as well. If we create index on the primary, the clear
+  // and createIndex will run sequentially so there would be no error. But if we create index on
+  // the secondary member and if the secondary member will not
+  // request a lock for clear operation, it will result in an EntryDestroyedException when create
+  // index is happening.
+
+  // Note: OP_LOCK_FOR_CLEAR, OP_CLEAR, OP_UNLOCK_FOR_CLEAR are messages for secondary members
+  // OP_LOCK_FOR_PR_CLEAR, OP_UNLOCK_FOR_PR_CLEAR, OP_PR_CLEAR can be for anybody
+
+  @Test
+  // all local buckets are primary, so only OP_LOCK_FOR_CLEAR and OP_CLEAR messages are sent to the
+  // secondary member
+  // in the end an OP_PR_CLEAR is sent to the secondary for no effect
+  public void clearFromPrimaryMember() throws Exception {
+    AsyncInvocation createIndex = secondary.invokeAsync(PRClearCreateIndexDUnitTest::createIndex);
+    AsyncInvocation clear = primary.invokeAsync(PRClearCreateIndexDUnitTest::clear);
+
+    createIndex.get();
+    clear.get();
+
+    // assert that secondary member received these messages
+    primary.invoke(() -> verifyEvents(false, false, false, false));
+    secondary.invoke(() -> verifyEvents(false, true, true, true));
+  }
+
+  @Test
+  // all local buckets are secondary, so an OP_PR_CLEAR is sent to the primary member, from there
+  // a OP_LOCK_FOR_CLEAR and OP_CLEAR messages are sent back to the secondary
+  public void clearFromSecondaryMember() throws Exception {
+    AsyncInvocation createIndex = secondary.invokeAsync(PRClearCreateIndexDUnitTest::createIndex);
+    AsyncInvocation clear = secondary.invokeAsync(PRClearCreateIndexDUnitTest::clear);
+
+    createIndex.get();
+    clear.get();
+
+    // assert that secondary member received these messages
+    primary.invoke(() -> verifyEvents(false, true, false, false));
+    secondary.invoke(() -> verifyEvents(false, false, true, true));
+  }
+
+  /**
+   * For interested client connecting to secondary member
+   * 1. locks all local region
+   * 2. send OP_LOCK_FOR_PR_CLEAR to lock all other members
+   * 3. send OP_PR_CLEAR to primary to clear
+   * 4. primary will send a OP_CLEAR message back to the secondary to clear
+   */
+  @Test
+  public void clearFromInterestedClientConnectingToSecondaryMember() throws Exception {
+    int port = secondary.getPort();
+    client = cluster.startClientVM(2, c -> c.withServerConnection(port).withPoolSubscription(true));
+    AsyncInvocation createIndex = secondary.invokeAsync(PRClearCreateIndexDUnitTest::createIndex);
+
+    AsyncInvocation clear = client.invokeAsync(() -> {
+      Thread.sleep(200);
+      ClientCache clientCache = ClusterStartupRule.getClientCache();
+      Region<Object, Object> regionA =
+          clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("regionA");
+      regionA.registerInterestForAllKeys();
+      regionA.clear();
+    });
+
+    createIndex.get();
+    clear.get();
+    primary.invoke(() -> verifyEvents(true, true, false, false));
+    secondary.invoke(() -> verifyEvents(false, false, false, true));
+  }
+
+  @Test
+  /**
+   * For interested client connecting to primary member, behaves like starting from primary member
+   * except it locks first
+   * 1. locks local region
+   * 2. send OP_LOCK_FOR_PR_CLEAR to lock all other members
+   * 3. then since it already locked the current member, won't send a OP_LOCK_FOR_CLEAR message
+   * to secondaries, only OP_CLEAR will be sent
+   */
+  public void clearFromInterestedClientConnectingToPrimaryMember() throws Exception {
+    int port = primary.getPort();
+    client = cluster.startClientVM(2, c -> c.withServerConnection(port).withPoolSubscription(true));
+    AsyncInvocation createIndex = secondary.invokeAsync(PRClearCreateIndexDUnitTest::createIndex);
+
+    AsyncInvocation clear = client.invokeAsync(() -> {
+      Thread.sleep(200);

Review comment:
       Why are the Thread sleeps needed 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:
users@infra.apache.org


> PR clear and create index on secondary member would result in EntryDestroyedException
> -------------------------------------------------------------------------------------
>
>                 Key: GEODE-8878
>                 URL: https://issues.apache.org/jira/browse/GEODE-8878
>             Project: Geode
>          Issue Type: Sub-task
>          Components: regions
>            Reporter: Jinmei Liao
>            Assignee: Jinmei Liao
>            Priority: Major
>              Labels: GeodeOperationAPI
>
> When doing a PR clear, a lock message should be sent to all the other members to request lock to prevent "create index" operation from happening at the same time on secondary server.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)