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/master Commit: 686db9c07eda35a435f9cf7ba8745f1c79f14000 Parents: 9103a3d Author: agingade <[email protected]> Authored: Thu Jul 21 15:50:36 2016 -0700 Committer: agingade <[email protected]> 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")); + } }
