You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/05/12 14:16:36 UTC
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6464: GEODE-9221: Remove uses of ByteArrayWrapper from RedisString
jdeppe-pivotal commented on a change in pull request #6464:
URL: https://github.com/apache/geode/pull/6464#discussion_r631037662
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -684,23 +675,23 @@ public boolean equals(Object o) {
return false;
}
RedisString that = (RedisString) o;
- return Objects.equals(value, that.value);
+ return Arrays.equals(value, that.value);
}
@Override
public int hashCode() {
return Objects.hash(super.hashCode(), value);
}
- ByteArrayWrapper getValue() {
+ byte[] getValue() {
return value;
}
@Override
public String toString() {
return "RedisString{" +
super.toString() + ", " +
- "value=" + value +
+ "value=" + new String(value, StandardCharsets.UTF_8) +
Review comment:
This should not do any explicit encoding; since we wouldn't know what the original encoding was.
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
##########
@@ -43,85 +45,81 @@
@BeforeClass
public static void beforeClass() {
- InternalDataSerializer
- .getDSFIDSerializer()
- .registerDSFID(DataSerializableFixedID.REDIS_BYTE_ARRAY_WRAPPER, ByteArrayWrapper.class);
InternalDataSerializer.getDSFIDSerializer().registerDSFID(
DataSerializableFixedID.REDIS_STRING_ID,
RedisString.class);
}
@Test
public void constructorSetsValue() {
- ByteArrayWrapper byteArrayWrapper = new ByteArrayWrapper(new byte[] {0, 1, 2});
- RedisString string = new RedisString(byteArrayWrapper);
- ByteArrayWrapper returnedByteArrayWrapper = string.get();
- assertThat(returnedByteArrayWrapper).isNotNull();
- assertThat(returnedByteArrayWrapper.value).isEqualTo(byteArrayWrapper.value);
+ byte[] bytes = {0, 1, 2};
+ RedisString string = new RedisString(bytes);
+ byte[] returnedBytes = string.get();
+ assertThat(returnedBytes).isNotNull();
+ assertThat(returnedBytes).isEqualTo(bytes);
}
@Test
public void setSetsValue() {
RedisString string = new RedisString();
- string.set(new ByteArrayWrapper(new byte[] {0, 1, 2}));
- ByteArrayWrapper returnedByteArrayWrapper = string.get();
- assertThat(returnedByteArrayWrapper).isNotNull();
- assertThat(returnedByteArrayWrapper.value)
- .isEqualTo(new ByteArrayWrapper(new byte[] {0, 1, 2}).value);
+ byte[] bytes = {0, 1, 2};
+ string.set(bytes);
+ byte[] returnedBytes = string.get();
+ assertThat(returnedBytes).isNotNull();
+ assertThat(returnedBytes).isEqualTo(bytes);
}
@Test
public void getReturnsSetValue() {
- RedisString string = new RedisString(new ByteArrayWrapper(new byte[] {0, 1}));
- ByteArrayWrapper returnedByteArrayWrapper = string.get();
- assertThat(returnedByteArrayWrapper).isNotNull();
- assertThat(returnedByteArrayWrapper.value)
- .isEqualTo(new ByteArrayWrapper(new byte[] {0, 1}).value);
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ byte[] returnedBytes = string.get();
+ assertThat(returnedBytes).isNotNull();
+ assertThat(returnedBytes).isEqualTo(bytes);
}
@Test
public void appendResizesByteArray() {
- // allows unchecked cast of mock to Region<RedisKey, RedisData>
- @SuppressWarnings("unchecked")
- Region<RedisKey, RedisData> region = mock(Region.class);
- RedisString redisString = new RedisString(new ByteArrayWrapper(new byte[] {0, 1}));
- ByteArrayWrapper part2 = new ByteArrayWrapper(new byte[] {2, 3, 4, 5});
+ Region<RedisKey, RedisData> region = uncheckedCast(mock(Region.class));
+ RedisString redisString = new RedisString(new byte[] {0, 1});
int redisStringSize = redisString.strlen();
- int part2Size = part2.length();
- int appendedStringSize = redisString.append(part2, region, null);
- assertThat(appendedStringSize).isEqualTo(redisStringSize + part2Size);
+ byte[] bytesToAppend = {2, 3, 4, 5};
+ int appendedSize = bytesToAppend.length;
+ int appendedStringSize = redisString.append(region, null, bytesToAppend);
+ assertThat(appendedStringSize).isEqualTo(redisStringSize + appendedSize);
}
@Test
public void appendStoresStableDelta() throws IOException {
- // allows unchecked cast of mock to Region<RedisKey, RedisData>
- @SuppressWarnings("unchecked")
- Region<RedisKey, RedisData> region = mock(Region.class);
- RedisString o1 = new RedisString(new ByteArrayWrapper(new byte[] {0, 1}));
- ByteArrayWrapper part2 = new ByteArrayWrapper(new byte[] {2, 3});
- o1.append(part2, region, null);
- assertThat(o1.hasDelta()).isTrue();
- assertThat(o1.get()).isEqualTo(new ByteArrayWrapper(new byte[] {0, 1, 2, 3}));
+ Region<RedisKey, RedisData> region = uncheckedCast(mock(Region.class));
+ byte[] baseBytes = {0, 1};
+ byte[] bytesToAppend = {2, 3};
+ byte[] baseAndAppendedBytes = {0, 1, 2, 3};
+
+ RedisString stringOne = new RedisString(baseBytes);
+ stringOne.append(region, null, bytesToAppend);
+ assertThat(stringOne.hasDelta()).isTrue();
+ assertThat(stringOne.get()).isEqualTo(baseAndAppendedBytes);
HeapDataOutputStream out = new HeapDataOutputStream(100);
- o1.toDelta(out);
- assertThat(o1.hasDelta()).isFalse();
+ stringOne.toDelta(out);
+ assertThat(stringOne.hasDelta()).isFalse();
ByteArrayDataInput in = new ByteArrayDataInput(out.toByteArray());
- RedisString o2 = new RedisString(new ByteArrayWrapper(new byte[] {0, 1}));
- assertThat(o2).isNotEqualTo(o1);
- o2.fromDelta(in);
- assertThat(o2.get()).isEqualTo(new ByteArrayWrapper(new byte[] {0, 1, 2, 3}));
- assertThat(o2).isEqualTo(o1);
+ RedisString stringTwo = new RedisString(baseBytes);
+ assertThat(stringTwo).isNotEqualTo(stringOne);
+ stringTwo.fromDelta(in);
+ assertThat(stringTwo.get()).isEqualTo(baseAndAppendedBytes);
+ assertThat(stringTwo).isEqualTo(stringOne);
}
@Test
public void confirmSerializationIsStable() throws IOException, ClassNotFoundException {
- RedisString o1 = new RedisString(new ByteArrayWrapper(new byte[] {0, 1, 2, 3}));
- o1.setExpirationTimestampNoDelta(1000);
+ RedisString stringOne = new RedisString(new byte[] {0, 1, 2, 3});
+ stringOne.setExpirationTimestampNoDelta(1000);
HeapDataOutputStream outputStream = new HeapDataOutputStream(100);
- DataSerializer.writeObject(o1, outputStream);
+ DataSerializer.writeObject(stringOne, outputStream);
ByteArrayDataInput dataInput = new ByteArrayDataInput(outputStream.toByteArray());
- RedisString o2 = DataSerializer.readObject(dataInput);
- assertThat(o2).isEqualTo(o1);
+ RedisString stringTwo = DataSerializer.readObject(dataInput);
+ assertThat(stringTwo).isEqualTo(stringOne);
Review comment:
Since the expiration is being set on `stringOne`, it would probably make sense to also assert it on `stringTwo`.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -233,18 +227,17 @@ public int bitpos(Region<RedisKey, RedisData> region, RedisKey key, int bit,
if (start > length) {
start = length - 1;
}
- if (end > length) {
+ if (end >= length) {
Review comment:
Well spotted! Please would you also add a test that validates this change.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -718,15 +709,16 @@ protected void handleSetExpiration(SetOptions options) {
////// methods that modify the "value" field ////////////
protected void valueAppend(byte[] bytes) {
- value.append(bytes);
- }
-
- protected void valueSet(ByteArrayWrapper newValue) {
- value = newValue;
+ int initialLength = value.length;
Review comment:
Since this is being pulled from `ByteArrayWrapper`, I think it can be deleted from that class now.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -718,15 +709,16 @@ protected void handleSetExpiration(SetOptions options) {
////// methods that modify the "value" field ////////////
protected void valueAppend(byte[] bytes) {
- value.append(bytes);
- }
-
- protected void valueSet(ByteArrayWrapper newValue) {
- value = newValue;
+ int initialLength = value.length;
+ int additionalLength = bytes.length;
+ byte[] combined = new byte[initialLength + additionalLength];
+ System.arraycopy(value, 0, combined, 0, initialLength);
+ System.arraycopy(bytes, 0, combined, initialLength, additionalLength);
+ value = combined;
}
- protected void valueSetBytes(byte[] bytes) {
- value.setBytes(bytes);
+ protected void valueSet(byte[] bytes) {
Review comment:
This class uses a mix of this method and direct assignment to `value`. I'd prefer just one approach. Don't mind which, but would lean towards just direct `value = ...`
--
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:
users@infra.apache.org