[geode] branch develop updated: GEODE-6704: optimize getAllGatewaySenderIds (#3518)

[ds...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

dschneider pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/develop by this push:
     new f2710b1  GEODE-6704: optimize getAllGatewaySenderIds (#3518)
f2710b1 is described below

commit f2710b1d4c468b7bc591b990f8ffbcb4f409c36b
Author: Darrel Schneider <ds...@pivotal.io>
AuthorDate: Tue Apr 30 10:14:23 2019 -0700

    GEODE-6704: optimize getAllGatewaySenderIds (#3518)
    
    * added unit tests for AbstractRegion.getAllGatewaySenderIds
    
    * optimized AbstractRegion.allGatewaySenderIds to do no object creations
---
 .../geode/internal/cache/AbstractRegion.java       | 11 ++--
 .../internal/cache/AbstractRegionJUnitTest.java    | 75 +++++++++++++++++++++-
 2 files changed, 81 insertions(+), 5 deletions(-)

diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegion.java b/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegion.java
index 9b28c7f..182c899 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegion.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegion.java
@@ -23,6 +23,7 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -190,6 +191,9 @@ public abstract class AbstractRegion implements InternalRegion, AttributesMutato
 
   private Set<String> visibleAsyncEventQueueIds;
 
+  /**
+   * This set is always unmodifiable.
+   */
   private Set<String> allGatewaySenderIds;
 
   protected boolean enableSubscriptionConflation;
@@ -703,7 +707,7 @@ public abstract class AbstractRegion implements InternalRegion, AttributesMutato
 
   @Override
   public Set<String> getAllGatewaySenderIds() {
-    return Collections.unmodifiableSet(this.allGatewaySenderIds);
+    return this.allGatewaySenderIds;
   }
 
   /**
@@ -997,12 +1001,11 @@ public abstract class AbstractRegion implements InternalRegion, AttributesMutato
     if (getGatewaySenderIds().isEmpty() && getAsyncEventQueueIds().isEmpty()) {
       this.allGatewaySenderIds = Collections.emptySet(); // fix for bug 45774
     }
-    Set<String> tmp = new CopyOnWriteArraySet<String>();
-    tmp.addAll(this.getGatewaySenderIds());
+    Set<String> tmp = new HashSet<String>(this.getGatewaySenderIds());
     for (String asyncQueueId : this.getAsyncEventQueueIds()) {
       tmp.add(AsyncEventQueueImpl.getSenderIdFromAsyncEventQueueId(asyncQueueId));
     }
-    this.allGatewaySenderIds = tmp;
+    this.allGatewaySenderIds = Collections.unmodifiableSet(tmp);
   }
 
   private void initializeVisibleAsyncEventQueueIds(InternalRegionArguments internalRegionArgs) {
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/AbstractRegionJUnitTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/AbstractRegionJUnitTest.java
index 41c79f0..a04f5b7 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/cache/AbstractRegionJUnitTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/AbstractRegionJUnitTest.java
@@ -15,13 +15,26 @@
 package org.apache.geode.internal.cache;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
+import java.util.Set;
 
 import org.junit.Test;
 
+import org.apache.geode.cache.DataPolicy;
+import org.apache.geode.cache.DiskWriteAttributes;
+import org.apache.geode.cache.EvictionAction;
+import org.apache.geode.cache.EvictionAttributes;
+import org.apache.geode.cache.ExpirationAttributes;
 import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionAttributes;
+import org.apache.geode.cache.asyncqueue.internal.AsyncEventQueueImpl;
+import org.apache.geode.internal.cache.LocalRegion.RegionMapConstructor;
 import org.apache.geode.internal.cache.extension.ExtensionPoint;
 import org.apache.geode.internal.cache.extension.SimpleExtensionPoint;
+import org.apache.geode.test.fake.Fakes;
 
 /**
  * Unit tests for {@link AbstractRegion}.
@@ -38,10 +51,70 @@ public class AbstractRegionJUnitTest {
    * rest.
    */
   @Test
-
   public void extensionPointIsSimpleExtensionPointByDefault() {
     AbstractRegion region = spy(AbstractRegion.class);
     ExtensionPoint<Region<?, ?>> extensionPoint = region.getExtensionPoint();
     assertThat(extensionPoint).isNotNull().isInstanceOf(SimpleExtensionPoint.class);
   }
+
+  @Test
+  public void getAllGatewaySenderIdsReturnsEmptySet() {
+    AbstractRegion region = createTestableAbstractRegion();
+
+    Set<String> result = region.getAllGatewaySenderIds();
+
+    assertThat(result).isEmpty();
+  }
+
+  @Test
+  public void getAllGatewaySenderIdsIncludesAsyncEventQueueId() {
+    AbstractRegion region = createTestableAbstractRegion();
+    region.addAsyncEventQueueId("asyncQueueId", false);
+    String asyncQueueId = AsyncEventQueueImpl.getSenderIdFromAsyncEventQueueId("asyncQueueId");
+
+    Set<String> result = region.getAllGatewaySenderIds();
+
+    assertThat(result).containsExactlyInAnyOrder(asyncQueueId);
+  }
+
+  @Test
+  public void getAllGatewaySenderIdsIncludesGatewaySenderIds() {
+    AbstractRegion region = createTestableAbstractRegion();
+    region.addGatewaySenderId("gatewaySenderId");
+
+    Set<String> result = region.getAllGatewaySenderIds();
+
+    assertThat(result).containsExactlyInAnyOrder("gatewaySenderId");
+  }
+
+  @Test
+  public void getAllGatewaySenderIdsIncludesBothGatewaySenderIdsAndAsyncQueueIds() {
+    AbstractRegion region = createTestableAbstractRegion();
+    region.addGatewaySenderId("gatewaySenderId");
+    region.addAsyncEventQueueId("asyncQueueId", false);
+    String asyncQueueId = AsyncEventQueueImpl.getSenderIdFromAsyncEventQueueId("asyncQueueId");
+
+    Set<String> result = region.getAllGatewaySenderIds();
+
+    assertThat(result).containsExactlyInAnyOrder("gatewaySenderId", asyncQueueId);
+  }
+
+  private AbstractRegion createTestableAbstractRegion() {
+    RegionAttributes regionAttributes = mock(RegionAttributes.class);
+    when(regionAttributes.getDataPolicy()).thenReturn(DataPolicy.DEFAULT);
+    EvictionAttributes evictionAttributes = mock(EvictionAttributes.class);
+    when(evictionAttributes.getAction()).thenReturn(EvictionAction.NONE);
+    when(regionAttributes.getEvictionAttributes()).thenReturn(evictionAttributes);
+    ExpirationAttributes expirationAttributes = mock(ExpirationAttributes.class);
+    when(regionAttributes.getRegionTimeToLive()).thenReturn(expirationAttributes);
+    when(regionAttributes.getRegionIdleTimeout()).thenReturn(expirationAttributes);
+    when(regionAttributes.getEntryTimeToLive()).thenReturn(expirationAttributes);
+    when(regionAttributes.getEntryIdleTimeout()).thenReturn(expirationAttributes);
+    DiskWriteAttributes diskWriteAttributes = mock(DiskWriteAttributes.class);
+    when(regionAttributes.getDiskWriteAttributes()).thenReturn(diskWriteAttributes);
+    RegionMapConstructor regionMapConstructor = mock(RegionMapConstructor.class);
+    AbstractRegion region = new LocalRegion("regionName", regionAttributes, null, Fakes.cache(),
+        new InternalRegionArguments(), null, regionMapConstructor, null, null, null);
+    return region;
+  }
 }