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 2022/01/04 17:28:34 UTC
[GitHub] [geode] boglesby opened a new pull request #7235: GEODE-9913: Added basicBridgePut condition to ensure VersionTag set
boglesby opened a new pull request #7235:
URL: https://github.com/apache/geode/pull/7235
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
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
[GitHub] [geode] kirklund commented on a change in pull request #7235: GEODE-9913: Added basicBridgePut condition to ensure VersionTag set
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7235:
URL: https://github.com/apache/geode/pull/7235#discussion_r780453781
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/LocalRegionTest.java
##########
@@ -290,4 +293,48 @@ public void forPdxInstanceByPassTheFutureInLocalRegionOptimizedGetObject() {
assertThat(object).isNotSameAs(result);
assertThat(object).isSameAs(newResult);
}
+
+ @Test
+ public void verifyBasicBridgePutSetsVersionTagOnClientEventIfConcurrencyConflictAndPossibleDuplicate() {
+ // Create the region
+ LocalRegion region =
Review comment:
You might be able to use a mock of `InternalRegion` instead. You could add method(s) to `InternalRegion` if needed.
--
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
[GitHub] [geode] boglesby merged pull request #7235: GEODE-9913: Added basicBridgePut condition to ensure VersionTag set
Posted by GitBox <gi...@apache.org>.
boglesby merged pull request #7235:
URL: https://github.com/apache/geode/pull/7235
--
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
[GitHub] [geode] boglesby commented on a change in pull request #7235: GEODE-9913: Added basicBridgePut condition to ensure VersionTag set
Posted by 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
[GitHub] [geode] kirklund commented on a change in pull request #7235: GEODE-9913: Added basicBridgePut condition to ensure VersionTag set
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #7235:
URL: https://github.com/apache/geode/pull/7235#discussion_r780453781
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/LocalRegionTest.java
##########
@@ -290,4 +293,48 @@ public void forPdxInstanceByPassTheFutureInLocalRegionOptimizedGetObject() {
assertThat(object).isNotSameAs(result);
assertThat(object).isSameAs(newResult);
}
+
+ @Test
+ public void verifyBasicBridgePutSetsVersionTagOnClientEventIfConcurrencyConflictAndPossibleDuplicate() {
+ // Create the region
+ LocalRegion region =
Review comment:
You might be able to use a mock of `InternalRegion` instead. You could add method(s) to `InternalRegion` if needed.
--
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
[GitHub] [geode] boglesby commented on a change in pull request #7235: GEODE-9913: Added basicBridgePut condition to ensure VersionTag set
Posted by GitBox <gi...@apache.org>.
boglesby commented on a change in pull request #7235:
URL: https://github.com/apache/geode/pull/7235#discussion_r783513412
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/LocalRegionTest.java
##########
@@ -290,4 +293,48 @@ public void forPdxInstanceByPassTheFutureInLocalRegionOptimizedGetObject() {
assertThat(object).isNotSameAs(result);
assertThat(object).isSameAs(newResult);
}
+
+ @Test
+ public void verifyBasicBridgePutSetsVersionTagOnClientEventIfConcurrencyConflictAndPossibleDuplicate() {
+ // Create the region
+ LocalRegion region =
Review comment:
Thanks for the comment.
I looked into this a bit. I need to invoke the real LocalRegion.basicBridgePut. I could mock LocalRegion, but I get stuck on setting the entryEventFactory field which is directly referenced in that method. If I add a getter for that field, I see this when I run the test:
```
Following stubbings are unnecessary (click to navigate to relevant line of code):
1. -> at org.apache.geode.internal.cache.LocalRegionTest.setUp(LocalRegionTest.java:103)
2. -> at org.apache.geode.internal.cache.LocalRegionTest.setUp(LocalRegionTest.java:104)
3. -> at org.apache.geode.internal.cache.LocalRegionTest.setUp(LocalRegionTest.java:105)
4. -> at org.apache.geode.internal.cache.LocalRegionTest.setUp(LocalRegionTest.java:106)
5. -> at org.apache.geode.internal.cache.LocalRegionTest.setUp(LocalRegionTest.java:107)
6. -> at org.apache.geode.internal.cache.LocalRegionTest.setUp(LocalRegionTest.java:108)
7. -> at org.apache.geode.internal.cache.LocalRegionTest.setUp(LocalRegionTest.java:109)
8. -> at org.apache.geode.internal.cache.LocalRegionTest.setUp(LocalRegionTest.java:110)
9. -> at org.apache.geode.internal.cache.LocalRegionTest.setUp(LocalRegionTest.java:111)
10. -> at org.apache.geode.internal.cache.LocalRegionTest.setUp(LocalRegionTest.java:112)
11. -> at org.apache.geode.internal.cache.LocalRegionTest.setUp(LocalRegionTest.java:113)
```
These point to a bunch of `when/thenReturn` calls in setUp which a bunch of other tests use.
If I remove those calls, the test passes, but then I'd have to modify all the other tests that use those calls.
I think I'll leave it as is for now.
--
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
[GitHub] [geode] nabarunnag commented on a change in pull request #7235: GEODE-9913: Added basicBridgePut condition to ensure VersionTag set
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on a change in pull request #7235:
URL: https://github.com/apache/geode/pull/7235#discussion_r778326793
##########
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.
Also in GatewayReceiverCommand, the clientEvent.versionTag is set as VersionTag.create(region.getVersionMember());
Was just wondering what the difference is these two ways.
--
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
[GitHub] [geode] jchen21 commented on a change in pull request #7235: GEODE-9913: Added basicBridgePut condition to ensure VersionTag set
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #7235:
URL: https://github.com/apache/geode/pull/7235#discussion_r778363527
##########
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) {
+ clientEvent.setVersionTag(event.getVersionTag());
Review comment:
Is `stopper.checkCancelInProgress(null);` needed here as well?
--
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
[GitHub] [geode] boglesby commented on a change in pull request #7235: GEODE-9913: Added basicBridgePut condition to ensure VersionTag set
Posted by GitBox <gi...@apache.org>.
boglesby commented on a change in pull request #7235:
URL: https://github.com/apache/geode/pull/7235#discussion_r778478441
##########
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) {
+ clientEvent.setVersionTag(event.getVersionTag());
Review comment:
Good question. I don't think so. It'll fall through to that check if this condition fails.
--
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