kirklund commented on a change in pull request #6590: URL: https://github.com/apache/geode/pull/6590#discussion_r650181203
########## File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursorTest.java ########## @@ -0,0 +1,349 @@ +/* + * 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.geode.redis.internal.collections; + +import static org.apache.geode.redis.internal.collections.SizeableObject2ObjectOpenCustomHashMapWithCursor.BACKING_ARRAY_LENGTH_COEFFICIENT; +import static org.apache.geode.redis.internal.collections.SizeableObject2ObjectOpenCustomHashMapWithCursor.BACKING_ARRAY_OVERHEAD_CONSTANT; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.HashMap; +import java.util.Map; +import java.util.Random; +import java.util.stream.IntStream; + +import it.unimi.dsi.fastutil.Hash; +import it.unimi.dsi.fastutil.bytes.ByteArrays; +import org.assertj.core.api.SoftAssertions; +import org.junit.Test; + +import org.apache.geode.internal.size.ReflectionObjectSizer; + +public class SizeableObject2ObjectOpenCustomHashMapWithCursorTest { + private final ReflectionObjectSizer sizer = ReflectionObjectSizer.getInstance(); Review comment: Another singleton. Since this is a test, just instantiate it. ########## File path: geode-core/src/main/java/org/apache/geode/internal/size/ReflectionSingleObjectSizer.java ########## @@ -179,5 +181,8 @@ private static int sizeType(Class<?> t) { } } + public static ReflectionSingleObjectSizer getInstance() { + return INSTANCE; + } Review comment: Please don't add any new singletons into the code base. We want to remove all of the one that are already there. It's better to have classes that are dependent on this class create an instance. If you need to share an instance across many dependent instances, then have a factory create an instance of ReflectionSingleObjectSizer to pass into all of the dependents. ########## File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/collections/SizeableObjectOpenCustomHashSet.java ########## @@ -14,23 +14,23 @@ */ package org.apache.geode.redis.internal.collections; -import static org.apache.geode.internal.size.ReflectionSingleObjectSizer.roundUpSize; - import java.util.Collection; import java.util.Iterator; import it.unimi.dsi.fastutil.objects.ObjectCollection; import it.unimi.dsi.fastutil.objects.ObjectOpenCustomHashSet; import org.apache.geode.annotations.VisibleForTesting; +import org.apache.geode.internal.size.ReflectionSingleObjectSizer; import org.apache.geode.internal.size.Sizeable; public class SizeableObjectOpenCustomHashSet<K> extends ObjectOpenCustomHashSet<K> implements Sizeable { private static final long serialVersionUID = 9174920505089089517L; - public static final int MEMBER_OVERHEAD_CONSTANT = 16; public static final int BACKING_ARRAY_OVERHEAD_CONSTANT = 92; public static final int BACKING_ARRAY_LENGTH_COEFFICIENT = 4; + private static final ReflectionSingleObjectSizer elementSizer = + ReflectionSingleObjectSizer.getInstance(); Review comment: Unless you're creating large numbers of instances of SizeableObjectOpenCustomHashSet, I recommend avoiding singletons even here. Unless there's some reason that ReflectionSingleObjectSizer should NEVER have multiple instances, then just create an instance here. Also, can you type it as SingleObjectSizer? It would be so much better to depend only on interfaces instead of concrete implementation. ########## File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursorTest.java ########## @@ -0,0 +1,349 @@ +/* + * 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.geode.redis.internal.collections; + +import static org.apache.geode.redis.internal.collections.SizeableObject2ObjectOpenCustomHashMapWithCursor.BACKING_ARRAY_LENGTH_COEFFICIENT; +import static org.apache.geode.redis.internal.collections.SizeableObject2ObjectOpenCustomHashMapWithCursor.BACKING_ARRAY_OVERHEAD_CONSTANT; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.HashMap; +import java.util.Map; +import java.util.Random; +import java.util.stream.IntStream; + +import it.unimi.dsi.fastutil.Hash; +import it.unimi.dsi.fastutil.bytes.ByteArrays; +import org.assertj.core.api.SoftAssertions; +import org.junit.Test; + +import org.apache.geode.internal.size.ReflectionObjectSizer; + +public class SizeableObject2ObjectOpenCustomHashMapWithCursorTest { + private final ReflectionObjectSizer sizer = ReflectionObjectSizer.getInstance(); + + private static final Hash.Strategy<Integer> NATURAL_HASH = new Hash.Strategy<Integer>() { + @Override + public int hashCode(Integer o) { + return o.hashCode(); + } + + @Override + public boolean equals(Integer a, Integer b) { + return a.equals(b); + } + }; + + private static Hash.Strategy<Integer> COLLIDING_HASH = new Hash.Strategy<Integer>() { + @Override + public int hashCode(Integer o) { + return o % 5; + } + + @Override + public boolean equals(Integer a, Integer b) { + return a.equals(b); + } + }; + + @Test + public void scanEntireMap_ReturnsExpectedElements() { + SizeableObject2ObjectOpenCustomHashMapWithCursor<Integer, String> map = + new SizeableObject2ObjectOpenCustomHashMapWithCursor<>(NATURAL_HASH); + IntStream.range(0, 10).forEach(i -> map.put(i, "value-" + i)); + + HashMap<Integer, String> scanned = new HashMap<>(); + int result = map.scan(0, 10000, HashMap::put, scanned); + assertThat(result).isEqualTo(0); + assertThat(scanned).isEqualTo(map); + } + + @Test + public void twoScansWithNoModifications_ReturnsExpectedElements() { + SizeableObject2ObjectOpenCustomHashMapWithCursor<Integer, String> map = + new SizeableObject2ObjectOpenCustomHashMapWithCursor<>(NATURAL_HASH); + IntStream.range(0, 10).forEach(i -> map.put(i, "value-" + i)); + + HashMap<Integer, String> scanned = new HashMap<>(); + int cursor = map.scan(0, 3, HashMap::put, scanned); + assertThat(scanned).hasSize(3); + cursor = map.scan(cursor, 3, HashMap::put, scanned); + assertThat(scanned).hasSize(6); + cursor = map.scan(cursor, 4, HashMap::put, scanned); + assertThat(scanned).hasSize(10); + cursor = map.scan(cursor, 4, HashMap::put, scanned); + assertThat(scanned).hasSize(10); + assertThat(cursor).isEqualTo(0); + + assertThat(scanned).isEqualTo(map); + } Review comment: Since this is a unit test that runs very fast, it's much better to copy the test for each assertion and then just assert one thing in each test method. ########## File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/collections/SizeableObjectOpenCustomHashSetTest.java ########## @@ -99,13 +50,15 @@ public void backingArrayOverheadCalculationTest() { // Calculate the overhead associated only with the backing array backingArrayOverhead = sizer.sizeof(set) - set.getMemberOverhead(); + int expected = BACKING_ARRAY_OVERHEAD_CONSTANT + + BACKING_ARRAY_LENGTH_COEFFICIENT * set.getBackingArrayLength(); softly.assertThat(backingArrayOverhead) Review comment: Not a request for changes... Please steer developers away from using SoftAssertions. This is not a good practice in unit tests. ########## File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursorTest.java ########## @@ -0,0 +1,349 @@ +/* + * 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.geode.redis.internal.collections; + +import static org.apache.geode.redis.internal.collections.SizeableObject2ObjectOpenCustomHashMapWithCursor.BACKING_ARRAY_LENGTH_COEFFICIENT; +import static org.apache.geode.redis.internal.collections.SizeableObject2ObjectOpenCustomHashMapWithCursor.BACKING_ARRAY_OVERHEAD_CONSTANT; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.HashMap; +import java.util.Map; +import java.util.Random; +import java.util.stream.IntStream; + +import it.unimi.dsi.fastutil.Hash; +import it.unimi.dsi.fastutil.bytes.ByteArrays; +import org.assertj.core.api.SoftAssertions; +import org.junit.Test; + +import org.apache.geode.internal.size.ReflectionObjectSizer; + +public class SizeableObject2ObjectOpenCustomHashMapWithCursorTest { + private final ReflectionObjectSizer sizer = ReflectionObjectSizer.getInstance(); + + private static final Hash.Strategy<Integer> NATURAL_HASH = new Hash.Strategy<Integer>() { + @Override + public int hashCode(Integer o) { + return o.hashCode(); + } + + @Override + public boolean equals(Integer a, Integer b) { + return a.equals(b); + } + }; + + private static Hash.Strategy<Integer> COLLIDING_HASH = new Hash.Strategy<Integer>() { + @Override + public int hashCode(Integer o) { + return o % 5; + } + + @Override + public boolean equals(Integer a, Integer b) { + return a.equals(b); + } + }; + + @Test + public void scanEntireMap_ReturnsExpectedElements() { + SizeableObject2ObjectOpenCustomHashMapWithCursor<Integer, String> map = + new SizeableObject2ObjectOpenCustomHashMapWithCursor<>(NATURAL_HASH); + IntStream.range(0, 10).forEach(i -> map.put(i, "value-" + i)); + + HashMap<Integer, String> scanned = new HashMap<>(); Review comment: Let's stop using concrete implementations for type declarations. This should be: ``` Map<Integer, String> scanned = new HashMap<>(); ``` Same for any other type declarations. -- 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. For queries about this service, please contact Infrastructure at: [email protected]
