Mailing-List: contact commits-help@geode.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@geode.incubator.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: klund@apache.org
To: commits@geode.incubator.apache.org
Date: Fri, 22 Jul 2016 05:17:46 -0000
Message-Id: <077aa6fe9a8a4886bb0d30494dc4eb09@git.apache.org>
In-Reply-To: <228fbc31db714d4bb23977c0824d61fa@git.apache.org>
References: <228fbc31db714d4bb23977c0824d61fa@git.apache.org>
Subject: [08/10] incubator-geode git commit: GEODE-1669: Made changes to
 add/remove list of interested keys using addAll() and removeAll()
archived-at: Fri, 22 Jul 2016 05:17:46 -0000

GEODE-1669: Made changes to add/remove list of interested keys using addAll() and removeAll()

The client interests are managed in "FilterProfile" class on server side. These are maintained
using the concurrent data structures CopyOnWriteHashSet and CopyOnWriteHashMap...

When set of keys are registered from client, the keys are added to CopyOnWriteHashSet one by
one (FilterProfile.registerClientInterestList()); Where a new HashSet is created each time when
an entry is added, which could impact performance based on the number of keys registered.

Testing:
precheckin
Added new unit test.


Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/686db9c0
Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/686db9c0
Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/686db9c0

Branch: refs/heads/feature/GEODE-1673-PR-212
Commit: 686db9c07eda35a435f9cf7ba8745f1c79f14000
Parents: 9103a3d
Author: agingade <agingade@pivotal.io>
Authored: Thu Jul 21 15:50:36 2016 -0700
Committer: agingade <agingade@pivotal.io>
Committed: Thu Jul 21 15:50:56 2016 -0700

----------------------------------------------------------------------
 .../gemfire/internal/cache/FilterProfile.java   | 46 ++++++-----
 .../tier/sockets/FilterProfileJUnitTest.java    | 82 ++++++++++++++++++++
 2 files changed, 104 insertions(+), 24 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/686db9c0/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/FilterProfile.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/FilterProfile.java b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/FilterProfile.java
index 08a6484..73df7be 100755
--- a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/FilterProfile.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/FilterProfile.java
@@ -515,20 +515,19 @@ public class FilterProfile implements DataSerializableFixedID {
   public Set registerClientInterestList(Object inputClientID,
       List keys, boolean updatesAsInvalidates) {
     Long clientID = getClientIDForMaps(inputClientID);
-    Set keysRegistered = new HashSet();
+    Set keysRegistered = new HashSet(keys);
     synchronized (interestListLock) {
-      Map<Object, Set> koi = updatesAsInvalidates?
-          getKeysOfInterestInv() : getKeysOfInterest();
-      Set interestList = koi.get(clientID);
+      Map<Object, Set> koi = updatesAsInvalidates?getKeysOfInterestInv():getKeysOfInterest();
+      CopyOnWriteHashSet interestList = (CopyOnWriteHashSet)koi.get(clientID);
       if (interestList == null) {
         interestList = new CopyOnWriteHashSet();
         koi.put(clientID, interestList);
+      } else {
+        // Get the list of keys that will be registered new, not already registered.
+        keysRegistered.removeAll(interestList.getSnapshot());
       }
-      for (Object key: keys) { 
-        if (interestList.add(key)) { 
-          keysRegistered.add(key); 
-        } 
-      } 
+      interestList.addAll(keys);
+
       if (this.region != null && this.isLocalProfile) {
         sendProfileOperation(clientID, operationType.REGISTER_KEYS, keys, updatesAsInvalidates);
       }
@@ -549,36 +548,35 @@ public class FilterProfile implements DataSerializableFixedID {
   public Set unregisterClientInterestList(Object inputClientID,
       List keys) {
     Long clientID = getClientIDForMaps(inputClientID);
-    Set keysUnregistered = new HashSet(); 
+    Set keysUnregistered = new HashSet(keys);
+    Set keysNotUnregistered = new HashSet(keys);
     synchronized (interestListLock) {
-      Set interestList = getKeysOfInterest().get(clientID);
+      CopyOnWriteHashSet interestList = (CopyOnWriteHashSet)getKeysOfInterest().get(clientID);
       if (interestList != null) {
-        for (Iterator i = keys.iterator(); i.hasNext();) { 
-          Object keyOfInterest = i.next(); 
-          if (interestList.remove(keyOfInterest)) { 
-            keysUnregistered.add(keyOfInterest); 
-          } 
-        }       
+        // Get the list of keys that are not registered but in unregister set.
+        keysNotUnregistered.removeAll(interestList.getSnapshot());
+        interestList.removeAll(keys);
+
         if (interestList.isEmpty()) {
           getKeysOfInterest().remove(clientID);
         }
       }
-      interestList = getKeysOfInterestInv().get(clientID);
+      interestList = (CopyOnWriteHashSet)getKeysOfInterestInv().get(clientID);
       if (interestList != null) {
-        for (Iterator i = keys.iterator(); i.hasNext();) { 
-          Object keyOfInterest = i.next(); 
-          if (interestList.remove(keyOfInterest)) { 
-            keysUnregistered.add(keyOfInterest); 
-          } 
-        }       
+        keysNotUnregistered.removeAll(interestList.getSnapshot());
+        interestList.removeAll(keys);
+
         if (interestList.isEmpty()) {
           getKeysOfInterestInv().remove(clientID);
         }
       }
+
       if (this.region != null && this.isLocalProfile) {
         sendProfileOperation(clientID, operationType.UNREGISTER_KEYS, keys, false);
       }
     } // synchronized
+    // Get the keys that are not unregistered.
+    keysUnregistered.removeAll(keysNotUnregistered);
     return keysUnregistered;
   }
   

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/686db9c0/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/FilterProfileJUnitTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/FilterProfileJUnitTest.java b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/FilterProfileJUnitTest.java
index 5d53dd2..29709a7 100644
--- a/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/FilterProfileJUnitTest.java
+++ b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/FilterProfileJUnitTest.java
@@ -19,7 +19,10 @@ package com.gemstone.gemfire.internal.cache.tier.sockets;
 import static org.junit.Assert.*;
 import static org.mockito.Mockito.*;
 
+import java.util.Arrays;
 import java.util.Collections;
+import java.util.List;
+import java.util.Set;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -403,4 +406,83 @@ public class FilterProfileJUnitTest {
     assertFalse(fprofile.hasFilterInterestFor(clientId, inv));
   }
   
+  @Test
+  public void testRegisterUnregisterClientInterestListAndVerifyKeysRegistered() {
+    registerUnregisterClientInterestListAndVerifyKeysRegistered(false);
+  }
+
+  @Test
+  public void testRegisterUnregisterClientInterestListInvAndVerifyKeysRegistered() {
+    registerUnregisterClientInterestListAndVerifyKeysRegistered(true);
+  }
+
+
+  private void registerUnregisterClientInterestListAndVerifyKeysRegistered(boolean updatesAsInvalidates){
+    String clientId = "client";
+    List<String> keys = Arrays.asList("K1", "K2");
+
+    Set registeredKeys = fprofile.registerClientInterestList(clientId, keys, updatesAsInvalidates);
+    int numKeys = updatesAsInvalidates? fprofile.getKeysOfInterestInv(clientId).size():fprofile.getKeysOfInterest(clientId).size();
+    assertEquals(2, numKeys);
+    assertTrue("Expected key not found in registered list.", registeredKeys.containsAll(keys));
+
+    // Re-register same keys. The return should be empty.
+    registeredKeys = fprofile.registerClientInterestList(clientId, keys, updatesAsInvalidates);
+    numKeys = updatesAsInvalidates? fprofile.getKeysOfInterestInv(clientId).size():fprofile.getKeysOfInterest(clientId).size();
+    assertEquals(2, numKeys);
+    assertEquals(0, registeredKeys.size());
+
+    // Register one old key and new. It should return only the new key.
+    keys = Arrays.asList("K2", "K3");
+    registeredKeys = fprofile.registerClientInterestList(clientId, keys, updatesAsInvalidates);
+    numKeys = updatesAsInvalidates? fprofile.getKeysOfInterestInv(clientId).size():fprofile.getKeysOfInterest(clientId).size();
+    assertEquals(3, numKeys);
+    assertEquals(1, registeredKeys.size());
+    assertTrue("Expected key not found in registered list.", registeredKeys.contains("K3"));
+
+    // Keys Registered are K1, K2, K3
+    keys = Arrays.asList("K1", "K2");
+    registeredKeys = fprofile.unregisterClientInterestList(clientId, keys);
+    numKeys = updatesAsInvalidates? fprofile.getKeysOfInterestInv(clientId).size():fprofile.getKeysOfInterest(clientId).size();
+    assertEquals(1, numKeys);
+    assertTrue("Expected keys not found in unregistered list.", keys.containsAll(registeredKeys));
+
+    // Unregister previously unregistered key and a new key.
+    keys = Arrays.asList("K2", "K3");
+    registeredKeys = fprofile.unregisterClientInterestList(clientId, keys);
+    // Once all the interest for client is removed, the client id is removed from the interest list map.
+    Set keySet = updatesAsInvalidates? fprofile.getKeysOfInterestInv(clientId):fprofile.getKeysOfInterest(clientId);
+    assertNull(keySet);
+    assertEquals(1, registeredKeys.size());
+    assertTrue("Expected key not found in unregistered list.", registeredKeys.contains("K3"));
+
+    // Unregister again, this should return empty set.
+    registeredKeys = fprofile.unregisterClientInterestList(clientId, keys);
+    assertTrue(registeredKeys.isEmpty());
+  }
+
+  @Test
+  public void testRegisterUnregisterClientInterestListsAndVerifyKeysRegistered() {
+    String clientId = "client";
+    List<String> keys = Arrays.asList("K1", "K2");
+
+    Set registeredKeys = fprofile.registerClientInterestList(clientId, keys, false);
+    // Register interest with invalidates.
+    keys = Arrays.asList("K3", "K4");
+    registeredKeys = fprofile.registerClientInterestList(clientId, keys, true);
+
+    // Unregister keys from both list.
+    keys = Arrays.asList("K2", "K3", "K5"); // K5 is not registered.
+    registeredKeys = fprofile.unregisterClientInterestList(clientId, keys);
+    assertEquals(2, registeredKeys.size());
+    assertFalse("Expected key not found in registered list.", registeredKeys.contains("K5"));
+
+    Set keySet = fprofile.getKeysOfInterest(clientId);
+    assertEquals(1, keySet.size());
+    assertTrue("Expected key not found in registered list.", keySet.contains("K1"));
+
+    keySet = fprofile.getKeysOfInterestInv(clientId);
+    assertEquals(1, keySet.size());
+    assertTrue("Expected key not found in registered list.", keySet.contains("K4"));
+  }
 }