From 8fb76138bd4db93001cffe016718ba05027cd69a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Xavier=20L=C3=A9aut=C3=A9?= Date: Wed, 14 May 2014 21:52:31 -0700 Subject: [PATCH 1/2] tests must not assume findOvershadowed ordering --- .../VersionedIntervalTimelineTest.java | 59 ++++++++++++++----- 1 file changed, 45 insertions(+), 14 deletions(-) diff --git a/common/src/test/java/io/druid/timeline/VersionedIntervalTimelineTest.java b/common/src/test/java/io/druid/timeline/VersionedIntervalTimelineTest.java index 864eda882ce..012f55741a3 100644 --- a/common/src/test/java/io/druid/timeline/VersionedIntervalTimelineTest.java +++ b/common/src/test/java/io/druid/timeline/VersionedIntervalTimelineTest.java @@ -19,8 +19,11 @@ package io.druid.timeline; +import com.google.common.base.Function; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; import com.google.common.collect.Ordering; +import com.google.common.collect.Sets; import com.metamx.common.Pair; import io.druid.timeline.partition.ImmutablePartitionHolder; import io.druid.timeline.partition.IntegerPartitionChunk; @@ -38,6 +41,7 @@ import org.junit.Test; import java.util.Arrays; import java.util.Iterator; import java.util.List; +import java.util.Set; /** */ @@ -1104,13 +1108,13 @@ public class VersionedIntervalTimelineTest add("2011-01-01/2011-01-20", "3", 5); assertValues( - Arrays.asList( + Sets.newHashSet( createExpected("2011-01-02/2011-01-08", "2", 3), createExpected("2011-01-10/2011-01-16", "2", 4), createExpected("2011-01-03/2011-01-06", "1", 1), createExpected("2011-01-09/2011-01-12", "1", 2) ), - timeline.findOvershadowed() + Sets.newHashSet(timeline.findOvershadowed()) ); } @@ -1128,12 +1132,12 @@ public class VersionedIntervalTimelineTest add("2011-01-01/2011-01-10", "3", 4); assertValues( - Arrays.asList( + Sets.newHashSet( createExpected("2011-01-01/2011-01-05", "2", 1), createExpected("2011-01-05/2011-01-10", "2", 2), createExpected("2011-01-01/2011-01-10", "1", 3) ), - timeline.findOvershadowed() + Sets.newHashSet(timeline.findOvershadowed()) ); } @@ -1151,11 +1155,11 @@ public class VersionedIntervalTimelineTest add("2011-01-01/2011-01-10", "3", 4); assertValues( - Arrays.asList( + Sets.newHashSet( createExpected("2011-01-03/2011-01-12", "1", 3), createExpected("2011-01-01/2011-01-05", "2", 1) ), - timeline.findOvershadowed() + Sets.newHashSet(timeline.findOvershadowed()) ); } @@ -1342,12 +1346,12 @@ public class VersionedIntervalTimelineTest add("2011-04-01/2011-04-12", "2", 1); assertValues( - Arrays.asList( + Sets.newHashSet( createExpected("2011-04-01/2011-04-03", "1", 2), createExpected("2011-04-03/2011-04-06", "1", 3), createExpected("2011-04-09/2011-04-12", "1", 4) ), - timeline.findOvershadowed() + Sets.newHashSet(timeline.findOvershadowed()) ); } @@ -1444,11 +1448,11 @@ public class VersionedIntervalTimelineTest add("2011-04-03/2011-04-06", "1", 3); assertValues( - Arrays.asList( + Sets.newHashSet( createExpected("2011-04-03/2011-04-06", "1", 3), createExpected("2011-04-09/2011-04-12", "1", 3) ), - timeline.findOvershadowed() + Sets.newHashSet(timeline.findOvershadowed()) ); } @@ -1462,11 +1466,11 @@ public class VersionedIntervalTimelineTest add("2011-04-01/2011-04-09", "2", 3); assertValues( - Arrays.asList( + Sets.newHashSet( createExpected("2011-04-01/2011-04-09", "2", 3), createExpected("2011-04-01/2011-04-09", "1", 1) ), - timeline.findOvershadowed() + Sets.newHashSet(timeline.findOvershadowed()) ); } @@ -1481,11 +1485,11 @@ public class VersionedIntervalTimelineTest add("2011-04-01/2011-04-09", "9", 4); assertValues( - Arrays.asList( + Sets.newHashSet( createExpected("2011-04-01/2011-04-09", "2", 3), createExpected("2011-04-01/2011-04-09", "1", 1) ), - timeline.findOvershadowed() + Sets.newHashSet(timeline.findOvershadowed()) ); } @@ -1559,6 +1563,33 @@ public class VersionedIntervalTimelineTest } } + private void assertValues( + Set>>> expected, + Set> actual + ) + { + Assert.assertEquals("Sizes did not match.", expected.size(), actual.size()); + + Set>>> actualSet = + Sets.newHashSet( + Iterables.transform( + actual, + new Function, Pair>>>() + { + @Override + public Pair>> apply( + TimelineObjectHolder input + ) + { + return new Pair<>(input.getInterval(), new Pair<>(input.getVersion(), input.getObject())); + } + } + ) + ); + + Assert.assertEquals(expected, actualSet); + } + private VersionedIntervalTimeline makeStringIntegerTimeline() { return new VersionedIntervalTimeline(Ordering.natural()); From 088c60cf3dfeb24036ba5b3c507e54f60b794cf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Xavier=20L=C3=A9aut=C3=A9?= Date: Wed, 14 May 2014 22:45:28 -0700 Subject: [PATCH 2/2] always use ByteCountingLRUMap.remove() to remove --- .../java/io/druid/client/cache/ByteCountingLRUMap.java | 7 ++++--- server/src/main/java/io/druid/client/cache/MapCache.java | 8 +++++++- .../io/druid/client/cache/ByteCountingLRUMapTest.java | 8 +++++++- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/io/druid/client/cache/ByteCountingLRUMap.java b/server/src/main/java/io/druid/client/cache/ByteCountingLRUMap.java index ba91b290d16..5f358992a4b 100644 --- a/server/src/main/java/io/druid/client/cache/ByteCountingLRUMap.java +++ b/server/src/main/java/io/druid/client/cache/ByteCountingLRUMap.java @@ -22,6 +22,7 @@ package io.druid.client.cache; import com.metamx.common.logger.Logger; import java.nio.ByteBuffer; +import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; @@ -110,13 +111,13 @@ class ByteCountingLRUMap extends LinkedHashMap } /** - * We want keySet().iterator().remove() to account for object removal - * The underlying Map calls this.remove(key) so we do not need to override this + * Don't allow key removal using the underlying keySet iterator + * All removal operations must use ByteCountingLRUMap.remove() */ @Override public Set keySet() { - return super.keySet(); + return Collections.unmodifiableSet(super.keySet()); } @Override diff --git a/server/src/main/java/io/druid/client/cache/MapCache.java b/server/src/main/java/io/druid/client/cache/MapCache.java index 5221fd0ca91..4d26d135ce4 100644 --- a/server/src/main/java/io/druid/client/cache/MapCache.java +++ b/server/src/main/java/io/druid/client/cache/MapCache.java @@ -19,12 +19,14 @@ package io.druid.client.cache; +import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.primitives.Ints; import java.nio.ByteBuffer; import java.util.Collections; import java.util.Iterator; +import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; @@ -121,6 +123,7 @@ public class MapCache implements Cache } synchronized (clearLock) { Iterator iter = baseMap.keySet().iterator(); + List toRemove = Lists.newLinkedList(); while (iter.hasNext()) { ByteBuffer next = iter.next(); @@ -128,9 +131,12 @@ public class MapCache implements Cache && next.get(1) == idBytes[1] && next.get(2) == idBytes[2] && next.get(3) == idBytes[3]) { - iter.remove(); + toRemove.add(next); } } + for(ByteBuffer key : toRemove) { + baseMap.remove(key); + } } } diff --git a/server/src/test/java/io/druid/client/cache/ByteCountingLRUMapTest.java b/server/src/test/java/io/druid/client/cache/ByteCountingLRUMapTest.java index 4a7143d3efa..c1003c8cb7c 100644 --- a/server/src/test/java/io/druid/client/cache/ByteCountingLRUMapTest.java +++ b/server/src/test/java/io/druid/client/cache/ByteCountingLRUMapTest.java @@ -19,12 +19,14 @@ package io.druid.client.cache; +import com.google.common.collect.Lists; import org.junit.Assert; import org.junit.Before; import org.junit.Test; import java.nio.ByteBuffer; import java.util.Iterator; +import java.util.List; /** */ @@ -68,12 +70,16 @@ public class ByteCountingLRUMapTest Assert.assertEquals(oneByte, ByteBuffer.wrap(map.get(twoByte))); Iterator it = map.keySet().iterator(); + List toRemove = Lists.newLinkedList(); while(it.hasNext()) { ByteBuffer buf = it.next(); if(buf.remaining() == 10) { - it.remove(); + toRemove.add(buf); } } + for(ByteBuffer buf : toRemove) { + map.remove(buf); + } assertMapValues(1, 3, 2); map.remove(twoByte);