[GitHub] [geode] boglesby commented on a change in pull request #7235: GEODE-9913: Added basicBridgePut condition to ensure VersionTag set

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

boglesby commented on a change in pull request #7235:
URL: https://github.com/apache/geode/pull/7235#discussion_r778461809



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
##########
@@ -5237,6 +5237,9 @@ public boolean basicBridgePut(Object key, Object value, byte[] deltaBytes, boole
       if (success) {
         clientEvent.setVersionTag(event.getVersionTag());
         getCachePerfStats().endPut(startPut, event.isOriginRemote());
+      } else if (clientEvent.isConcurrencyConflict() && clientEvent.isPossibleDuplicate()
+          && clientEvent.getVersionTag() == null) {

Review comment:
       Barry in line 5209 we set the event.setVersionTag(clientEvent.getVersionTag()) and then here we do the reverse. I was wondering if it was already set.
   ```
   Thats exactly the issue. Its not already set in the clientEvent.
   
   The sequence here is:
   
   - the clientEvent (an EventIDHolder) is created in Put70.
   
   - it sets the VersionTag (see BaseCommand.recoverVersionTagForRetriedOperation). This is null.
   
   java.lang.Exception
   	at org.apache.geode.internal.cache.FindVersionTagOperation.findVersionTag(FindVersionTagOperation.java:44)
   	at org.apache.geode.internal.cache.tier.sockets.BaseCommand.recoverVersionTagForRetriedOperation(BaseCommand.java:237)
   	at org.apache.geode.internal.cache.tier.sockets.command.Put70.shouldSetPossibleDuplicate(Put70.java:477)
   	at org.apache.geode.internal.cache.tier.sockets.command.Put70.cmdExecute(Put70.java:225)
   
   - it then sets the VersionTag in the EntryEventImpl based on the clientEvent (line 5209 as you mentioned)
   
   - If the EntryEventImpl's VersionTag is null, the second call to FindVersionTagOperation.findVersionTag is in DistributedRegion.hasSeenEvent. In this case, its not null.
   
   java.lang.Exception
   	at org.apache.geode.internal.cache.FindVersionTagOperation.findVersionTag(FindVersionTagOperation.java:44)
   	at org.apache.geode.internal.cache.DistributedRegion.hasSeenEvent(DistributedRegion.java:539)
   	at org.apache.geode.internal.cache.DistributedRegion.virtualPut(DistributedRegion.java:359)
   	at org.apache.geode.internal.cache.LocalRegionDataView.putEntry(LocalRegionDataView.java:171)
   	at org.apache.geode.internal.cache.LocalRegion.basicUpdate(LocalRegion.java:5579)
   	at org.apache.geode.internal.cache.LocalRegion.basicUpdate(LocalRegion.java:5539)
   	at org.apache.geode.internal.cache.LocalRegion.basicBridgePut(LocalRegion.java:5218)
   	at org.apache.geode.internal.cache.tier.sockets.command.Put70.cmdExecute(Put70.java:388)
   
   Then, the ConcurrentCacheModificationException occurs.
   
   At this point, the clientEvent's VersionTag is null, the EntryEventImpl's VersionTag is set and its a concurrency conflict.
   
   The condition I added, sets the clientEvent's VersionTag in this case. I think I don't need the isPossibleDuplicate check in that condition. I'll probably change this.
   ```
   Also in GatewayReceiverCommand, the clientEvent.versionTag is set as VersionTag.create(region.getVersionMember());
   
   Was just wondering what the difference is these two ways.
   ```
   The GatewayReceiverCommand creates a new VersionTag every time based on the local member id and remote DS id and timestamp.
   
   VersionTag tag = VersionTag.create(region.getVersionMember());
   tag.setIsGatewayTag(true);
   tag.setVersionTimeStamp(versionTimeStamp);
   tag.setDistributedSystemId(dsid);
   clientEvent.setVersionTag(tag);
   
   It then processes that VersionTag in AbstractRegionEntry.processGatewayTag to see if its a conflict based on the DS id and timestamp. So, it doesn't use the local VersionTag at all.
   ```




-- 
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