[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7290: GEODE-9885: Handle duplicated appends in Redis StringsDUnitTest

[GitBox <gi...@apache.org>]

dschneider-pivotal commented on a change in pull request #7290:
URL: https://github.com/apache/geode/pull/7290#discussion_r789868938



##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -469,7 +469,7 @@ public void handleSetExpiration(SetOptions options) {
 
   ////// methods that modify the "value" field ////////////
 
-  protected void valueAppend(byte[] bytes) {
+  protected synchronized void valueAppend(byte[] bytes) {

Review comment:
       Could you also update the comment on line 365 to not refer to "appendSequence" which no longer exists?

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -469,7 +469,7 @@ public void handleSetExpiration(SetOptions options) {
 
   ////// methods that modify the "value" field ////////////
 
-  protected void valueAppend(byte[] bytes) {
+  protected synchronized void valueAppend(byte[] bytes) {

Review comment:
       My understanding is that this method does not need to be synchronized because it does not modify "value" in place. It just reads value and copies it into a new byte array and then at the end sets the value field. We do this same thing in other places (for example incr). Since our concern here is thread safety with "toData", which only reads "value", we would only cause corruption for toData if we modified "value" in place. So this method does not need to be synchronized. But this line of applyReplaceByteArrayAtOffsetDelta does need to be sync'd:
         System.arraycopy(valueToAdd, 0, value, offset, valueToAdd.length);
   




-- 
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: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org