traffic type exists refactor

Author: sanzmauro

client/src/main/java/io/split/client/SplitClientImpl.java

@@ -192,7 +192,7 @@ private boolean track(Event event) {
             event.trafficTypeName = event.trafficTypeName.toLowerCase();
         }
 
-        if (!_splitFetcher.fetchKnownTrafficTypes().contains(event.trafficTypeName)) {
+        if (!_splitFetcher.trafficTypeExists(event.trafficTypeName)) {
             _log.warn("track: Traffic Type " + event.trafficTypeName + " does not have any corresponding Splits in this environment, " +
                     "make sure you’re tracking your events to a valid traffic type defined in the Split console.");
         }

client/src/main/java/io/split/engine/cache/InMemoryCacheImp.java

@@ -1,14 +1,17 @@
 package io.split.engine.cache;
 
+import com.google.common.collect.ConcurrentHashMultiset;
 import com.google.common.collect.Maps;
+import com.google.common.collect.Multiset;
+import com.google.common.collect.Sets;
 import io.split.engine.experiments.ParsedSplit;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
-import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.atomic.AtomicLong;
 
@@ -17,27 +20,37 @@ public class InMemoryCacheImp implements SplitCache {
     private static final Logger _log = LoggerFactory.getLogger(InMemoryCacheImp.class);
 
     private final ConcurrentMap<String, ParsedSplit> _concurrentMap;
+    private final Multiset<String> _concurrentTrafficTypeNameSet;
+
     private AtomicLong _changeNumber;
 
+    public InMemoryCacheImp() {
+        this(-1);
+    }
+
     public InMemoryCacheImp(long startingChangeNumber) {
         _concurrentMap = Maps.newConcurrentMap();
         _changeNumber = new AtomicLong(startingChangeNumber);
+        _concurrentTrafficTypeNameSet = ConcurrentHashMultiset.create();
     }
 
     @Override
     public void put(ParsedSplit split) {
         _concurrentMap.put(split.feature(), split);
-    }
 
-    @Override
-    public void putAll(Map<String, ParsedSplit> splits) {
-        _concurrentMap.putAll(splits);
+        if (split.trafficTypeName() != null) {
+            _concurrentTrafficTypeNameSet.add(split.trafficTypeName());
+        }
     }
 
     @Override
     public boolean remove(String name) {
         ParsedSplit removed = _concurrentMap.remove(name);
 
+        if (removed != null && removed.trafficTypeName() != null) {
+            _concurrentTrafficTypeNameSet.remove(removed.trafficTypeName());
+        }
+
         return removed != null;
     }
 
@@ -81,12 +94,15 @@ public void setChangeNumber(long changeNumber) {
     }
 
     @Override
-    public boolean trafficTypeExists(String trafficType) {
-        return false;
+    public boolean trafficTypeExists(String trafficTypeName) {
+        // If the multiset has [{"user",2}.{"account",0}], elementSet only returns
+        // ["user"] (it ignores "account")
+        return Sets.newHashSet(_concurrentTrafficTypeNameSet.elementSet()).contains(trafficTypeName);
     }
 
     @Override
     public void clear() {
         _concurrentMap.clear();
+        _concurrentTrafficTypeNameSet.clear();
     }
 }

client/src/main/java/io/split/engine/cache/SplitCache.java

@@ -4,17 +4,16 @@
 
 import java.util.Collection;
 import java.util.List;
-import java.util.Map;
+import java.util.Set;
 
 public interface SplitCache {
     void put(ParsedSplit split);
-    void putAll(Map<String, ParsedSplit> splits);
     boolean remove(String name);
     ParsedSplit get(String name);
     Collection<ParsedSplit> getAll();
     Collection<ParsedSplit> getMany(List<String> names);
     long getChangeNumber();
     void setChangeNumber(long changeNumber);
-    boolean trafficTypeExists(String trafficType);
+    boolean trafficTypeExists(String trafficTypeName);
     void clear();
 }

client/src/main/java/io/split/engine/experiments/RefreshableSplitFetcher.java

@@ -1,11 +1,6 @@
 package io.split.engine.experiments;
 
-import com.google.common.collect.ConcurrentHashMultiset;
 import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
-import com.google.common.collect.Multiset;
-import com.google.common.collect.Multisets;
-import com.google.common.collect.Sets;
 import io.split.client.dtos.Split;
 import io.split.client.dtos.SplitChange;
 import io.split.client.dtos.Status;
@@ -16,8 +11,6 @@
 
 import java.util.Collection;
 import java.util.List;
-import java.util.Map;
-import java.util.Set;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
@@ -45,7 +38,6 @@ public class RefreshableSplitFetcher implements SplitFetcher, Runnable {
      * The count is used to maintain how many splits are using a traffic type, so when
      * an ARCHIVED split is received, we know if we need to remove a traffic type from the multiset.
      */
-    Multiset<String> _concurrentTrafficTypeNameSet = ConcurrentHashMultiset.create();
 
     public RefreshableSplitFetcher(SplitChangeFetcher splitChangeFetcher, SplitParser parser, SDKReadinessGates gates, SplitCache splitCache) {
         _splitChangeFetcher = checkNotNull(splitChangeFetcher);
@@ -111,11 +103,8 @@ public List<ParsedSplit> fetchAll() {
     }
 
     @Override
-    public Set<String> fetchKnownTrafficTypes() {
-        // We return the "keys" of the multiset that have a count greater than 0
-        // If the multiset has [{"user",2}.{"account",0}], elementSet only returns
-        // ["user"] (it ignores "account")
-        return Sets.newHashSet(_concurrentTrafficTypeNameSet.elementSet());
+    public boolean trafficTypeExists(String trafficTypeName) {
+        return _splitCache.trafficTypeExists(trafficTypeName);
     }
 
     public Collection<ParsedSplit> fetch() {
@@ -124,7 +113,6 @@ public Collection<ParsedSplit> fetch() {
 
     public void clear() {
         _splitCache.clear();
-        _concurrentTrafficTypeNameSet.clear();
     }
 
     @Override
@@ -180,68 +168,40 @@ public void runWithoutExceptionHandling() throws InterruptedException {
                 return;
             }
 
-            Set<String> toRemove = Sets.newHashSet();
-            Map<String, ParsedSplit> toAdd = Maps.newHashMap();
-            List<String> trafficTypeNamesToRemove = Lists.newArrayList();
-            List<String> trafficTypeNamesToAdd = Lists.newArrayList();
-
             for (Split split : change.splits) {
                 if (Thread.currentThread().isInterrupted()) {
                     throw new InterruptedException();
                 }
 
                 if (split.status != Status.ACTIVE) {
                     // archive.
-                    toRemove.add(split.name);
-                    if (split.trafficTypeName != null) {
-                        trafficTypeNamesToRemove.add(split.trafficTypeName);
-                    }
+                    _splitCache.remove(split.name);
                     continue;
                 }
 
                 ParsedSplit parsedSplit = _parser.parse(split);
                 if (parsedSplit == null) {
                     _log.info("We could not parse the experiment definition for: " + split.name + " so we are removing it completely to be careful");
-                    toRemove.add(split.name);
-                    if (split.trafficTypeName != null) {
-                        trafficTypeNamesToRemove.add(split.trafficTypeName);
-                    }
+
+                    _splitCache.remove(split.name);
+                    _log.debug("Deleted feature: " + split.name);
+
                     continue;
                 }
 
-                toAdd.put(split.name, parsedSplit);
-
                 // If the split already exists, this is either an update, or the split has been
                 // deleted and recreated (possibly with a different traffic type).
                 // If it's an update, the traffic type should NOT be increased.
                 // If it's deleted & recreated, the old one should be decreased and the new one increased.
                 // To handle both cases, we simply delete the old one if the split is present.
                 // The new one is always increased.
                 ParsedSplit current = _splitCache.get(split.name);
-                if (current != null && current.trafficTypeName() != null) {
-                    trafficTypeNamesToRemove.add(current.trafficTypeName());
-                }
-
-                if (split.trafficTypeName != null) {
-                    trafficTypeNamesToAdd.add(split.trafficTypeName);
+                if (current != null) {
+                    _splitCache.remove(split.name);
                 }
-            }
-
-            _splitCache.putAll(toAdd);
-            _concurrentTrafficTypeNameSet.addAll(trafficTypeNamesToAdd);
-            //removeAll does not work here, since it wont remove all the occurrences, just one
-            Multisets.removeOccurrences(_concurrentTrafficTypeNameSet, trafficTypeNamesToRemove);
-
-            for (String remove : toRemove) {
-                _splitCache.remove(remove);
-            }
-
-            if (!toAdd.isEmpty()) {
-                _log.debug("Updated features: " + toAdd.keySet());
-            }
 
-            if (!toRemove.isEmpty()) {
-                _log.debug("Deleted features: " + toRemove);
+                _splitCache.put(parsedSplit);
+                _log.debug("Updated feature: " + parsedSplit.feature());
             }
 
             _splitCache.setChangeNumber(change.till);

client/src/main/java/io/split/engine/experiments/SplitFetcher.java

@@ -1,7 +1,6 @@
 package io.split.engine.experiments;
 
 import java.util.List;
-import java.util.Set;
 
 /**
  * Created by adilaijaz on 5/8/15.
@@ -11,15 +10,7 @@ public interface SplitFetcher {
 
     List<ParsedSplit> fetchAll();
 
-    /**
-     * Fetches all the traffic types that are being used by the splits that are currently stored.
-     *
-     * For example, if the fetcher currently contains three splits, one of traffic type "account"
-     * and two of traffic type "user", this method will return ["account", "user"]
-     *
-     * @return a set of all the traffic types used by the parsed splits
-     */
-    Set<String> fetchKnownTrafficTypes();
+    boolean trafficTypeExists(String trafficTypeName);
 
     /**
      * Forces a sync of splits, outside of any scheduled

client/src/test/java/io/split/engine/cache/InMemoryCacheTest.java

@@ -0,0 +1,98 @@
+package io.split.engine.cache;
+
+import io.split.engine.experiments.ParsedSplit;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.*;
+
+public class InMemoryCacheTest {
+    private SplitCache _cache;
+
+    @Before
+    public void before() {
+        _cache = new InMemoryCacheImp();
+    }
+
+    @Test
+    public void putAndGetSplit() {
+        ParsedSplit split = getParsedSplit("split_name");
+        _cache.put(split);
+
+        ParsedSplit result = _cache.get("split_name");
+        Assert.assertNotNull(result);
+        Assert.assertEquals(split.changeNumber(), result.changeNumber());
+        Assert.assertEquals(split.trafficTypeName(), result.trafficTypeName());
+        Assert.assertEquals(split.defaultTreatment(), result.defaultTreatment());
+    }
+
+    @Test
+    public void putDuplicateSplit() {
+        ParsedSplit split = getParsedSplit("split_name");
+        ParsedSplit split2 = getParsedSplit("split_name");
+        _cache.put(split);
+        _cache.put(split2);
+
+        int result = _cache.getAll().size();
+
+        Assert.assertEquals(1, result);
+    }
+
+    @Test
+    public void getInExistentSplit() {
+        ParsedSplit split = getParsedSplit("split_name");
+        _cache.put(split);
+
+        ParsedSplit result = _cache.get("split_name_2");
+        Assert.assertNull(result);
+    }
+
+    @Test
+    public void removeSplit() {
+        ParsedSplit split = getParsedSplit("split_name");
+        ParsedSplit split2 = getParsedSplit("split_name_2");
+        _cache.put(split);
+        _cache.put(split2);
+
+        int result = _cache.getAll().size();
+        Assert.assertEquals(2, result);
+
+        _cache.remove("split_name");
+        result = _cache.getAll().size();
+        Assert.assertEquals(1, result);
+
+        Assert.assertNull(_cache.get("split_name"));
+    }
+
+    @Test
+    public void setAndGetChangeNumber() {
+        _cache.setChangeNumber(223);
+
+        long changeNumber = _cache.getChangeNumber();
+        Assert.assertEquals(223, changeNumber);
+
+        _cache.setChangeNumber(539);
+        changeNumber = _cache.getChangeNumber();
+        Assert.assertEquals(539, changeNumber);
+    }
+
+    @Test
+    public void getMany() {
+        _cache.put(getParsedSplit("split_name_1"));
+        _cache.put(getParsedSplit("split_name_2"));
+        _cache.put(getParsedSplit("split_name_3"));
+        _cache.put(getParsedSplit("split_name_4"));
+
+        List<String> names = new ArrayList<>();
+        names.add("split_name_2");
+        names.add("split_name_3");
+
+        Collection<ParsedSplit> result = _cache.getMany(names);
+        Assert.assertEquals(2, result.size());
+    }
+
+    private ParsedSplit getParsedSplit(String splitName) {
+        return ParsedSplit.createParsedSplitForTests(splitName, 0, false, "default_treatment", new ArrayList<>(), "tt", 123, 2);
+    }
+}