From d006f2c120b6a734c549bd3e73895c787d40bb7c Mon Sep 17 00:00:00 2001 From: Martin Redolatti Date: Thu, 3 Sep 2020 14:46:56 -0300 Subject: [PATCH 1/4] update observer and add counter --- .../client/impressions/ImpressionCounter.java | 48 ++++++++ .../impressions/ImpressionObserver.java | 6 +- .../impressions/ImpressionCounterTest.java | 112 ++++++++++++++++++ .../impressions/ImpressionObserverTest.java | 42 ++++++- 4 files changed, 203 insertions(+), 5 deletions(-) create mode 100644 client/src/main/java/io/split/client/impressions/ImpressionCounter.java create mode 100644 client/src/test/java/io/split/client/impressions/ImpressionCounterTest.java diff --git a/client/src/main/java/io/split/client/impressions/ImpressionCounter.java b/client/src/main/java/io/split/client/impressions/ImpressionCounter.java new file mode 100644 index 000000000..047dcdaf7 --- /dev/null +++ b/client/src/main/java/io/split/client/impressions/ImpressionCounter.java @@ -0,0 +1,48 @@ +package io.split.client.impressions; + +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; + +public class ImpressionCounter { + + private static final long TIME_INTERVAL_MS = 3600 * 1000; + + private final ConcurrentHashMap _counts; + + public ImpressionCounter() { + _counts = new ConcurrentHashMap<>(); + } + + public void inc(String featureName, long timeFrame, int amount) { + String key = makeKey(featureName, timeFrame); + AtomicInteger count = _counts.get(key); + if (Objects.isNull(count)) { + count = new AtomicInteger(); + AtomicInteger old = _counts.putIfAbsent(key, count); + if (!Objects.isNull(old)) { // Some other thread won the race, use that AtomicInteger instead + count = old; + } + } + count.addAndGet(amount); + } + + public HashMap popAll() { + HashMap toReturn = new HashMap<>(); + for (String key : _counts.keySet()) { + AtomicInteger curr = _counts.remove(key); + toReturn.put(key ,curr.get()); + } + return toReturn; + } + + static String makeKey(String featureName, long timeFrame) { + return String.join("::", featureName, String.valueOf(truncateTimeframe(timeFrame))); + } + + static long truncateTimeframe(long timestampInMs) { + return timestampInMs - (timestampInMs % TIME_INTERVAL_MS); + } +} diff --git a/client/src/main/java/io/split/client/impressions/ImpressionObserver.java b/client/src/main/java/io/split/client/impressions/ImpressionObserver.java index b85a91da7..38164a445 100644 --- a/client/src/main/java/io/split/client/impressions/ImpressionObserver.java +++ b/client/src/main/java/io/split/client/impressions/ImpressionObserver.java @@ -3,7 +3,8 @@ import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; import io.split.client.dtos.KeyImpression; -import org.apache.http.annotation.NotThreadSafe; + +import java.util.Objects; /* According to guava's docs (https://guava.dev/releases/18.0/api/docs/com/google/common/annotations/Beta.html), @@ -13,7 +14,6 @@ */ @SuppressWarnings("UnstableApiUsage") -@NotThreadSafe public class ImpressionObserver { private final Cache _cache; @@ -33,6 +33,6 @@ public Long testAndSet(KeyImpression impression) { Long hash = ImpressionHasher.process(impression); Long previous = _cache.getIfPresent(hash); _cache.put(hash, impression.time); - return previous; + return (Objects.isNull(previous)) ? null : Math.min(previous, impression.time); } } \ No newline at end of file diff --git a/client/src/test/java/io/split/client/impressions/ImpressionCounterTest.java b/client/src/test/java/io/split/client/impressions/ImpressionCounterTest.java new file mode 100644 index 000000000..f5f8266da --- /dev/null +++ b/client/src/test/java/io/split/client/impressions/ImpressionCounterTest.java @@ -0,0 +1,112 @@ +package io.split.client.impressions; + +import org.junit.Test; + +import java.util.Calendar; +import java.util.Date; +import java.util.HashMap; +import java.util.Map; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.core.IsEqual.equalTo; + +public class ImpressionCounterTest { + + @Test + public void testTruncateTimeFrame() { + assertThat(ImpressionCounter.truncateTimeframe(new Date(2020, Calendar.SEPTEMBER, 2, 10, 53, 12).getTime()), + is(equalTo(new Date(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTime()))); + assertThat(ImpressionCounter.truncateTimeframe(new Date(2020, Calendar.SEPTEMBER, 2, 10, 00, 00).getTime()), + is(equalTo(new Date(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTime()))); + assertThat(ImpressionCounter.truncateTimeframe(new Date(2020, Calendar.SEPTEMBER, 2, 10, 53, 00).getTime()), + is(equalTo(new Date(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTime()))); + assertThat(ImpressionCounter.truncateTimeframe(new Date(2020, Calendar.SEPTEMBER, 2, 10, 00, 12).getTime()), + is(equalTo(new Date(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTime()))); + assertThat(ImpressionCounter.truncateTimeframe(new Date(1970, Calendar.JANUARY, 0, 0, 0, 0).getTime()), + is(equalTo(new Date(1970, Calendar.JANUARY, 0, 0, 0, 0).getTime()))); + } + + @Test + public void testMakeKey() { + assertThat(ImpressionCounter.makeKey("someFeature", new Date(2020, Calendar.SEPTEMBER, 2, 10, 5, 23).getTime()), + is(equalTo("someFeature::61557195600000"))); + assertThat(ImpressionCounter.makeKey("", new Date(2020, Calendar.SEPTEMBER, 2, 10, 5, 23).getTime()), + is(equalTo("::61557195600000"))); + assertThat(ImpressionCounter.makeKey(null, new Date(2020, Calendar.SEPTEMBER, 2, 10, 5, 23).getTime()), + is(equalTo("null::61557195600000"))); + assertThat(ImpressionCounter.makeKey(null, 0L), is(equalTo("null::0"))); + } + + @Test + public void testBasicUsage() { + final ImpressionCounter counter = new ImpressionCounter(); + final long timestamp = new Date(2020, Calendar.SEPTEMBER, 2, 10, 10, 12).getTime(); + counter.inc("feature1", timestamp, 1); + counter.inc("feature1", timestamp + 1, 1); + counter.inc("feature1", timestamp + 2, 1); + counter.inc("feature2", timestamp + 3, 2); + counter.inc("feature2", timestamp + 4, 2); + Map counted = counter.popAll(); + assertThat(counted.size(), is(equalTo(2))); + assertThat(counted.get(ImpressionCounter.makeKey("feature1", timestamp)), is(equalTo(3))); + assertThat(counted.get(ImpressionCounter.makeKey("feature2", timestamp)), is(equalTo(4))); + assertThat(counter.popAll().size(), is(equalTo(0))); + + final long nextHourTimestamp = new Date(2020, Calendar.SEPTEMBER, 2, 11, 10, 12).getTime(); + counter.inc("feature1", timestamp, 1); + counter.inc("feature1", timestamp + 1, 1); + counter.inc("feature1", timestamp + 2, 1); + counter.inc("feature2", timestamp + 3, 2); + counter.inc("feature2", timestamp + 4, 2); + counter.inc("feature1", nextHourTimestamp, 1); + counter.inc("feature1", nextHourTimestamp + 1, 1); + counter.inc("feature1", nextHourTimestamp + 2, 1); + counter.inc("feature2", nextHourTimestamp + 3, 2); + counter.inc("feature2", nextHourTimestamp + 4, 2); + counted = counter.popAll(); + assertThat(counted.size(), is(equalTo(4))); + assertThat(counted.get(ImpressionCounter.makeKey("feature1", timestamp)), is(equalTo(3))); + assertThat(counted.get(ImpressionCounter.makeKey("feature2", timestamp)), is(equalTo(4))); + assertThat(counted.get(ImpressionCounter.makeKey("feature1", nextHourTimestamp)), is(equalTo(3))); + assertThat(counted.get(ImpressionCounter.makeKey("feature2", nextHourTimestamp)), is(equalTo(4))); + assertThat(counter.popAll().size(), is(equalTo(0))); + } + + @Test + public void manyConcurrentCalls() throws InterruptedException { + final int iterations = 10000000; + final long timestamp = new Date(2020, Calendar.SEPTEMBER, 2, 10, 10, 12).getTime(); + final long nextHourTimestamp = new Date(2020, Calendar.SEPTEMBER, 2, 11, 10, 12).getTime(); + ImpressionCounter counter = new ImpressionCounter(); + Thread t1 = new Thread(() -> { + int times = iterations; + while (times-- > 0) { + counter.inc("feature1", timestamp, 1); + counter.inc("feature2", timestamp, 1); + counter.inc("feature1", nextHourTimestamp, 2); + counter.inc("feature2", nextHourTimestamp, 2); + } + }); + Thread t2 = new Thread(() -> { + int times = iterations; + while (times-- > 0) { + counter.inc("feature1", timestamp, 2); + counter.inc("feature2", timestamp, 2); + counter.inc("feature1", nextHourTimestamp, 1); + counter.inc("feature2", nextHourTimestamp, 1); + } + }); + + t1.setDaemon(true); t2.setDaemon(true); + t1.start(); t2.start(); + t1.join(); t2.join(); + + HashMap counted = counter.popAll(); + assertThat(counted.size(), is(equalTo(4))); + assertThat(counted.get(ImpressionCounter.makeKey("feature1", timestamp)), is(equalTo(iterations * 3))); + assertThat(counted.get(ImpressionCounter.makeKey("feature2", timestamp)), is(equalTo(iterations * 3))); + assertThat(counted.get(ImpressionCounter.makeKey("feature1", nextHourTimestamp)), is(equalTo(iterations * 3))); + assertThat(counted.get(ImpressionCounter.makeKey("feature2", nextHourTimestamp)), is(equalTo(iterations * 3))); + } +} diff --git a/client/src/test/java/io/split/client/impressions/ImpressionObserverTest.java b/client/src/test/java/io/split/client/impressions/ImpressionObserverTest.java index 7ef190391..c4930ae8c 100644 --- a/client/src/test/java/io/split/client/impressions/ImpressionObserverTest.java +++ b/client/src/test/java/io/split/client/impressions/ImpressionObserverTest.java @@ -11,11 +11,13 @@ import java.lang.reflect.Method; import java.util.ArrayList; import java.util.List; +import java.util.Random; +import java.util.concurrent.ConcurrentLinkedQueue; -import static org.hamcrest.Matchers.lessThan; -import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.*; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; +import static org.mockito.AdditionalMatchers.or; public class ImpressionObserverTest { @@ -92,6 +94,42 @@ public void testMemoryUsageStopsWhenCacheIsFull() throws Exception { long sizeAfterSecondPopulation = (long) getObjectSize.invoke(null, observer); assertThat((double) (sizeAfterSecondPopulation - sizeAfterInitialPopulation), lessThan (SIZE_DELTA * sizeAfterInitialPopulation)); + } + + + private void caller(ImpressionObserver o, int count, ConcurrentLinkedQueue imps) { + Random rand = new Random(); + while (count-- > 0) { + KeyImpression k = new KeyImpression(); + k.keyName = "key_" + rand.nextInt(100); + k.feature = "feature_" + rand.nextInt(10); + k.label = "label" + rand.nextInt(5); + k.treatment = rand.nextBoolean() ? "on" : "off"; + k.changeNumber = 1234567L; + k.time = System.currentTimeMillis(); + k.pt = o.testAndSet(k); + imps.offer(k); + } + } + @Test + public void testConcurrencyVsAccuracy() throws InterruptedException { + ImpressionObserver observer = new ImpressionObserver(500000); + ConcurrentLinkedQueue imps = new ConcurrentLinkedQueue<>(); + Thread t1 = new Thread(() -> caller(observer, 1000000, imps)); + Thread t2 = new Thread(() -> caller(observer, 1000000, imps)); + Thread t3 = new Thread(() -> caller(observer, 1000000, imps)); + Thread t4 = new Thread(() -> caller(observer, 1000000, imps)); + Thread t5 = new Thread(() -> caller(observer, 1000000, imps)); + + // start the 5 threads an wait for them to finish. + t1.setDaemon(true); t2.setDaemon(true); t3.setDaemon(true); t4.setDaemon(true); t5.setDaemon(true); + t1.start(); t2.start(); t3.start(); t4.start(); t5.start(); + t1.join(); t2.join(); t3.join(); t4.join(); t5.join(); + + assertThat(imps.size(), is(equalTo(5000000))); + for (KeyImpression i : imps) { + assertThat(i.pt, is(anyOf(nullValue(), lessThanOrEqualTo(i.time)))); + } } } From 289a96a79cde52616a3a5e85ae5f90b4a9e8bc02 Mon Sep 17 00:00:00 2001 From: Martin Redolatti Date: Thu, 3 Sep 2020 15:49:29 -0300 Subject: [PATCH 2/4] fix imports and use GregorianCalendar instead of deprecated Date --- .../impressions/ImpressionCounterTest.java | 42 +++++++++---------- .../impressions/ImpressionObserverTest.java | 12 +++--- 2 files changed, 28 insertions(+), 26 deletions(-) diff --git a/client/src/test/java/io/split/client/impressions/ImpressionCounterTest.java b/client/src/test/java/io/split/client/impressions/ImpressionCounterTest.java index f5f8266da..49deaafbe 100644 --- a/client/src/test/java/io/split/client/impressions/ImpressionCounterTest.java +++ b/client/src/test/java/io/split/client/impressions/ImpressionCounterTest.java @@ -3,7 +3,7 @@ import org.junit.Test; import java.util.Calendar; -import java.util.Date; +import java.util.GregorianCalendar; import java.util.HashMap; import java.util.Map; @@ -15,33 +15,33 @@ public class ImpressionCounterTest { @Test public void testTruncateTimeFrame() { - assertThat(ImpressionCounter.truncateTimeframe(new Date(2020, Calendar.SEPTEMBER, 2, 10, 53, 12).getTime()), - is(equalTo(new Date(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTime()))); - assertThat(ImpressionCounter.truncateTimeframe(new Date(2020, Calendar.SEPTEMBER, 2, 10, 00, 00).getTime()), - is(equalTo(new Date(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTime()))); - assertThat(ImpressionCounter.truncateTimeframe(new Date(2020, Calendar.SEPTEMBER, 2, 10, 53, 00).getTime()), - is(equalTo(new Date(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTime()))); - assertThat(ImpressionCounter.truncateTimeframe(new Date(2020, Calendar.SEPTEMBER, 2, 10, 00, 12).getTime()), - is(equalTo(new Date(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTime()))); - assertThat(ImpressionCounter.truncateTimeframe(new Date(1970, Calendar.JANUARY, 0, 0, 0, 0).getTime()), - is(equalTo(new Date(1970, Calendar.JANUARY, 0, 0, 0, 0).getTime()))); + assertThat(ImpressionCounter.truncateTimeframe(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 53, 12).getTimeInMillis()), + is(equalTo(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTimeInMillis()))); + assertThat(ImpressionCounter.truncateTimeframe(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTimeInMillis()), + is(equalTo(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTimeInMillis()))); + assertThat(ImpressionCounter.truncateTimeframe(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 53, 0 ).getTimeInMillis()), + is(equalTo(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTimeInMillis()))); + assertThat(ImpressionCounter.truncateTimeframe(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 0, 12).getTimeInMillis()), + is(equalTo(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTimeInMillis()))); + assertThat(ImpressionCounter.truncateTimeframe(new GregorianCalendar(1970, Calendar.JANUARY, 0, 0, 0, 0).getTimeInMillis()), + is(equalTo(new GregorianCalendar(1970, Calendar.JANUARY, 0, 0, 0, 0).getTimeInMillis()))); } @Test public void testMakeKey() { - assertThat(ImpressionCounter.makeKey("someFeature", new Date(2020, Calendar.SEPTEMBER, 2, 10, 5, 23).getTime()), - is(equalTo("someFeature::61557195600000"))); - assertThat(ImpressionCounter.makeKey("", new Date(2020, Calendar.SEPTEMBER, 2, 10, 5, 23).getTime()), - is(equalTo("::61557195600000"))); - assertThat(ImpressionCounter.makeKey(null, new Date(2020, Calendar.SEPTEMBER, 2, 10, 5, 23).getTime()), - is(equalTo("null::61557195600000"))); + assertThat(ImpressionCounter.makeKey("someFeature", new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 5, 23).getTimeInMillis()), + is(equalTo("someFeature::1599051600000"))); + assertThat(ImpressionCounter.makeKey("", new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 5, 23).getTimeInMillis()), + is(equalTo("::1599051600000"))); + assertThat(ImpressionCounter.makeKey(null, new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 5, 23).getTimeInMillis()), + is(equalTo("null::1599051600000"))); assertThat(ImpressionCounter.makeKey(null, 0L), is(equalTo("null::0"))); } @Test public void testBasicUsage() { final ImpressionCounter counter = new ImpressionCounter(); - final long timestamp = new Date(2020, Calendar.SEPTEMBER, 2, 10, 10, 12).getTime(); + final long timestamp = new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 10, 12).getTimeInMillis(); counter.inc("feature1", timestamp, 1); counter.inc("feature1", timestamp + 1, 1); counter.inc("feature1", timestamp + 2, 1); @@ -53,7 +53,7 @@ public void testBasicUsage() { assertThat(counted.get(ImpressionCounter.makeKey("feature2", timestamp)), is(equalTo(4))); assertThat(counter.popAll().size(), is(equalTo(0))); - final long nextHourTimestamp = new Date(2020, Calendar.SEPTEMBER, 2, 11, 10, 12).getTime(); + final long nextHourTimestamp = new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 11, 10, 12).getTimeInMillis(); counter.inc("feature1", timestamp, 1); counter.inc("feature1", timestamp + 1, 1); counter.inc("feature1", timestamp + 2, 1); @@ -76,8 +76,8 @@ public void testBasicUsage() { @Test public void manyConcurrentCalls() throws InterruptedException { final int iterations = 10000000; - final long timestamp = new Date(2020, Calendar.SEPTEMBER, 2, 10, 10, 12).getTime(); - final long nextHourTimestamp = new Date(2020, Calendar.SEPTEMBER, 2, 11, 10, 12).getTime(); + final long timestamp = new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 10, 12).getTimeInMillis(); + final long nextHourTimestamp = new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 11, 10, 12).getTimeInMillis(); ImpressionCounter counter = new ImpressionCounter(); Thread t1 = new Thread(() -> { int times = iterations; diff --git a/client/src/test/java/io/split/client/impressions/ImpressionObserverTest.java b/client/src/test/java/io/split/client/impressions/ImpressionObserverTest.java index c4930ae8c..d3a049772 100644 --- a/client/src/test/java/io/split/client/impressions/ImpressionObserverTest.java +++ b/client/src/test/java/io/split/client/impressions/ImpressionObserverTest.java @@ -2,22 +2,24 @@ import com.google.common.base.Strings; import io.split.client.dtos.KeyImpression; -// import jdk.nashorn.internal.ir.debug.ObjectSizeCalculator; import org.junit.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.ArrayList; import java.util.List; import java.util.Random; import java.util.concurrent.ConcurrentLinkedQueue; -import static org.hamcrest.Matchers.*; +import static org.hamcrest.CoreMatchers.nullValue; +import static org.hamcrest.Matchers.lessThan; +import static org.hamcrest.Matchers.lessThanOrEqualTo; +import static org.hamcrest.core.AnyOf.anyOf; +import static org.hamcrest.core.Is.is; +import static org.hamcrest.core.IsEqual.equalTo; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; -import static org.mockito.AdditionalMatchers.or; public class ImpressionObserverTest { @@ -93,7 +95,7 @@ public void testMemoryUsageStopsWhenCacheIsFull() throws Exception { long sizeAfterSecondPopulation = (long) getObjectSize.invoke(null, observer); - assertThat((double) (sizeAfterSecondPopulation - sizeAfterInitialPopulation), lessThan (SIZE_DELTA * sizeAfterInitialPopulation)); + assertThat((double) (sizeAfterSecondPopulation - sizeAfterInitialPopulation), lessThan(SIZE_DELTA * sizeAfterInitialPopulation)); } From ec0e0a2cdd009b7d80cdcb79897296566e47039f Mon Sep 17 00:00:00 2001 From: Martin Redolatti Date: Thu, 3 Sep 2020 17:21:14 -0300 Subject: [PATCH 3/4] fix timestamps --- .../client/impressions/ImpressionCounter.java | 1 - .../impressions/ImpressionCounterTest.java | 49 ++++++++++--------- 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/client/src/main/java/io/split/client/impressions/ImpressionCounter.java b/client/src/main/java/io/split/client/impressions/ImpressionCounter.java index 047dcdaf7..1787115cf 100644 --- a/client/src/main/java/io/split/client/impressions/ImpressionCounter.java +++ b/client/src/main/java/io/split/client/impressions/ImpressionCounter.java @@ -1,7 +1,6 @@ package io.split.client.impressions; import java.util.HashMap; -import java.util.Map; import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; diff --git a/client/src/test/java/io/split/client/impressions/ImpressionCounterTest.java b/client/src/test/java/io/split/client/impressions/ImpressionCounterTest.java index 49deaafbe..a25c4e78e 100644 --- a/client/src/test/java/io/split/client/impressions/ImpressionCounterTest.java +++ b/client/src/test/java/io/split/client/impressions/ImpressionCounterTest.java @@ -2,8 +2,8 @@ import org.junit.Test; -import java.util.Calendar; -import java.util.GregorianCalendar; +import java.time.ZoneId; +import java.time.ZonedDateTime; import java.util.HashMap; import java.util.Map; @@ -13,35 +13,40 @@ public class ImpressionCounterTest { + private long makeTimestamp(int year, int month, int day, int hour, int minute, int second) { + return ZonedDateTime.of(year, month, day, hour, minute, second, 0, ZoneId.of("UTC")).toInstant().toEpochMilli(); + } + @Test public void testTruncateTimeFrame() { - assertThat(ImpressionCounter.truncateTimeframe(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 53, 12).getTimeInMillis()), - is(equalTo(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTimeInMillis()))); - assertThat(ImpressionCounter.truncateTimeframe(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTimeInMillis()), - is(equalTo(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTimeInMillis()))); - assertThat(ImpressionCounter.truncateTimeframe(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 53, 0 ).getTimeInMillis()), - is(equalTo(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTimeInMillis()))); - assertThat(ImpressionCounter.truncateTimeframe(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 0, 12).getTimeInMillis()), - is(equalTo(new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 0, 0).getTimeInMillis()))); - assertThat(ImpressionCounter.truncateTimeframe(new GregorianCalendar(1970, Calendar.JANUARY, 0, 0, 0, 0).getTimeInMillis()), - is(equalTo(new GregorianCalendar(1970, Calendar.JANUARY, 0, 0, 0, 0).getTimeInMillis()))); + assertThat(ImpressionCounter.truncateTimeframe(makeTimestamp(2020, 9, 2, 10, 53, 12)), + is(equalTo(makeTimestamp(2020, 9, 2, 10, 0, 0)))); + assertThat(ImpressionCounter.truncateTimeframe(makeTimestamp(2020, 9, 2, 10, 0, 0)), + is(equalTo(makeTimestamp(2020, 9, 2, 10, 0, 0)))); + assertThat(ImpressionCounter.truncateTimeframe(makeTimestamp(2020, 9, 2, 10, 53, 0 )), + is(equalTo(makeTimestamp(2020, 9, 2, 10, 0, 0)))); + assertThat(ImpressionCounter.truncateTimeframe(makeTimestamp(2020, 9, 2, 10, 0, 12)), + is(equalTo(makeTimestamp(2020, 9, 2, 10, 0, 0)))); + assertThat(ImpressionCounter.truncateTimeframe(makeTimestamp(1970, 1, 1, 0, 0, 0)), + is(equalTo(makeTimestamp(1970, 1, 1, 0, 0, 0)))); } @Test public void testMakeKey() { - assertThat(ImpressionCounter.makeKey("someFeature", new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 5, 23).getTimeInMillis()), - is(equalTo("someFeature::1599051600000"))); - assertThat(ImpressionCounter.makeKey("", new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 5, 23).getTimeInMillis()), - is(equalTo("::1599051600000"))); - assertThat(ImpressionCounter.makeKey(null, new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 5, 23).getTimeInMillis()), - is(equalTo("null::1599051600000"))); + long targetTZ = makeTimestamp(2020, 9, 2, 10, 0, 0); + assertThat(ImpressionCounter.makeKey("someFeature", makeTimestamp(2020, 9, 2, 10, 5, 23)), + is(equalTo("someFeature::" + targetTZ))); + assertThat(ImpressionCounter.makeKey("", makeTimestamp(2020, 9, 2, 10, 5, 23)), + is(equalTo("::" + targetTZ))); + assertThat(ImpressionCounter.makeKey(null, makeTimestamp(2020, 9, 2, 10, 5, 23)), + is(equalTo("null::" + targetTZ))); assertThat(ImpressionCounter.makeKey(null, 0L), is(equalTo("null::0"))); } @Test public void testBasicUsage() { final ImpressionCounter counter = new ImpressionCounter(); - final long timestamp = new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 10, 12).getTimeInMillis(); + final long timestamp = makeTimestamp(2020, 9, 2, 10, 10, 12); counter.inc("feature1", timestamp, 1); counter.inc("feature1", timestamp + 1, 1); counter.inc("feature1", timestamp + 2, 1); @@ -53,7 +58,7 @@ public void testBasicUsage() { assertThat(counted.get(ImpressionCounter.makeKey("feature2", timestamp)), is(equalTo(4))); assertThat(counter.popAll().size(), is(equalTo(0))); - final long nextHourTimestamp = new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 11, 10, 12).getTimeInMillis(); + final long nextHourTimestamp = makeTimestamp(2020, 9, 2, 11, 10, 12); counter.inc("feature1", timestamp, 1); counter.inc("feature1", timestamp + 1, 1); counter.inc("feature1", timestamp + 2, 1); @@ -76,8 +81,8 @@ public void testBasicUsage() { @Test public void manyConcurrentCalls() throws InterruptedException { final int iterations = 10000000; - final long timestamp = new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 10, 10, 12).getTimeInMillis(); - final long nextHourTimestamp = new GregorianCalendar(2020, Calendar.SEPTEMBER, 2, 11, 10, 12).getTimeInMillis(); + final long timestamp = makeTimestamp(2020, 9, 2, 10, 10, 12); + final long nextHourTimestamp = makeTimestamp(2020, 9, 2, 11, 10, 12); ImpressionCounter counter = new ImpressionCounter(); Thread t1 = new Thread(() -> { int times = iterations; From 09522cbace373e3adf72ced26056b3e78d20e143 Mon Sep 17 00:00:00 2001 From: Martin Redolatti Date: Thu, 3 Sep 2020 19:27:42 -0300 Subject: [PATCH 4/4] fix sonarqube comments --- .../split/client/impressions/ImpressionCounter.java | 2 +- .../client/impressions/ImpressionObserverTest.java | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/client/src/main/java/io/split/client/impressions/ImpressionCounter.java b/client/src/main/java/io/split/client/impressions/ImpressionCounter.java index 1787115cf..09bfdb26b 100644 --- a/client/src/main/java/io/split/client/impressions/ImpressionCounter.java +++ b/client/src/main/java/io/split/client/impressions/ImpressionCounter.java @@ -7,7 +7,7 @@ public class ImpressionCounter { - private static final long TIME_INTERVAL_MS = 3600 * 1000; + private static final long TIME_INTERVAL_MS = 3600L * 1000L; private final ConcurrentHashMap _counts; diff --git a/client/src/test/java/io/split/client/impressions/ImpressionObserverTest.java b/client/src/test/java/io/split/client/impressions/ImpressionObserverTest.java index d3a049772..599d32797 100644 --- a/client/src/test/java/io/split/client/impressions/ImpressionObserverTest.java +++ b/client/src/test/java/io/split/client/impressions/ImpressionObserverTest.java @@ -28,6 +28,7 @@ public class ImpressionObserverTest { // We allow the cache implementation to have a 0.01% drift in size when elements change, given that it's internal // structure/references might vary, and the ObjectSizeCalculator is not 100% accurate private static final double SIZE_DELTA = 0.01; + private final Random _rand = new Random(); private List generateKeyImpressions(long count) { ArrayList imps = new ArrayList<>(); @@ -100,13 +101,13 @@ public void testMemoryUsageStopsWhenCacheIsFull() throws Exception { private void caller(ImpressionObserver o, int count, ConcurrentLinkedQueue imps) { - Random rand = new Random(); + while (count-- > 0) { KeyImpression k = new KeyImpression(); - k.keyName = "key_" + rand.nextInt(100); - k.feature = "feature_" + rand.nextInt(10); - k.label = "label" + rand.nextInt(5); - k.treatment = rand.nextBoolean() ? "on" : "off"; + k.keyName = "key_" + _rand.nextInt(100); + k.feature = "feature_" + _rand.nextInt(10); + k.label = "label" + _rand.nextInt(5); + k.treatment = _rand.nextBoolean() ? "on" : "off"; k.changeNumber = 1234567L; k.time = System.currentTimeMillis(); k.pt = o.testAndSet(k);