sabbey37 commented on a change in pull request #6296:
URL: https://github.com/apache/geode/pull/6296#discussion_r625806486
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -59,6 +59,11 @@
private ConcurrentHashMap<UUID, Long> hScanSnapShotCreationTimes;
private ScheduledExecutorService HSCANSnapshotExpirationExecutor = null;
+ private int myCalculatedSize;
Review comment:
Maybe we could call this `sizeInBytes`?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -59,6 +59,11 @@
private ConcurrentHashMap<UUID, Long> hScanSnapShotCreationTimes;
private ScheduledExecutorService HSCANSnapshotExpirationExecutor = null;
+ private int myCalculatedSize;
+ private static final int BASE_REDIS_HASH_OVERHEAD = 232;
+ protected static final int HASH_MAP_VALUE_PAIR_OVERHEAD = 96;
+ protected static final int SIZE_OF_OVERHEAD_OF_FIRST_PAIR = 96;
Review comment:
I think adding a comment here about how these were calculated and why
they are constants is useful.
##########
File path:
geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/internal/data/PartitionedRegionStatsUpdateTest.java
##########
@@ -0,0 +1,396 @@
+/*
+ * 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.data;
+
+import static
org.apache.geode.distributed.ConfigurationProperties.MAX_WAIT_TIME_RECONNECT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Properties;
+
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Ignore;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+
+public class PartitionedRegionStatsUpdateTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule clusterStartUpRule = new
RedisClusterStartupRule(3);
+
+ private static MemberVM server1;
+ private static MemberVM server2;
+
+ private static Jedis jedis1;
+ private static Jedis jedis2;
+
+ private static final int JEDIS_TIMEOUT =
Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static final String LOCAL_HOST = "127.0.0.1";
+ public static final String STRING_KEY = "string key";
+ public static final String SET_KEY = "set key";
+ public static final String HASH_KEY = "hash key";
+ public static final String LONG_APPEND_VALUE =
String.valueOf(Integer.MAX_VALUE);
+ public static final String FIELD = "field";
+
+ @BeforeClass
+ public static void classSetup() {
+ Properties locatorProperties = new Properties();
+ locatorProperties.setProperty(MAX_WAIT_TIME_RECONNECT, "15000");
+
+ MemberVM locator = clusterStartUpRule.startLocatorVM(0, locatorProperties);
+ int locatorPort = locator.getPort();
+
+ server1 = clusterStartUpRule.startRedisVM(1, locatorPort);
+ int redisServerPort1 = clusterStartUpRule.getRedisPort(1);
+ jedis1 = new Jedis(LOCAL_HOST, redisServerPort1, JEDIS_TIMEOUT);
+
+ server2 = clusterStartUpRule.startRedisVM(2, locatorPort);
+ int redisServerPort2 = clusterStartUpRule.getRedisPort(1);
+ jedis2 = new Jedis(LOCAL_HOST, redisServerPort2, JEDIS_TIMEOUT);
+ }
+
+ @Before
+ public void setup() {
+ jedis1.flushAll();
+ }
+
+ @Test
+ public void
should_showIncreaseInDatastoreBytesInUse_givenStringValueSizeIncreases() {
+ String LONG_APPEND_VALUE = String.valueOf(Integer.MAX_VALUE);
+ jedis1.set(STRING_KEY, "value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.append(STRING_KEY, LONG_APPEND_VALUE);
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+
assertThat(finalDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showDecreaseInDatastoreBytesInUse_givenStringValueDeleted() {
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ jedis1.set(STRING_KEY, "value");
+
+ long intermediateDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
assertThat(intermediateDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+
+ jedis1.del(STRING_KEY);
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showDecreaseInDatastoreBytesInUse_givenStringValueShortened() {
+ jedis1.set(STRING_KEY, "longer value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ jedis1.set(STRING_KEY, "value");
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+
assertThat(finalDataStoreBytesInUse).isLessThan(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void should_resetMemoryUsage_givenFlushAllCommand() {
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(initialDataStoreBytesInUse).isEqualTo(0L);
+
+ jedis1.set(STRING_KEY, "value");
+
+ jedis1.flushAll();
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showNoIncreaseInDatastoreBytesInUse_givenStringValueSizeDoesNotIncrease()
{
+ jedis1.set(STRING_KEY, "value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.set(STRING_KEY, "value");
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showIncreaseInDatastoreBytesInUse_givenSetValueSizeIncreases() {
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.sadd(SET_KEY, "value" + i);
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+
assertThat(finalDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showNoIncreaseInDatastoreBytesInUse_givenSetValueSizeDoesNotIncrease() {
+ jedis1.sadd(SET_KEY, "value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.sadd(SET_KEY, "value");
+ }
+
+ assertThat(jedis1.scard(SET_KEY)).isEqualTo(1);
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void should_showDecreaseInDatastoreBytesInUse_givenSetValueDeleted() {
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ jedis1.sadd(SET_KEY, "value");
+
+ long intermediateDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
assertThat(intermediateDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+
+ jedis1.del(SET_KEY);
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showDecreaseInDatastoreBytesInUse_givenSetValueSizeDecreases() {
+ for (int i = 0; i < 10; i++) {
+ jedis1.sadd(SET_KEY, "value" + i);
+ }
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 10; i++) {
+ jedis1.srem(SET_KEY, "value" + i);
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+
assertThat(finalDataStoreBytesInUse).isLessThan(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showIncreaseInDatastoreBytesInUse_givenHashValueSizeIncreases() {
+ jedis1.hset(HASH_KEY, FIELD, "value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.hset(HASH_KEY, FIELD + i, LONG_APPEND_VALUE);
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+
assertThat(finalDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void should_showDecreaseInDatastoreBytesInUse_givenHashValueDeleted()
{
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ jedis1.hset(HASH_KEY, FIELD, "value");
+
+ long intermediateDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
assertThat(intermediateDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+
+ jedis1.del(HASH_KEY);
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showNoIncreaseInDatastoreBytesInUse_givenHSetDoesNotIncreaseHashSize() {
+ jedis2.hset(HASH_KEY, FIELD, "initialvalue"); // two hsets are required to
force
+ jedis2.hset(HASH_KEY, FIELD, "value"); // deserialization on both servers
+ // otherwise primary/secondary can disagree on size, and which server is
primary varies
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server2);
+
+ for (int i = 0; i < 10; i++) {
+ jedis2.hset(HASH_KEY, FIELD, "value");
+ }
+
+ assertThat(jedis2.hgetAll(HASH_KEY).size()).isEqualTo(1);
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server2);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showIncreaseInDatastoreBytesInUse_givenHSetNXIncreasesHashSize() {
+ jedis1.hset(HASH_KEY, FIELD, "value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.hsetnx(HASH_KEY, FIELD + i, "value");
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+
assertThat(finalDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showNoIncreaseInDatastoreBytesInUse_givenHSetNXDoesNotIncreaseHashSize()
{
+ jedis1.hset(HASH_KEY, FIELD, "value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.hsetnx(HASH_KEY, FIELD, "value");
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ /******* confirm that the other member agrees upon size *******/
+
+ @Test
+ public void
should_showMembersAgreeUponUsedHashMemory_afterDeltaPropagation() {
+ jedis1.hset(HASH_KEY, FIELD, "initialvalue"); // two hsets are required to
force
+ jedis1.hset(HASH_KEY, FIELD, "finalvalue"); // deserialization on both
servers
+ // otherwise primary/secondary can disagree on size, and which server is
primary varies
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server2);
+
+ for (int i = 0; i < 10; i++) {
+ jedis1.hset(HASH_KEY, FIELD, "finalvalue");
+ }
+
+ assertThat(jedis1.hgetAll(HASH_KEY).size()).isEqualTo(1);
+
+ long server2FinalDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server2);
+ long server1FinalDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(server1FinalDataStoreBytesInUse)
+ .isEqualTo(server2FinalDataStoreBytesInUse)
+ .isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ @Ignore("confirm that bucket size stats are being calculated correctly
before enabling")
+ public void should_showMembersAgreeUponUsedSetMemory_afterDeltaPropagation()
{
+ jedis1.sadd(SET_KEY, "other"); // two sadds are required to force
+ jedis1.sadd(SET_KEY, "value"); // deserialization on both servers
+ // otherwise primary/secondary can disagree on size, and which server is
primary varies
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server2);
+
+ for (int i = 0; i < 10; i++) {
+ jedis1.sadd(SET_KEY, "value");
+ }
+
+ assertThat(jedis1.scard(SET_KEY)).isEqualTo(2);
+
+ long server1FinalDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+ long server2FinalDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server2);
+
+ assertThat(server1FinalDataStoreBytesInUse)
+ .isEqualTo(server2FinalDataStoreBytesInUse)
+ .isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ @Ignore("confirm that bucket size stats are being calculated correctly
before enabling")
+ public void
should_showMembersAgreeUponUsedStringMemory_afterDeltaPropagation() {
Review comment:
Are we able to remove this `@Ignore` now?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -35,27 +35,61 @@
import org.apache.commons.lang3.tuple.ImmutablePair;
import org.apache.commons.lang3.tuple.Pair;
+import org.apache.geode.DataSerializer;
import org.apache.geode.annotations.VisibleForTesting;
import org.apache.geode.cache.Region;
-import org.apache.geode.internal.InternalDataSerializer;
import org.apache.geode.internal.serialization.DeserializationContext;
import org.apache.geode.internal.serialization.KnownVersion;
import org.apache.geode.internal.serialization.SerializationContext;
+import org.apache.geode.internal.size.ReflectionObjectSizer;
import org.apache.geode.redis.internal.delta.AddsDeltaInfo;
import org.apache.geode.redis.internal.delta.DeltaInfo;
import org.apache.geode.redis.internal.delta.RemsDeltaInfo;
public class RedisSet extends AbstractRedisData {
-
private HashSet<ByteArrayWrapper> members;
+ private static int baseRedissetOverhead;
Review comment:
If we end up keeping this variable, set should be capitalized.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -35,27 +35,61 @@
import org.apache.commons.lang3.tuple.ImmutablePair;
import org.apache.commons.lang3.tuple.Pair;
+import org.apache.geode.DataSerializer;
import org.apache.geode.annotations.VisibleForTesting;
import org.apache.geode.cache.Region;
-import org.apache.geode.internal.InternalDataSerializer;
import org.apache.geode.internal.serialization.DeserializationContext;
import org.apache.geode.internal.serialization.KnownVersion;
import org.apache.geode.internal.serialization.SerializationContext;
+import org.apache.geode.internal.size.ReflectionObjectSizer;
import org.apache.geode.redis.internal.delta.AddsDeltaInfo;
import org.apache.geode.redis.internal.delta.DeltaInfo;
import org.apache.geode.redis.internal.delta.RemsDeltaInfo;
public class RedisSet extends AbstractRedisData {
-
private HashSet<ByteArrayWrapper> members;
+ private static int baseRedissetOverhead;
+ private static int perMemberOverhead;
+ private static int internalHashsetStorageOverhead;
+
+ private int myCalculatedSize;
Review comment:
Maybe we could call the variable `sizeInBytes`?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -35,11 +36,15 @@
import org.apache.geode.redis.internal.netty.Coder;
public class RedisString extends AbstractRedisData {
-
private int appendSequence;
private ByteArrayWrapper value;
+ // this value is empirically derived using ReflectionObjectSizer, which
provides an exact size
+ // of the object. It can't be used directly because of its performance
impact. This value causes
+ // the size we keep track of to converge to the actual size as it increases.
+ public static final int PER_STRING_OVERHEAD = 74;
Review comment:
For consistency, could we give this constant a similar name to the one
in `RedisHash`, maybe something like: `BASE_REDIS_STRING_OVERHEAD`?
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisSetTest.java
##########
@@ -165,4 +169,65 @@ public void
setExpirationTimestamp_stores_delta_that_is_stable() throws IOExcept
o2.fromDelta(in);
assertThat(o2).isEqualTo(o1);
}
+
+ @SuppressWarnings("unchecked")
+ @Test
+ public void should_calculateSize_closeToROS_withVaryingMemberCounts() {
+ for (int i = 0; i < 1024; i += 16) {
+ RedisSet set = createRedisSetOfSpecifiedSize(i);
+
+ int expected = reflectionObjectSizer.sizeof(set);
+ Long actual = Long.valueOf(set.getSizeInBytes());
+ Offset<Long> offset = Offset.offset(Math.round(expected * 0.06));
+
+ assertThat(actual).isCloseTo(expected, offset);
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ @Test
+ public void should_calculateSize_closeToROS_withVaryingMemberSize() {
+ for (int i = 0; i < 16; i++) {
+ RedisSet set = createRedisSetWithMemberOfSpecifiedSize(i * 64);
+ int expected = reflectionObjectSizer.sizeof(set);
+ Long actual = Long.valueOf(set.getSizeInBytes());
+ Offset<Long> offset = Offset.offset(Math.round(expected * 0.06));
+
+ assertThat(actual).isCloseTo(expected, offset);
+ }
+ }
+
+ private RedisSet createRedisSetOfSpecifiedSize(int setSize) {
+ ArrayList<ByteArrayWrapper> arrayList = new ArrayList<>();
+ for (int i = 0; i < setSize; i++) {
+ arrayList.add(new ByteArrayWrapper(("abcdefgh" + i).getBytes()));
+ }
+ return new RedisSet(arrayList);
+ }
+
+ private RedisSet createRedisSetWithMemberOfSpecifiedSize(int memberSize) {
+ ArrayList<ByteArrayWrapper> arrayList = new ArrayList<>();
+ ByteArrayWrapper member =
+ new ByteArrayWrapper(createMemberOfSpecifiedSize("a",
memberSize).getBytes());
+ if (member.length() > 0) {
+ arrayList.add(member);
+ }
+ return new RedisSet(arrayList);
+ }
+
+ private String createMemberOfSpecifiedSize(final String base, final int
stringSize) {
+ Random random = new Random();
+ if (base.length() > stringSize) {
+ return "";
+ }
+ StringBuffer sb = new StringBuffer(stringSize);
+ sb.append(base);
+ for (int i = base.length(); i < stringSize; i++) {
+ int randy = random.nextInt(10);
+ sb.append(randy);
+ }
+ String javaString = sb.toString();
+ return javaString;
+ }
+
Review comment:
Do we need to add tests like the ones that are in `RedisHash` (e.g.
sizeShouldDecreaseWhenValueIsRemoved,
shouldReturnToSetOverhead_whenAllMembersAreRemoved, etc.)?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -35,27 +35,61 @@
import org.apache.commons.lang3.tuple.ImmutablePair;
import org.apache.commons.lang3.tuple.Pair;
+import org.apache.geode.DataSerializer;
import org.apache.geode.annotations.VisibleForTesting;
import org.apache.geode.cache.Region;
-import org.apache.geode.internal.InternalDataSerializer;
import org.apache.geode.internal.serialization.DeserializationContext;
import org.apache.geode.internal.serialization.KnownVersion;
import org.apache.geode.internal.serialization.SerializationContext;
+import org.apache.geode.internal.size.ReflectionObjectSizer;
import org.apache.geode.redis.internal.delta.AddsDeltaInfo;
import org.apache.geode.redis.internal.delta.DeltaInfo;
import org.apache.geode.redis.internal.delta.RemsDeltaInfo;
public class RedisSet extends AbstractRedisData {
-
private HashSet<ByteArrayWrapper> members;
+ private static int baseRedissetOverhead;
+ private static int perMemberOverhead;
+ private static int internalHashsetStorageOverhead;
+
+ private int myCalculatedSize;
+
@SuppressWarnings("unchecked")
RedisSet(Collection<ByteArrayWrapper> members) {
+ calibrate_memory_values();
+
if (members instanceof HashSet) {
this.members = (HashSet<ByteArrayWrapper>) members;
} else {
this.members = new HashSet<>(members);
}
+
+ for (ByteArrayWrapper value : this.members) {
+ myCalculatedSize += perMemberOverhead + value.length();
+ }
+ }
+
+ private void calibrate_memory_values() {
+ ReflectionObjectSizer reflectionObjectSizer =
ReflectionObjectSizer.getInstance();
+ baseRedissetOverhead = reflectionObjectSizer.sizeof(this) + 18;
+
+ HashSet<ByteArrayWrapper> temp_hashset = new HashSet<>();
+ int base_hashset_size = reflectionObjectSizer.sizeof(temp_hashset);
+ baseRedissetOverhead += base_hashset_size;
+
+ ByteArrayWrapper baw1 = new ByteArrayWrapper("a".getBytes());
+ ByteArrayWrapper baw2 = new ByteArrayWrapper("b".getBytes());
+ temp_hashset.add(baw1);
+ int one_entry_hashset_size = reflectionObjectSizer.sizeof(temp_hashset);
+ temp_hashset.add(baw2);
+ int two_entries_hashset_size = reflectionObjectSizer.sizeof(temp_hashset);
+
+ perMemberOverhead = two_entries_hashset_size - one_entry_hashset_size + 5;
+ internalHashsetStorageOverhead =
+ two_entries_hashset_size - (2 * perMemberOverhead) - base_hashset_size;
+
+ myCalculatedSize = baseRedissetOverhead;
Review comment:
Since we decided to use constants for the overhead values in
`RedisHash`, should we do that here as well? If not, we could move this to a
static initialization block or make `sizeInBytes` static and initialize it with
this static method.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -161,16 +168,46 @@ public int getDSFID() {
private synchronized ByteArrayWrapper hashPut(ByteArrayWrapper field,
ByteArrayWrapper value) {
- return hash.put(field, value);
+ if (this.hash.isEmpty()) {
+ this.myCalculatedSize += SIZE_OF_OVERHEAD_OF_FIRST_PAIR;
+ }
+
+ ByteArrayWrapper oldvalue = hash.put(field, value);
+
+ if (oldvalue == null) {
+ calculateSizeOfNewFieldValuePair(field, value);
+ } else {
+ this.myCalculatedSize += value.length() - oldvalue.length();
+ }
+
+ return oldvalue;
}
private synchronized ByteArrayWrapper hashPutIfAbsent(ByteArrayWrapper field,
ByteArrayWrapper value) {
- return hash.putIfAbsent(field, value);
+ ByteArrayWrapper oldvalue = hash.putIfAbsent(field, value);
+
Review comment:
I think we need that same clause from `hashPut` here:
```
if (hash.isEmpty()) {
this.myCalculatedSize += SIZE_OF_OVERHEAD_OF_FIRST_PAIR;
}
```
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -214,19 +250,31 @@ public int getDSFID() {
}
private synchronized boolean membersAdd(ByteArrayWrapper memberToAdd) {
- return members.add(memberToAdd);
+ boolean actuallyAdded = members.add(memberToAdd);
Review comment:
Not sure if we have a convention for boolean variable names in Geode,
but the general Java convention seems to be prefixing boolean variables with
`is`, `has`, `can`, or `should`. With that in mind, could this variable be
called `isAdded` or `isMemberAdded`?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -214,19 +250,31 @@ public int getDSFID() {
}
private synchronized boolean membersAdd(ByteArrayWrapper memberToAdd) {
- return members.add(memberToAdd);
+ boolean actuallyAdded = members.add(memberToAdd);
+ if (actuallyAdded) {
+ myCalculatedSize += perMemberOverhead + memberToAdd.length();
+ }
+ return actuallyAdded;
}
private boolean membersRemove(ByteArrayWrapper memberToRemove) {
- return members.remove(memberToRemove);
+ boolean actuallyRemoved = members.remove(memberToRemove);
Review comment:
Same comment as `actuallyAdded` above.
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisHashTest.java
##########
@@ -258,11 +260,373 @@ public void
hscanSnaphots_shouldExpireAfterExpiryPeriod() {
});
}
+ /************* Hash Size *************/
+ /******* constants *******/
+ @Test
+ public void constantValuePairOverhead_shouldEqualCalculatedOverhead() {
+ RedisHash redisHash = new RedisHash();
+ int sizeOfDataForOneFieldValuePair = 16; // initial byte[]s are 8 bytes
each
+
+ HashMap<ByteArrayWrapper, ByteArrayWrapper> tempHashmap = new HashMap<>();
+
+ ByteArrayWrapper field1 = new ByteArrayWrapper("a".getBytes());
+ ByteArrayWrapper value1 = new ByteArrayWrapper("b".getBytes());
+ ByteArrayWrapper field2 = new ByteArrayWrapper("c".getBytes());
+ ByteArrayWrapper value2 = new ByteArrayWrapper("d".getBytes());
+
+ tempHashmap.put(field1, value1);
+ int oneEntryHashMapSize = reflectionObjectSizer.sizeof(tempHashmap);
+
+ tempHashmap.put(field2, value2);
+ int twoEntriesHashMapSize = reflectionObjectSizer.sizeof(tempHashmap);
+
+ int expectedValuePairOverhead = twoEntriesHashMapSize - oneEntryHashMapSize
+ - sizeOfDataForOneFieldValuePair;
+
+
assertThat(RedisHash.HASH_MAP_VALUE_PAIR_OVERHEAD).isEqualTo(expectedValuePairOverhead);
+ }
+
+ @Test
+ public void constantFirstPairOverhead_shouldEqual_calculatedOverhead() {
+ HashMap<ByteArrayWrapper, ByteArrayWrapper> tempHashmap = new HashMap<>();
+ int emptyHashMapSize = reflectionObjectSizer.sizeof(tempHashmap);
+
+ ByteArrayWrapper field = new ByteArrayWrapper("a".getBytes());
+ ByteArrayWrapper value = new ByteArrayWrapper("b".getBytes());
+
+ tempHashmap.put(field, value);
+ int oneEntryHashMapSize = reflectionObjectSizer.sizeof(tempHashmap);
+
+ int expectedFirstPairOverhead = oneEntryHashMapSize - emptyHashMapSize
+ - RedisHash.HASH_MAP_VALUE_PAIR_OVERHEAD;
+
+
assertThat(RedisHash.SIZE_OF_OVERHEAD_OF_FIRST_PAIR).isEqualTo(expectedFirstPairOverhead);
+ }
+
+ /******* constructor *******/
+ @Test
+ public void should_calculateSize_equalToROSSize_ofEmptyIndividualInstance() {
+ RedisHash redisHash = new RedisHash();
+
+ int expected = reflectionObjectSizer.sizeof(redisHash);
+ int actual = redisHash.getSizeInBytes();
+
+ assertThat(actual).isEqualTo(expected);
+ }
+
+ @SuppressWarnings("unchecked")
+ @Test
+ public void
should_calculateSize_equalToROSSize_ofIndividualInstanceWithSingleValue() {
Review comment:
Could `equalToROSSize` be changed to `closeToROSSize` like the test
below it since we aren't expecting exact equality?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -161,16 +168,46 @@ public int getDSFID() {
private synchronized ByteArrayWrapper hashPut(ByteArrayWrapper field,
ByteArrayWrapper value) {
- return hash.put(field, value);
+ if (this.hash.isEmpty()) {
+ this.myCalculatedSize += SIZE_OF_OVERHEAD_OF_FIRST_PAIR;
+ }
+
+ ByteArrayWrapper oldvalue = hash.put(field, value);
+
+ if (oldvalue == null) {
+ calculateSizeOfNewFieldValuePair(field, value);
+ } else {
+ this.myCalculatedSize += value.length() - oldvalue.length();
+ }
+
+ return oldvalue;
}
private synchronized ByteArrayWrapper hashPutIfAbsent(ByteArrayWrapper field,
ByteArrayWrapper value) {
- return hash.putIfAbsent(field, value);
+ ByteArrayWrapper oldvalue = hash.putIfAbsent(field, value);
+
+ if (oldvalue == null) {
+ calculateSizeOfNewFieldValuePair(field, value);
+ }
+ return oldvalue;
+ }
+
+ private void calculateSizeOfNewFieldValuePair(ByteArrayWrapper field,
ByteArrayWrapper value) {
+ this.myCalculatedSize += HASH_MAP_VALUE_PAIR_OVERHEAD + field.length() +
value.length();
}
private synchronized ByteArrayWrapper hashRemove(ByteArrayWrapper field) {
- return hash.remove(field);
+ ByteArrayWrapper oldValue = hash.remove(field);
+ if (oldValue != null) {
+ myCalculatedSize -= HASH_MAP_VALUE_PAIR_OVERHEAD + field.length() +
oldValue.length();
+ }
+
+ if (hash.size() == 0) {
Review comment:
We used `hash.isEmpty()` above. I realize it's basically the same, but
thought we could use one or the other for consistency.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -161,16 +168,46 @@ public int getDSFID() {
private synchronized ByteArrayWrapper hashPut(ByteArrayWrapper field,
ByteArrayWrapper value) {
- return hash.put(field, value);
+ if (this.hash.isEmpty()) {
+ this.myCalculatedSize += SIZE_OF_OVERHEAD_OF_FIRST_PAIR;
+ }
+
+ ByteArrayWrapper oldvalue = hash.put(field, value);
+
+ if (oldvalue == null) {
+ calculateSizeOfNewFieldValuePair(field, value);
+ } else {
+ this.myCalculatedSize += value.length() - oldvalue.length();
+ }
+
+ return oldvalue;
}
private synchronized ByteArrayWrapper hashPutIfAbsent(ByteArrayWrapper field,
ByteArrayWrapper value) {
- return hash.putIfAbsent(field, value);
+ ByteArrayWrapper oldvalue = hash.putIfAbsent(field, value);
+
+ if (oldvalue == null) {
+ calculateSizeOfNewFieldValuePair(field, value);
+ }
+ return oldvalue;
+ }
+
+ private void calculateSizeOfNewFieldValuePair(ByteArrayWrapper field,
ByteArrayWrapper value) {
+ this.myCalculatedSize += HASH_MAP_VALUE_PAIR_OVERHEAD + field.length() +
value.length();
Review comment:
Based on just the method name, I expected
`calculateSizeOfNewFieldValuePair` to return the size of the new field/value
pair. I was surprised to find it added the size directly. Maybe we could call
it something like `addSizeOfNewFieldValuePair` or something to indicate it
directly adds it?
Alternatively, we could just return the size of the new field value pair
from this method. Then it could also be used in `hashRemove`.
##########
File path:
geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/internal/data/PartitionedRegionStatsUpdateTest.java
##########
@@ -0,0 +1,396 @@
+/*
+ * 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.data;
+
+import static
org.apache.geode.distributed.ConfigurationProperties.MAX_WAIT_TIME_RECONNECT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Properties;
+
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Ignore;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+
+public class PartitionedRegionStatsUpdateTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule clusterStartUpRule = new
RedisClusterStartupRule(3);
+
+ private static MemberVM server1;
+ private static MemberVM server2;
+
+ private static Jedis jedis1;
+ private static Jedis jedis2;
+
+ private static final int JEDIS_TIMEOUT =
Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static final String LOCAL_HOST = "127.0.0.1";
+ public static final String STRING_KEY = "string key";
+ public static final String SET_KEY = "set key";
+ public static final String HASH_KEY = "hash key";
+ public static final String LONG_APPEND_VALUE =
String.valueOf(Integer.MAX_VALUE);
+ public static final String FIELD = "field";
+
+ @BeforeClass
+ public static void classSetup() {
+ Properties locatorProperties = new Properties();
+ locatorProperties.setProperty(MAX_WAIT_TIME_RECONNECT, "15000");
+
+ MemberVM locator = clusterStartUpRule.startLocatorVM(0, locatorProperties);
+ int locatorPort = locator.getPort();
+
+ server1 = clusterStartUpRule.startRedisVM(1, locatorPort);
+ int redisServerPort1 = clusterStartUpRule.getRedisPort(1);
+ jedis1 = new Jedis(LOCAL_HOST, redisServerPort1, JEDIS_TIMEOUT);
+
+ server2 = clusterStartUpRule.startRedisVM(2, locatorPort);
+ int redisServerPort2 = clusterStartUpRule.getRedisPort(1);
+ jedis2 = new Jedis(LOCAL_HOST, redisServerPort2, JEDIS_TIMEOUT);
+ }
+
+ @Before
+ public void setup() {
+ jedis1.flushAll();
+ }
+
+ @Test
+ public void
should_showIncreaseInDatastoreBytesInUse_givenStringValueSizeIncreases() {
+ String LONG_APPEND_VALUE = String.valueOf(Integer.MAX_VALUE);
+ jedis1.set(STRING_KEY, "value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.append(STRING_KEY, LONG_APPEND_VALUE);
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+
assertThat(finalDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showDecreaseInDatastoreBytesInUse_givenStringValueDeleted() {
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ jedis1.set(STRING_KEY, "value");
+
+ long intermediateDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
assertThat(intermediateDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+
+ jedis1.del(STRING_KEY);
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showDecreaseInDatastoreBytesInUse_givenStringValueShortened() {
+ jedis1.set(STRING_KEY, "longer value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ jedis1.set(STRING_KEY, "value");
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+
assertThat(finalDataStoreBytesInUse).isLessThan(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void should_resetMemoryUsage_givenFlushAllCommand() {
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(initialDataStoreBytesInUse).isEqualTo(0L);
+
+ jedis1.set(STRING_KEY, "value");
+
+ jedis1.flushAll();
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showNoIncreaseInDatastoreBytesInUse_givenStringValueSizeDoesNotIncrease()
{
+ jedis1.set(STRING_KEY, "value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.set(STRING_KEY, "value");
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showIncreaseInDatastoreBytesInUse_givenSetValueSizeIncreases() {
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.sadd(SET_KEY, "value" + i);
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+
assertThat(finalDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showNoIncreaseInDatastoreBytesInUse_givenSetValueSizeDoesNotIncrease() {
+ jedis1.sadd(SET_KEY, "value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.sadd(SET_KEY, "value");
+ }
+
+ assertThat(jedis1.scard(SET_KEY)).isEqualTo(1);
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void should_showDecreaseInDatastoreBytesInUse_givenSetValueDeleted() {
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ jedis1.sadd(SET_KEY, "value");
+
+ long intermediateDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
assertThat(intermediateDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+
+ jedis1.del(SET_KEY);
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showDecreaseInDatastoreBytesInUse_givenSetValueSizeDecreases() {
+ for (int i = 0; i < 10; i++) {
+ jedis1.sadd(SET_KEY, "value" + i);
+ }
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 10; i++) {
+ jedis1.srem(SET_KEY, "value" + i);
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+
assertThat(finalDataStoreBytesInUse).isLessThan(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showIncreaseInDatastoreBytesInUse_givenHashValueSizeIncreases() {
+ jedis1.hset(HASH_KEY, FIELD, "value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.hset(HASH_KEY, FIELD + i, LONG_APPEND_VALUE);
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+
assertThat(finalDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void should_showDecreaseInDatastoreBytesInUse_givenHashValueDeleted()
{
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ jedis1.hset(HASH_KEY, FIELD, "value");
+
+ long intermediateDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
assertThat(intermediateDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+
+ jedis1.del(HASH_KEY);
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showNoIncreaseInDatastoreBytesInUse_givenHSetDoesNotIncreaseHashSize() {
+ jedis2.hset(HASH_KEY, FIELD, "initialvalue"); // two hsets are required to
force
+ jedis2.hset(HASH_KEY, FIELD, "value"); // deserialization on both servers
+ // otherwise primary/secondary can disagree on size, and which server is
primary varies
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server2);
+
+ for (int i = 0; i < 10; i++) {
+ jedis2.hset(HASH_KEY, FIELD, "value");
+ }
+
+ assertThat(jedis2.hgetAll(HASH_KEY).size()).isEqualTo(1);
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server2);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showIncreaseInDatastoreBytesInUse_givenHSetNXIncreasesHashSize() {
+ jedis1.hset(HASH_KEY, FIELD, "value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.hsetnx(HASH_KEY, FIELD + i, "value");
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+
assertThat(finalDataStoreBytesInUse).isGreaterThan(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ public void
should_showNoIncreaseInDatastoreBytesInUse_givenHSetNXDoesNotIncreaseHashSize()
{
+ jedis1.hset(HASH_KEY, FIELD, "value");
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ for (int i = 0; i < 1000; i++) {
+ jedis1.hsetnx(HASH_KEY, FIELD, "value");
+ }
+
+ long finalDataStoreBytesInUse =
clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(finalDataStoreBytesInUse).isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ /******* confirm that the other member agrees upon size *******/
+
+ @Test
+ public void
should_showMembersAgreeUponUsedHashMemory_afterDeltaPropagation() {
+ jedis1.hset(HASH_KEY, FIELD, "initialvalue"); // two hsets are required to
force
+ jedis1.hset(HASH_KEY, FIELD, "finalvalue"); // deserialization on both
servers
+ // otherwise primary/secondary can disagree on size, and which server is
primary varies
+
+ long initialDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server2);
+
+ for (int i = 0; i < 10; i++) {
+ jedis1.hset(HASH_KEY, FIELD, "finalvalue");
+ }
+
+ assertThat(jedis1.hgetAll(HASH_KEY).size()).isEqualTo(1);
+
+ long server2FinalDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server2);
+ long server1FinalDataStoreBytesInUse =
+ clusterStartUpRule.getDataStoreBytesInUseForDataRegion(server1);
+
+ assertThat(server1FinalDataStoreBytesInUse)
+ .isEqualTo(server2FinalDataStoreBytesInUse)
+ .isEqualTo(initialDataStoreBytesInUse);
+ }
+
+ @Test
+ @Ignore("confirm that bucket size stats are being calculated correctly
before enabling")
+ public void should_showMembersAgreeUponUsedSetMemory_afterDeltaPropagation()
{
Review comment:
Are we able to remove this `@Ignore` now?
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
##########
@@ -351,4 +354,50 @@ public void
setExpirationTimestamp_stores_delta_that_is_stable() throws IOExcept
o2.fromDelta(in);
assertThat(o2).isEqualTo(o1);
}
+
+ @Test
+ public void overheadConstants_shouldNotChange_withoutForethoughtAndTesting()
{
+ assertThat(RedisString.PER_OBJECT_OVERHEAD).isEqualTo(8);
+
assertThat(RedisString.getPerStringOverhead()).isEqualTo(RedisString.PER_STRING_OVERHEAD);
Review comment:
The `getPerStringOverhead` method just returns the same constant we are
checking against. Could we test this constant using the ROS size of
`RedisString` like we did for `RedisHash`? Also, could we change the name of
the test to something like
`constantRedisStringOverhead_shouldEqual_calculatedOverhead` ?
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
##########
@@ -351,4 +354,50 @@ public void
setExpirationTimestamp_stores_delta_that_is_stable() throws IOExcept
o2.fromDelta(in);
assertThat(o2).isEqualTo(o1);
}
+
+ @Test
+ public void overheadConstants_shouldNotChange_withoutForethoughtAndTesting()
{
+ assertThat(RedisString.PER_OBJECT_OVERHEAD).isEqualTo(8);
+
assertThat(RedisString.getPerStringOverhead()).isEqualTo(RedisString.PER_STRING_OVERHEAD);
+ }
+
+ @SuppressWarnings("unchecked")
+ @Test
+ public void should_calculateSize_closeToROSSize_ofIndividualInstances() {
+ String javaString;
+ for (int i = 0; i < 512; i += 8) {
+ javaString = makeStringOfSpecifiedSize(i);
+ RedisString string = new RedisString(new
RedisKey(javaString.getBytes()));
+
+ int expected = reflectionObjectSizer.sizeof(string);
+ Long actual = Long.valueOf(string.getSizeInBytes());
Review comment:
Intellij complains of `unnecessary boxing` for this as well as the
similar line in the test below it... looks like it can just be cast to a `long`
vs. using `Long.valueOf()`. Apparently this is preferred due to space/time
performance.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -35,27 +35,61 @@
import org.apache.commons.lang3.tuple.ImmutablePair;
import org.apache.commons.lang3.tuple.Pair;
+import org.apache.geode.DataSerializer;
import org.apache.geode.annotations.VisibleForTesting;
import org.apache.geode.cache.Region;
-import org.apache.geode.internal.InternalDataSerializer;
import org.apache.geode.internal.serialization.DeserializationContext;
import org.apache.geode.internal.serialization.KnownVersion;
import org.apache.geode.internal.serialization.SerializationContext;
+import org.apache.geode.internal.size.ReflectionObjectSizer;
import org.apache.geode.redis.internal.delta.AddsDeltaInfo;
import org.apache.geode.redis.internal.delta.DeltaInfo;
import org.apache.geode.redis.internal.delta.RemsDeltaInfo;
public class RedisSet extends AbstractRedisData {
-
private HashSet<ByteArrayWrapper> members;
+ private static int baseRedissetOverhead;
+ private static int perMemberOverhead;
+ private static int internalHashsetStorageOverhead;
Review comment:
If we end up keeping this variable, set should be capitalized.
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisHashTest.java
##########
@@ -258,11 +260,373 @@ public void
hscanSnaphots_shouldExpireAfterExpiryPeriod() {
});
}
+ /************* Hash Size *************/
+ /******* constants *******/
+ @Test
+ public void constantValuePairOverhead_shouldEqualCalculatedOverhead() {
+ RedisHash redisHash = new RedisHash();
Review comment:
This variable is unused.
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
##########
@@ -351,4 +354,50 @@ public void
setExpirationTimestamp_stores_delta_that_is_stable() throws IOExcept
o2.fromDelta(in);
assertThat(o2).isEqualTo(o1);
}
+
+ @Test
+ public void overheadConstants_shouldNotChange_withoutForethoughtAndTesting()
{
+ assertThat(RedisString.PER_OBJECT_OVERHEAD).isEqualTo(8);
+
assertThat(RedisString.getPerStringOverhead()).isEqualTo(RedisString.PER_STRING_OVERHEAD);
+ }
+
+ @SuppressWarnings("unchecked")
+ @Test
+ public void should_calculateSize_closeToROSSize_ofIndividualInstances() {
+ String javaString;
+ for (int i = 0; i < 512; i += 8) {
+ javaString = makeStringOfSpecifiedSize(i);
+ RedisString string = new RedisString(new
RedisKey(javaString.getBytes()));
+
+ int expected = reflectionObjectSizer.sizeof(string);
+ Long actual = Long.valueOf(string.getSizeInBytes());
+ Offset<Long> offset = Offset.offset(Math.round(expected * 0.05));
+
+ assertThat(actual).isCloseTo(expected, offset);
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ @Test
+ public void should_calculateSize_closeToROSSize_ofLargeStrings() {
+ String javaString = makeStringOfSpecifiedSize(10_000);
+
+ RedisKey byteArrayWrapper = new RedisKey(javaString.getBytes());
+ RedisString string = new RedisString(byteArrayWrapper);
+
+ Long actual = Long.valueOf(string.getSizeInBytes());
+ int expected = reflectionObjectSizer.sizeof(string);
+ Offset<Long> offset = Offset.offset(Math.round(expected * 0.05));
+
+ assertThat(actual).isCloseTo(expected, offset);
+ }
+
+ private String makeStringOfSpecifiedSize(final int stringSize) {
+ StringBuffer sb = new StringBuffer(stringSize);
+ for (int i = 0; i < stringSize; i++) {
+ sb.append("a");
+ }
+ String javaString = sb.toString();
+ return javaString;
+ }
Review comment:
Do we need to add tests like the ones that are in `RedisHash` (e.g.
sizeShouldChangeWhenValueIsChanged,
shouldReturnToStringOverhead_whenValueIsRemoved, etc.)?
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
##########
@@ -351,4 +354,50 @@ public void
setExpirationTimestamp_stores_delta_that_is_stable() throws IOExcept
o2.fromDelta(in);
assertThat(o2).isEqualTo(o1);
}
+
+ @Test
+ public void overheadConstants_shouldNotChange_withoutForethoughtAndTesting()
{
+ assertThat(RedisString.PER_OBJECT_OVERHEAD).isEqualTo(8);
+
assertThat(RedisString.getPerStringOverhead()).isEqualTo(RedisString.PER_STRING_OVERHEAD);
+ }
+
+ @SuppressWarnings("unchecked")
+ @Test
+ public void should_calculateSize_closeToROSSize_ofIndividualInstances() {
+ String javaString;
+ for (int i = 0; i < 512; i += 8) {
+ javaString = makeStringOfSpecifiedSize(i);
+ RedisString string = new RedisString(new
RedisKey(javaString.getBytes()));
+
+ int expected = reflectionObjectSizer.sizeof(string);
+ Long actual = Long.valueOf(string.getSizeInBytes());
+ Offset<Long> offset = Offset.offset(Math.round(expected * 0.05));
+
+ assertThat(actual).isCloseTo(expected, offset);
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ @Test
+ public void should_calculateSize_closeToROSSize_ofLargeStrings() {
+ String javaString = makeStringOfSpecifiedSize(10_000);
+
+ RedisKey byteArrayWrapper = new RedisKey(javaString.getBytes());
Review comment:
It seems strange that we make a new `RedisKey` here as well as the test
above this vs. creating a new `ByteArrayWrapper`.
--
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]
