This is an automated email from the ASF dual-hosted git repository. zhouxj pushed a commit to branch feature/GEODE-Test6149 in repository https://gitbox.apache.org/repos/asf/geode.git
commit 7cd26a77d05639dc17d1eaef16105277e05c9776 Author: zhouxh <[email protected]> AuthorDate: Thu Dec 6 22:49:09 2018 -0800 Revert "GEODE-6149: when client's cache is closing, its GetClientPRMetaDataOp could end up with NPE (#2952)" Test6149 --- .../client/internal/GetClientPRMetaDataOp.java | 4 +- .../management/internal/MBeanProxyFactory.java | 8 +++- .../internal/GetClientPRMetaDataOpJUnitTest.java | 50 ---------------------- .../management/internal/MBeanProxyFactoryTest.java | 24 ++--------- gradle/java.gradle | 2 +- 5 files changed, 12 insertions(+), 76 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/GetClientPRMetaDataOp.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/GetClientPRMetaDataOp.java index 8fcf566..c82860d 100755 --- a/geode-core/src/main/java/org/apache/geode/cache/client/internal/GetClientPRMetaDataOp.java +++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/GetClientPRMetaDataOp.java @@ -105,9 +105,7 @@ public class GetClientPRMetaDataOp { "GetClientPRMetaDataOpImpl#processResponse: for bucketId : {} locations are {}", bucketId, locations); } - if (advisor != null) { - advisor.updateBucketServerLocations(bucketId, locations, cms); - } + advisor.updateBucketServerLocations(bucketId, locations, cms); Set<ClientPartitionAdvisor> cpas = cms.getColocatedClientPartitionAdvisor(regionFullPath); diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/MBeanProxyFactory.java b/geode-core/src/main/java/org/apache/geode/management/internal/MBeanProxyFactory.java index cbbd516..e54ce8a 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/MBeanProxyFactory.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/MBeanProxyFactory.java @@ -166,8 +166,7 @@ public class MBeanProxyFactory { removeProxy(member, mbeanName, val); } catch (EntryNotFoundException entryNotFoundException) { // Entry has already been removed by another thread, so no need to remove it - logger.warn("Proxy for entry {} and member {} has already been removed", entry, - member.getId()); + logProxyAlreadyRemoved(member, entry); } catch (Exception e) { if (!(e.getCause() instanceof InstanceNotFoundException)) { logger.warn("Remove Proxy failed for {} due to {}", key, e.getMessage(), e); @@ -269,4 +268,9 @@ public class MBeanProxyFactory { } + void logProxyAlreadyRemoved(DistributedMember member, Entry<String, Object> entry) { + logger.warn("Proxy for entry {} and member {} has already been removed", entry, + member.getId()); + } + } diff --git a/geode-core/src/test/java/org/apache/geode/cache/client/internal/GetClientPRMetaDataOpJUnitTest.java b/geode-core/src/test/java/org/apache/geode/cache/client/internal/GetClientPRMetaDataOpJUnitTest.java deleted file mode 100644 index a1208e5..0000000 --- a/geode-core/src/test/java/org/apache/geode/cache/client/internal/GetClientPRMetaDataOpJUnitTest.java +++ /dev/null @@ -1,50 +0,0 @@ -/* - * 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.cache.client.internal; - -import static org.junit.Assert.assertNull; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -import org.junit.Test; - -import org.apache.geode.cache.Cache; -import org.apache.geode.internal.cache.tier.MessageType; -import org.apache.geode.internal.cache.tier.sockets.Message; - -public class GetClientPRMetaDataOpJUnitTest { - @Test - public void processResponseWhenCacheClosedShuouldReturnNull() throws Exception { - Cache cache = mock(Cache.class); - ClientMetadataService cms = new ClientMetadataService(cache); - cms = spy(cms); - doReturn(true).when(cache).isClosed(); - - Message msg = mock(Message.class); - GetClientPRMetaDataOp.GetClientPRMetaDataOpImpl op = - new GetClientPRMetaDataOp.GetClientPRMetaDataOpImpl("testRegion", cms); - op = spy(op); - - when(msg.getMessageType()).thenReturn(MessageType.RESPONSE_CLIENT_PR_METADATA); - - assertNull(op.processResponse(msg)); - verify(cms, times(1)).setMetadataStable(eq(true)); - } -} diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/MBeanProxyFactoryTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/MBeanProxyFactoryTest.java index c719fbd..73fc15d 100644 --- a/geode-core/src/test/java/org/apache/geode/management/internal/MBeanProxyFactoryTest.java +++ b/geode-core/src/test/java/org/apache/geode/management/internal/MBeanProxyFactoryTest.java @@ -16,45 +16,29 @@ package org.apache.geode.management.internal; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.powermock.api.mockito.PowerMockito.mockStatic; -import static org.powermock.api.mockito.PowerMockito.when; import java.util.HashSet; import java.util.Map; import java.util.Set; -import org.apache.logging.log4j.Logger; import org.junit.Test; -import org.junit.runner.RunWith; -import org.powermock.api.mockito.PowerMockito; -import org.powermock.core.classloader.annotations.PowerMockIgnore; -import org.powermock.core.classloader.annotations.PrepareForTest; -import org.powermock.modules.junit4.PowerMockRunner; import org.apache.geode.cache.EntryNotFoundException; import org.apache.geode.cache.Region; import org.apache.geode.distributed.DistributedMember; -import org.apache.geode.internal.logging.LogService; -@RunWith(PowerMockRunner.class) -@PrepareForTest(LogService.class) -@PowerMockIgnore("javax.script.*") public class MBeanProxyFactoryTest { @Test public void removeAllProxiesEntryNotFoundLogged() { - mockStatic(LogService.class); - Logger mockLogger = PowerMockito.mock(Logger.class); - when(mockLogger.isDebugEnabled()).thenReturn(true); - when(LogService.getLogger()).thenReturn(mockLogger); - MBeanProxyFactory mBeanProxyFactory = - new MBeanProxyFactory(mock(MBeanJMXAdapter.class), mock(SystemManagementService.class)); + spy(new MBeanProxyFactory(mock(MBeanJMXAdapter.class), + mock(SystemManagementService.class))); Region mockRegion = mock(Region.class); Set entrySet = new HashSet<Map.Entry<String, Object>>(); @@ -68,6 +52,6 @@ public class MBeanProxyFactoryTest { // EntryNotFoundException should just result in a warning as it implies // the proxy has already been removed and the entry has already been destroyed - verify(mockLogger, times(1)).warn(anyString(), any(), any()); + verify(mBeanProxyFactory, times(1)).logProxyAlreadyRemoved(any(), any()); } } diff --git a/gradle/java.gradle b/gradle/java.gradle index c2cd072..874e577 100644 --- a/gradle/java.gradle +++ b/gradle/java.gradle @@ -33,7 +33,7 @@ subprojects { } String javaVersion = System.properties['java.version'] - if (javaVersion.startsWith("1.8.0") && javaVersion.split("_")[1].toInteger() < 121) { + if (javaVersion.startsWith("1.8.0") && javaVersion.split("-")[0].split("_")[1].toInteger() < 121) { throw new GradleException("Java version 1.8.0_121 or later required, but was " + javaVersion) }
