You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by es...@apache.org on 2018/11/30 23:09:53 UTC
[geode] branch develop updated: GEODE-6108: Handle client
putIfAbsent returns value due to retry (#2925)
This is an automated email from the ASF dual-hosted git repository.
eshu11 pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new 16dde1a GEODE-6108: Handle client putIfAbsent returns value due to retry (#2925)
16dde1a is described below
commit 16dde1ae47bfb1e3c89fef7cf77a154cb55e50dd
Author: pivotal-eshu <es...@pivotal.io>
AuthorDate: Fri Nov 30 15:09:44 2018 -0800
GEODE-6108: Handle client putIfAbsent returns value due to retry (#2925)
* GEODE-6108: Handle client putIfAbsent returns value due to retry
* Check putIfAbsent return value from server if it could caused by retry.
* Make sure client putIfAbsent operation message.isRetry is set if it retries after failed when using singleHop
---
.../apache/geode/cache/client/internal/PutOp.java | 7 +-
.../geode/internal/cache/EntryEventImpl.java | 10 ++
.../apache/geode/internal/cache/LocalRegion.java | 27 +++-
.../geode/internal/cache/LocalRegionTest.java | 143 +++++++++++++++++++++
4 files changed, 182 insertions(+), 5 deletions(-)
diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/PutOp.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/PutOp.java
index de56cce..580aba9 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/client/internal/PutOp.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/PutOp.java
@@ -82,11 +82,16 @@ public class PutOp {
if (e instanceof ServerOperationException) {
throw e; // fixed 44656
}
+ op.getMessage().setIsRetry();
cms.removeBucketServerLocation(server);
}
}
}
- return pool.execute(op);
+ Object result = pool.execute(op);
+ if (op.getMessage().isRetry()) {
+ event.setRetried(true);
+ }
+ return result;
}
public static Object execute(ExecutablePool pool, String regionName, Object key, Object value,
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java b/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
index a6db3db..ff27adf 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
@@ -181,6 +181,8 @@ public class EntryEventImpl implements InternalEntryEvent, InternalCacheEvent,
private transient boolean isPendingSecondaryExpireDestroy = false;
+ private transient boolean hasRetried = false;
+
public static final Object SUSPECT_TOKEN = new Object();
public EntryEventImpl() {
@@ -610,6 +612,14 @@ public class EntryEventImpl implements InternalEntryEvent, InternalCacheEvent,
return this.isEvicted;
}
+ public boolean hasRetried() {
+ return hasRetried;
+ }
+
+ public void setRetried(boolean retried) {
+ hasRetried = retried;
+ }
+
public boolean isPendingSecondaryExpireDestroy() {
return this.isPendingSecondaryExpireDestroy;
}
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
index 90089f4..488cd58 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
@@ -3041,10 +3041,7 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory,
event.setConcurrentMapOldValue(result);
}
if (op == Operation.PUT_IF_ABSENT) {
- if (result != null) {
- // customers don't see this exception
- throw new EntryNotFoundException("entry existed for putIfAbsent");
- }
+ checkPutIfAbsentResult(event, value, result);
} else if (op == Operation.REPLACE) {
if (requireOldValue && result == null) {
throw new EntryNotFoundException("entry not found for replace");
@@ -3060,6 +3057,28 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory,
}
}
+ void checkPutIfAbsentResult(EntryEventImpl event, Object value, Object result) {
+ if (result != null) {
+ // we may see a non null result possibly due to retry
+ if (event.hasRetried() && putIfAbsentResultHasSameValue(value, result)) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("retried putIfAbsent and result is the value to be put,"
+ + " treat as a successful putIfAbsent");
+ }
+ } else {
+ // customers don't see this exception
+ throw new EntryNotFoundException("entry existed for putIfAbsent");
+ }
+ }
+ }
+
+ boolean putIfAbsentResultHasSameValue(Object value, Object result) {
+ if (Token.isInvalid(result)) {
+ return value == null;
+ }
+ return result.equals(value);
+ }
+
/**
* Destroy an entry on the server given its event.
*
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/LocalRegionTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/LocalRegionTest.java
new file mode 100644
index 0000000..771d360
--- /dev/null
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/LocalRegionTest.java
@@ -0,0 +1,143 @@
+/*
+ * 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.internal.cache;
+
+import static org.assertj.core.api.Java6Assertions.assertThat;
+import static org.mockito.Mockito.doCallRealMethod;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.geode.CancelCriterion;
+import org.apache.geode.cache.EntryNotFoundException;
+import org.apache.geode.cache.Operation;
+import org.apache.geode.cache.client.internal.ServerRegionProxy;
+
+public class LocalRegionTest {
+ private LocalRegion region;
+ private EntryEventImpl event;
+ private ServerRegionProxy serverRegionProxy;
+ private Operation operation;
+ private CancelCriterion cancelCriterion;
+
+ private final Object key = new Object();
+ private final String value = "value";
+
+ @Before
+ public void setup() {
+ region = mock(LocalRegion.class);
+ event = mock(EntryEventImpl.class);
+ serverRegionProxy = mock(ServerRegionProxy.class);
+ cancelCriterion = mock(CancelCriterion.class);
+
+ when(region.getServerProxy()).thenReturn(serverRegionProxy);
+ when(event.isFromServer()).thenReturn(false);
+ when(event.getKey()).thenReturn(key);
+ when(event.getRawNewValue()).thenReturn(value);
+ when(region.getCancelCriterion()).thenReturn(cancelCriterion);
+ }
+
+ @Test
+ public void serverPutWillCheckPutIfAbsentResult() {
+ Object result = new Object();
+ operation = Operation.PUT_IF_ABSENT;
+ when(event.getOperation()).thenReturn(operation);
+ when(event.isCreate()).thenReturn(true);
+ when(serverRegionProxy.put(key, value, null, event, operation, true, null, null, true))
+ .thenReturn(result);
+ doCallRealMethod().when(region).serverPut(event, true, null);
+
+ region.serverPut(event, true, null);
+
+ verify(region).checkPutIfAbsentResult(event, value, result);
+ }
+
+ @Test
+ public void checkPutIfAbsentResultSucceedsIfResultIsNull() {
+ Object result = null;
+ doCallRealMethod().when(region).checkPutIfAbsentResult(event, value, result);
+
+ region.checkPutIfAbsentResult(event, value, result);
+
+ verify(event, never()).hasRetried();
+ }
+
+ @Test(expected = EntryNotFoundException.class)
+ public void checkPutIfAbsentResultThrowsIfResultNotNullAndEventHasNotRetried() {
+ Object result = new Object();
+ when(event.hasRetried()).thenReturn(false);
+ doCallRealMethod().when(region).checkPutIfAbsentResult(event, value, result);
+
+ region.checkPutIfAbsentResult(event, value, result);
+ }
+
+ @Test(expected = EntryNotFoundException.class)
+ public void checkPutIfAbsentResultThrowsIfEventHasRetriedButResultNotHaveSameValue() {
+ Object result = new Object();
+ when(event.hasRetried()).thenReturn(true);
+ when(region.putIfAbsentResultHasSameValue(value, result)).thenReturn(false);
+ doCallRealMethod().when(region).checkPutIfAbsentResult(event, value, result);
+
+ region.checkPutIfAbsentResult(event, value, result);
+ }
+
+ @Test
+ public void checkPutIfAbsentResultSucceedsIfEventHasRetriedAndResultHasSameValue() {
+ Object result = new Object();
+ when(event.hasRetried()).thenReturn(true);
+ when(region.putIfAbsentResultHasSameValue(value, result)).thenReturn(true);
+ doCallRealMethod().when(region).checkPutIfAbsentResult(event, value, result);
+
+ region.checkPutIfAbsentResult(event, value, result);
+
+ verify(event).hasRetried();
+ verify(region).putIfAbsentResultHasSameValue(value, result);
+ }
+
+ @Test
+ public void putIfAbsentResultHasSameValueReturnTrueIfResultIsInvalidTokenAndValueToBePutIsNull() {
+ when(region.putIfAbsentResultHasSameValue(null, Token.INVALID)).thenCallRealMethod();
+
+ assertThat(region.putIfAbsentResultHasSameValue(null, Token.INVALID)).isTrue();
+ }
+
+ @Test
+ public void putIfAbsentResultHasSameValueReturnFalseIfResultIsInvalidTokenAndValueToBePutIsNotNull() {
+ when(region.putIfAbsentResultHasSameValue(value, Token.INVALID)).thenCallRealMethod();
+
+ assertThat(region.putIfAbsentResultHasSameValue(value, Token.INVALID)).isFalse();
+ }
+
+ @Test
+ public void putIfAbsentResultHasSameValueReturnTrueIfResultHasSameValue() {
+ Object result = "value";
+ when(region.putIfAbsentResultHasSameValue(value, result)).thenCallRealMethod();
+
+ assertThat(region.putIfAbsentResultHasSameValue(value, result)).isTrue();
+ }
+
+ @Test
+ public void putIfAbsentResultHasSameValueReturnFalseIfResultDoesNotHaveSameValue() {
+ Object result = "differentValue";
+ when(region.putIfAbsentResultHasSameValue(value, result)).thenCallRealMethod();
+
+ assertThat(region.putIfAbsentResultHasSameValue(value, result)).isFalse();
+ }
+
+}