dcapwell commented on code in PR #2256: URL: https://github.com/apache/cassandra/pull/2256#discussion_r1163399051
########## test/unit/org/apache/cassandra/journal/IndexTest.java: ########## @@ -0,0 +1,133 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.journal; + +import java.io.IOException; +import java.nio.file.Files; + +import org.junit.Test; + +import org.apache.cassandra.io.util.File; +import org.apache.cassandra.utils.TimeUUID; + +import static org.apache.cassandra.utils.TimeUUID.Generator.nextTimeUUID; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class IndexTest +{ + private static final int[] EMPTY = new int[0]; + + @Test + public void testInMemoryIndexBasics() + { + InMemoryIndex<TimeUUID> index = InMemoryIndex.create(TimeUUIDKeySupport.INSTANCE); + + TimeUUID key0 = nextTimeUUID(); + TimeUUID key1 = nextTimeUUID(); + TimeUUID key2 = nextTimeUUID(); + TimeUUID key3 = nextTimeUUID(); + TimeUUID key4 = nextTimeUUID(); + + assertEquals(0, index.lookUp(key0).length); + assertEquals(0, index.lookUp(key1).length); + assertEquals(0, index.lookUp(key2).length); + assertEquals(0, index.lookUp(key3).length); + assertEquals(0, index.lookUp(key4).length); Review Comment: personal preference: I prefer `assertThat(index.lookUp(key0)).isEmpty();` as the error msg is more informative example failure ``` Expecting empty but was:<[1100]> ``` This shows the contents of the return, which can be useful for debugging. ########## test/unit/org/apache/cassandra/journal/IndexTest.java: ########## @@ -0,0 +1,133 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.journal; + +import java.io.IOException; +import java.nio.file.Files; + +import org.junit.Test; + +import org.apache.cassandra.io.util.File; +import org.apache.cassandra.utils.TimeUUID; + +import static org.apache.cassandra.utils.TimeUUID.Generator.nextTimeUUID; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class IndexTest +{ + private static final int[] EMPTY = new int[0]; + + @Test + public void testInMemoryIndexBasics() + { + InMemoryIndex<TimeUUID> index = InMemoryIndex.create(TimeUUIDKeySupport.INSTANCE); + + TimeUUID key0 = nextTimeUUID(); + TimeUUID key1 = nextTimeUUID(); + TimeUUID key2 = nextTimeUUID(); + TimeUUID key3 = nextTimeUUID(); + TimeUUID key4 = nextTimeUUID(); + + assertEquals(0, index.lookUp(key0).length); + assertEquals(0, index.lookUp(key1).length); + assertEquals(0, index.lookUp(key2).length); + assertEquals(0, index.lookUp(key3).length); + assertEquals(0, index.lookUp(key4).length); + + int val11 = 1100; + int val21 = 2100; + int val22 = 2200; + int val31 = 3100; + int val32 = 3200; + int val33 = 3300; + + index.update(key1, val11); + index.update(key2, val21); + index.update(key2, val22); + index.update(key3, val31); + index.update(key3, val32); + index.update(key3, val33); + + assertArrayEquals(EMPTY, index.lookUp(key0)); + assertArrayEquals(new int[] { val11 }, index.lookUp(key1)); + assertArrayEquals(new int[] { val21, val22 }, index.lookUp(key2)); + assertArrayEquals(new int[] { val31, val32, val33 }, index.lookUp(key3)); + assertArrayEquals(EMPTY, index.lookUp(key4)); + + assertEquals(key1, index.firstId()); + assertEquals(key3, index.lastId()); + + assertFalse(index.mayContainId(key0, TimeUUIDKeySupport.INSTANCE)); + assertTrue(index.mayContainId(key1, TimeUUIDKeySupport.INSTANCE)); + assertTrue(index.mayContainId(key2, TimeUUIDKeySupport.INSTANCE)); + assertTrue(index.mayContainId(key3, TimeUUIDKeySupport.INSTANCE)); + assertFalse(index.mayContainId(key4, TimeUUIDKeySupport.INSTANCE)); + } + + @Test + public void testInMemoryIndexPersists() throws IOException Review Comment: Here is the failing test as a stand alone test ``` @Test public void sample() throws IOException { File directory = new File(Files.createTempDirectory(null)); directory.deleteOnExit(); test(directory, ImmutableMap.of(TimeUUID.fromString("58ed800a-d357-11b2-0000-000000000000"), new int[]{ 0 }, TimeUUID.fromString("8001c17e-d357-11b2-0000-000000000000"), new int[]{ 0 } )); } ``` ########## test/unit/org/apache/cassandra/journal/IndexTest.java: ########## @@ -0,0 +1,133 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.journal; + +import java.io.IOException; +import java.nio.file.Files; + +import org.junit.Test; + +import org.apache.cassandra.io.util.File; +import org.apache.cassandra.utils.TimeUUID; + +import static org.apache.cassandra.utils.TimeUUID.Generator.nextTimeUUID; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class IndexTest +{ + private static final int[] EMPTY = new int[0]; + + @Test + public void testInMemoryIndexBasics() + { + InMemoryIndex<TimeUUID> index = InMemoryIndex.create(TimeUUIDKeySupport.INSTANCE); + + TimeUUID key0 = nextTimeUUID(); + TimeUUID key1 = nextTimeUUID(); + TimeUUID key2 = nextTimeUUID(); + TimeUUID key3 = nextTimeUUID(); + TimeUUID key4 = nextTimeUUID(); + + assertEquals(0, index.lookUp(key0).length); + assertEquals(0, index.lookUp(key1).length); + assertEquals(0, index.lookUp(key2).length); + assertEquals(0, index.lookUp(key3).length); + assertEquals(0, index.lookUp(key4).length); + + int val11 = 1100; + int val21 = 2100; + int val22 = 2200; + int val31 = 3100; + int val32 = 3200; + int val33 = 3300; + + index.update(key1, val11); + index.update(key2, val21); + index.update(key2, val22); + index.update(key3, val31); + index.update(key3, val32); + index.update(key3, val33); + + assertArrayEquals(EMPTY, index.lookUp(key0)); + assertArrayEquals(new int[] { val11 }, index.lookUp(key1)); + assertArrayEquals(new int[] { val21, val22 }, index.lookUp(key2)); + assertArrayEquals(new int[] { val31, val32, val33 }, index.lookUp(key3)); + assertArrayEquals(EMPTY, index.lookUp(key4)); + + assertEquals(key1, index.firstId()); + assertEquals(key3, index.lastId()); + + assertFalse(index.mayContainId(key0, TimeUUIDKeySupport.INSTANCE)); + assertTrue(index.mayContainId(key1, TimeUUIDKeySupport.INSTANCE)); + assertTrue(index.mayContainId(key2, TimeUUIDKeySupport.INSTANCE)); + assertTrue(index.mayContainId(key3, TimeUUIDKeySupport.INSTANCE)); + assertFalse(index.mayContainId(key4, TimeUUIDKeySupport.INSTANCE)); + } + + @Test + public void testInMemoryIndexPersists() throws IOException Review Comment: this test is better as a fuzz test rather than using fixed values. I took a stab at that, and seems OnDisk lost a value? ``` diff --git a/src/java/org/apache/cassandra/journal/Journal.java b/src/java/org/apache/cassandra/journal/Journal.java index a9a19ed2c7..eab96c66cc 100644 --- a/src/java/org/apache/cassandra/journal/Journal.java +++ b/src/java/org/apache/cassandra/journal/Journal.java @@ -43,7 +43,6 @@ import org.apache.cassandra.io.util.DataOutputBuffer; import org.apache.cassandra.io.util.File; import org.apache.cassandra.io.util.PathUtils; import org.apache.cassandra.journal.Segments.ReferencedSegments; -import org.apache.cassandra.net.MessagingService; import org.apache.cassandra.service.StorageService; import org.apache.cassandra.utils.Crc; import org.apache.cassandra.utils.JVMStabilityInspector; diff --git a/test/unit/org/apache/cassandra/journal/IndexTest.java b/test/unit/org/apache/cassandra/journal/IndexTest.java index 819e1f1c1f..62ea082b59 100644 --- a/test/unit/org/apache/cassandra/journal/IndexTest.java +++ b/test/unit/org/apache/cassandra/journal/IndexTest.java @@ -19,17 +19,29 @@ package org.apache.cassandra.journal; import java.io.IOException; import java.nio.file.Files; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; +import com.google.common.collect.Maps; import org.junit.Test; +import org.agrona.collections.IntHashSet; import org.apache.cassandra.io.util.File; +import org.apache.cassandra.utils.Generators; import org.apache.cassandra.utils.TimeUUID; +import org.quicktheories.core.Gen; +import org.quicktheories.impl.Constraint; import static org.apache.cassandra.utils.TimeUUID.Generator.nextTimeUUID; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.quicktheories.QuickTheory.qt; public class IndexTest { @@ -130,4 +142,107 @@ public class IndexTest assertFalse(onDisk.mayContainId(key4, TimeUUIDKeySupport.INSTANCE)); } } + + @Test + public void prop() throws IOException + { + Constraint sizeConstraint = Constraint.between(1, 10); + Constraint valueSizeConstraint = Constraint.between(0, 10); + Constraint positionConstraint = Constraint.between(0, Integer.MAX_VALUE); + Gen<TimeUUID> keyGen = Generators.timeUUID(); + Gen<int[]> valueGen = rs -> { + int[] array = new int[(int) rs.next(valueSizeConstraint)]; + IntHashSet uniq = new IntHashSet(); + for (int i = 0; i < array.length; i++) + { + int value = (int) rs.next(positionConstraint); + if (!uniq.add(value)) + value = (int) rs.next(positionConstraint); + array[i] = value; + } + return array; + }; + Gen<Map<TimeUUID, int[]>> gen = rs -> { + int size = (int) rs.next(sizeConstraint); + Map<TimeUUID, int[]> map = Maps.newHashMapWithExpectedSize(size); + for (int i = 0; i < size; i++) + { + TimeUUID key = keyGen.generate(rs); + while (map.containsKey(key)) + key = keyGen.generate(rs); + int[] value = valueGen.generate(rs); + map.put(key, value); + } + return map; + }; + gen = gen.describedAs(map -> { + StringBuilder sb = new StringBuilder(); + for (Map.Entry<TimeUUID, int[]> entry : map.entrySet()) + sb.append('\n').append(entry.getKey()).append('\t').append(Arrays.toString(entry.getValue())); + return sb.toString(); + }); + File directory = new File(Files.createTempDirectory(null)); + directory.deleteOnExit(); + qt().withFixedSeed(1735898489413583L).forAll(gen).checkAssert(map -> test(directory, map)); + } + + private static void test(File directory, Map<TimeUUID, int[]> map) + { + InMemoryIndex<TimeUUID> inMemory = InMemoryIndex.create(TimeUUIDKeySupport.INSTANCE); + for (Map.Entry<TimeUUID, int[]> e : map.entrySet()) + { + TimeUUID key = e.getKey(); + assertThat(inMemory.lookUp(key)).isEmpty(); + assertThat(inMemory.mayContainId(key, TimeUUIDKeySupport.INSTANCE)).isFalse(); + + int[] value = e.getValue(); + if (value.length == 0) + continue; + for (int i : value) + inMemory.update(key, i); + Arrays.sort(value); + } + assertIndex(map, inMemory); + + Descriptor descriptor = Descriptor.create(directory, System.nanoTime(), 1); + inMemory.persist(descriptor); + + try (OnDiskIndex<TimeUUID> onDisk = OnDiskIndex.open(descriptor, TimeUUIDKeySupport.INSTANCE)) + { + assertIndex(map, onDisk); + } + } + + private static void assertIndex(Map<TimeUUID, int[]> expected, Index<TimeUUID> actual) + { + List<TimeUUID> keys = expected.entrySet().stream().filter(e -> e.getValue().length != 0).map(Map.Entry::getKey).collect(Collectors.toList()); + Collections.sort(keys); + if (keys.isEmpty()) + { + assertThat(actual.firstId()).describedAs("Index %s had wrong firstId", actual).isNull(); + assertThat(actual.lastId()).describedAs("Index %s had wrong lastId", actual).isNull(); + } + else + { + assertThat(actual.firstId()).describedAs("Index %s had wrong firstId", actual).isEqualTo(keys.get(0)); + assertThat(actual.lastId()).describedAs("Index %s had wrong lastId", actual).isEqualTo(keys.get(keys.size() - 1)); + } + + for (Map.Entry<TimeUUID, int[]> e : expected.entrySet()) + { + TimeUUID key = e.getKey(); + int[] value = e.getValue(); + int[] read = actual.lookUp(key); + if (value.length == 0) + { + assertThat(read).describedAs("Index %s returned wrong values", actual).isEmpty(); + assertThat(actual.mayContainId(key, TimeUUIDKeySupport.INSTANCE)).describedAs("Index %s", actual).isFalse(); + } + else + { + assertThat(read).describedAs("Index %s returned wrong values", actual).isEqualTo(value); + assertThat(actual.mayContainId(key, TimeUUIDKeySupport.INSTANCE)).describedAs("Index %s", actual).isTrue(); + } + } + } } \ No newline at end of file diff --git a/test/unit/org/apache/cassandra/utils/Generators.java b/test/unit/org/apache/cassandra/utils/Generators.java index 3d17828426..77408b2a0e 100644 --- a/test/unit/org/apache/cassandra/utils/Generators.java +++ b/test/unit/org/apache/cassandra/utils/Generators.java @@ -407,6 +407,25 @@ public final class Generators .map(s -> new String(s.getBytes(StandardCharsets.UTF_8), StandardCharsets.UTF_8)); } + public static Gen<TimeUUID> timeUUID() + { + ZonedDateTime now = ZonedDateTime.of(2020, 8, 20, + 0, 0, 0, 0, ZoneOffset.UTC); + ZonedDateTime startOfTime = now.minusYears(50); + ZonedDateTime endOfDays = now.plusYears(50); + Constraint micros = Constraint.between(toMicros(startOfTime), toMicros(endOfDays)); + return rs -> { + long nowMicro = rs.next(micros); + long lsb = rs.next(Constraint.none()); + return new TimeUUID(TimeUUID.unixMicrosToRawTimestamp(nowMicro), lsb); + }; + } + + private static long toMicros(ZonedDateTime zdt) + { + return zdt.toInstant().toEpochMilli() * 1000 + zdt.getNano() / 1000; + } + private static boolean isDash(char c) { switch (c) ``` Failure ``` Smallest found falsifying value(s) :- 58ed800a-d357-11b2-0000-000000000000 [0] 8001c17e-d357-11b2-0000-000000000000 [0] 58ed8000-d357-11b2-0000-000000000000 [] Cause was :- org.opentest4j.AssertionFailedError: [Index org.apache.cassandra.journal.OnDiskIndex@72c8e7b returned wrong values] Expecting: <[]> to be equal to: <[0]> ... Seed was 1735898489413583 ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
