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 <e...@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(); + } + +}