From 4db3e8c28fa12ab38e18ddd9db5955a6d23396ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E7=BF=8A=20SionYang?= Date: Fri, 3 Feb 2023 14:13:08 +0800 Subject: [PATCH] [ISSUE #9816] Add expected final state for redo data. (#9893) * Fix #9816, add expected final state to make redo task execute right finally. * Upgrade version to 2.2.1-SNAPSHOT. --- .../remote/gprc/NamingGrpcClientProxy.java | 4 +- .../gprc/redo/NamingGrpcRedoService.java | 21 ++++- .../gprc/redo/data/BatchInstanceRedoData.java | 21 +++++ .../remote/gprc/redo/data/RedoData.java | 60 +++++++++++---- .../gprc/NamingGrpcClientProxyTest.java | 76 ++++++++++++++++++- .../gprc/redo/NamingGrpcRedoServiceTest.java | 20 +++++ .../gprc/redo/RedoScheduledTaskTest.java | 22 +++--- .../redo/data/BatchInstanceRedoDataTest.java | 56 ++++++++++++++ pom.xml | 2 +- 9 files changed, 251 insertions(+), 31 deletions(-) create mode 100644 client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/redo/data/BatchInstanceRedoDataTest.java diff --git a/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/NamingGrpcClientProxy.java b/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/NamingGrpcClientProxy.java index 8b2536bb6bf..0966657fe6a 100644 --- a/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/NamingGrpcClientProxy.java +++ b/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/NamingGrpcClientProxy.java @@ -153,7 +153,7 @@ private List getRetainInstance(String serviceName, String groupName, L instances)); } String combinedServiceName = NamingUtils.getGroupedName(serviceName, groupName); - InstanceRedoData instanceRedoData = redoService.getRegisteredInstancesBykey(combinedServiceName); + InstanceRedoData instanceRedoData = redoService.getRegisteredInstancesByKey(combinedServiceName); if (!(instanceRedoData instanceof BatchInstanceRedoData)) { throw new NacosException(NacosException.INVALID_PARAM, String.format( "[Batch deRegistration] batch deRegister is not BatchInstanceRedoData type , instances: %s,", @@ -231,7 +231,7 @@ public void doDeregisterService(String serviceName, String groupName, Instance i InstanceRequest request = new InstanceRequest(namespaceId, serviceName, groupName, NamingRemoteConstants.DE_REGISTER_INSTANCE, instance); requestToServer(request, Response.class); - redoService.removeInstanceForRedo(serviceName, groupName); + redoService.instanceDeregistered(serviceName, groupName); } @Override diff --git a/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/redo/NamingGrpcRedoService.java b/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/redo/NamingGrpcRedoService.java index a355999c883..de9af324269 100644 --- a/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/redo/NamingGrpcRedoService.java +++ b/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/redo/NamingGrpcRedoService.java @@ -132,7 +132,7 @@ public void instanceRegistered(String serviceName, String groupName) { synchronized (registeredInstances) { InstanceRedoData redoData = registeredInstances.get(key); if (null != redoData) { - redoData.setRegistered(true); + redoData.registered(); } } } @@ -149,6 +149,23 @@ public void instanceDeregister(String serviceName, String groupName) { InstanceRedoData redoData = registeredInstances.get(key); if (null != redoData) { redoData.setUnregistering(true); + redoData.setExpectedRegistered(false); + } + } + } + + /** + * Instance deregister finished, mark unregistered status. + * + * @param serviceName service name + * @param groupName group name + */ + public void instanceDeregistered(String serviceName, String groupName) { + String key = NamingUtils.getGroupedName(serviceName, groupName); + synchronized (registeredInstances) { + InstanceRedoData redoData = registeredInstances.get(key); + if (null != redoData) { + redoData.unregistered(); } } } @@ -281,7 +298,7 @@ public Set findSubscriberRedoData() { * get Cache service. * @return cache service */ - public InstanceRedoData getRegisteredInstancesBykey(String combinedServiceName) { + public InstanceRedoData getRegisteredInstancesByKey(String combinedServiceName) { return registeredInstances.get(combinedServiceName); } diff --git a/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/redo/data/BatchInstanceRedoData.java b/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/redo/data/BatchInstanceRedoData.java index 3c51a9e18f0..ee627a60c92 100644 --- a/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/redo/data/BatchInstanceRedoData.java +++ b/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/redo/data/BatchInstanceRedoData.java @@ -19,6 +19,7 @@ import com.alibaba.nacos.api.naming.pojo.Instance; import java.util.List; +import java.util.Objects; /** * batch instance redo service. @@ -54,4 +55,24 @@ public static BatchInstanceRedoData build(String serviceName, String groupName, result.setInstances(instances); return result; } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof BatchInstanceRedoData)) { + return false; + } + if (!super.equals(o)) { + return false; + } + BatchInstanceRedoData redoData = (BatchInstanceRedoData) o; + return Objects.equals(instances, redoData.instances); + } + + @Override + public int hashCode() { + return Objects.hash(super.hashCode(), instances); + } } diff --git a/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/redo/data/RedoData.java b/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/redo/data/RedoData.java index e235a41b708..17825e03ed9 100644 --- a/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/redo/data/RedoData.java +++ b/client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/redo/data/RedoData.java @@ -30,6 +30,16 @@ public abstract class RedoData { private final String groupName; + /** + * Expected states for finally. + * + *
    + *
  • {@code true} meas the cached data expect registered to server finally.
    • + *
  • {@code false} means unregistered from server.
    • + *
+ */ + private volatile boolean expectedRegistered; + /** * If {@code true} means cached data has been registered to server successfully. */ @@ -45,6 +55,7 @@ public abstract class RedoData { protected RedoData(String serviceName, String groupName) { this.serviceName = serviceName; this.groupName = groupName; + this.expectedRegistered = true; } public String getServiceName() { @@ -55,18 +66,26 @@ public String getGroupName() { return groupName; } - public boolean isRegistered() { - return registered; + public void setExpectedRegistered(boolean registered) { + this.expectedRegistered = registered; } - public void setRegistered(boolean registered) { - this.registered = registered; + public boolean isExpectedRegistered() { + return expectedRegistered; + } + + public boolean isRegistered() { + return registered; } public boolean isUnregistering() { return unregistering; } + public void setRegistered(boolean registered) { + this.registered = registered; + } + public void setUnregistering(boolean unregistering) { this.unregistering = unregistering; } @@ -79,8 +98,22 @@ public void set(T data) { this.data = data; } + public void registered() { + this.registered = true; + this.unregistering = false; + } + + public void unregistered() { + this.registered = false; + this.unregistering = true; + } + + public boolean isNeedRedo() { + return !RedoType.NONE.equals(getRedoType()); + } + /** - * Get redo type for current redo data. + * Get redo type for current redo data without expected state. * *
    *
  • {@code registered=true} & {@code unregistering=false} means data has registered, so redo should not do anything.
    • @@ -93,20 +126,16 @@ public void set(T data) { */ public RedoType getRedoType() { if (isRegistered() && !isUnregistering()) { - return RedoType.NONE; + return expectedRegistered ? RedoType.NONE : RedoType.UNREGISTER; } else if (isRegistered() && isUnregistering()) { return RedoType.UNREGISTER; } else if (!isRegistered() && !isUnregistering()) { return RedoType.REGISTER; } else { - return RedoType.REMOVE; + return expectedRegistered ? RedoType.REGISTER : RedoType.REMOVE; } } - public boolean isNeedRedo() { - return !RedoType.NONE.equals(getRedoType()); - } - public enum RedoType { /** @@ -129,7 +158,7 @@ public enum RedoType { */ REMOVE; } - + @Override public boolean equals(Object o) { if (this == o) { @@ -139,10 +168,11 @@ public boolean equals(Object o) { return false; } RedoData redoData = (RedoData) o; - return registered == redoData.registered && unregistering == redoData.unregistering - && serviceName.equals(redoData.serviceName) && groupName.equals(redoData.groupName) && data.equals(redoData.data); + return registered == redoData.registered && unregistering == redoData.unregistering && serviceName + .equals(redoData.serviceName) && groupName.equals(redoData.groupName) && Objects + .equals(data, redoData.data); } - + @Override public int hashCode() { return Objects.hash(serviceName, groupName, registered, unregistering, data); diff --git a/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/NamingGrpcClientProxyTest.java b/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/NamingGrpcClientProxyTest.java index f78d6b91bd5..ec9c22dbfb3 100644 --- a/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/NamingGrpcClientProxyTest.java +++ b/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/NamingGrpcClientProxyTest.java @@ -66,6 +66,7 @@ import java.lang.reflect.Field; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -74,6 +75,8 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.Mockito.times; @@ -223,6 +226,66 @@ public void testBatchRegisterService() throws NacosException { })); } + @Test(expected = NacosException.class) + public void testBatchDeregisterServiceWithEmptyInstances() throws NacosException { + client.batchDeregisterService(SERVICE_NAME, GROUP_NAME, Collections.EMPTY_LIST); + } + + @Test(expected = NacosException.class) + public void testBatchDeregisterServiceWithoutCacheData() throws NacosException { + List instanceList = new ArrayList<>(); + instance.setHealthy(true); + instanceList.add(instance); + client.batchDeregisterService(SERVICE_NAME, GROUP_NAME, instanceList); + } + + @Test(expected = NacosException.class) + public void testBatchDeregisterServiceNotBatchData() throws NacosException { + client.registerService(SERVICE_NAME, GROUP_NAME, instance); + List instanceList = new ArrayList<>(); + instance.setHealthy(true); + instanceList.add(instance); + client.batchDeregisterService(SERVICE_NAME, GROUP_NAME, instanceList); + } + + @Test(expected = NacosException.class) + public void testBatchDeregisterServiceWithEmptyBatchData() throws NacosException { + try { + client.batchRegisterService(SERVICE_NAME, GROUP_NAME, Collections.EMPTY_LIST); + } catch (Exception ignored) { + } + List instanceList = new ArrayList<>(); + instance.setHealthy(true); + instanceList.add(instance); + client.batchDeregisterService(SERVICE_NAME, GROUP_NAME, instanceList); + } + + @Test + public void testBatchDeregisterService() throws NacosException { + try { + List instanceList = new ArrayList<>(); + instance.setHealthy(true); + instanceList.add(instance); + instanceList.add(new Instance()); + client.batchRegisterService(SERVICE_NAME, GROUP_NAME, instanceList); + } catch (Exception ignored) { + } + response = new BatchInstanceResponse(); + when(this.rpcClient.request(any())).thenReturn(response); + List instanceList = new ArrayList<>(); + instance.setHealthy(true); + instanceList.add(instance); + client.batchDeregisterService(SERVICE_NAME, GROUP_NAME, instanceList); + verify(this.rpcClient, times(1)).request(argThat(request -> { + if (request instanceof BatchInstanceRequest) { + BatchInstanceRequest request1 = (BatchInstanceRequest) request; + request1.setRequestId("1"); + return request1.getInstances().size() == 1 && request1.getType().equals(NamingRemoteConstants.BATCH_REGISTER_INSTANCE); + } + return false; + })); + } + @Test public void testUpdateInstance() throws Exception { //TODO thrown.expect(UnsupportedOperationException.class); @@ -256,7 +319,7 @@ public void testCreateService() throws Exception { @Test public void testDeleteService() throws Exception { //TODO thrown.expect(UnsupportedOperationException.class); - Assert.assertFalse(client.deleteService(SERVICE_NAME, GROUP_NAME)); + assertFalse(client.deleteService(SERVICE_NAME, GROUP_NAME)); } @Test @@ -310,6 +373,17 @@ public void testUnsubscribe() throws Exception { })); } + @Test + public void testIsSubscribed() throws NacosException { + SubscribeServiceResponse res = new SubscribeServiceResponse(); + ServiceInfo info = new ServiceInfo(GROUP_NAME + "@@" + SERVICE_NAME + "@@" + CLUSTERS); + res.setServiceInfo(info); + when(this.rpcClient.request(any())).thenReturn(res); + assertFalse(client.isSubscribed(SERVICE_NAME, GROUP_NAME, CLUSTERS)); + client.subscribe(SERVICE_NAME, GROUP_NAME, CLUSTERS); + assertTrue(client.isSubscribed(SERVICE_NAME, GROUP_NAME, CLUSTERS)); + } + @Test public void testServerHealthy() { when(this.rpcClient.isRunning()).thenReturn(true); diff --git a/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/redo/NamingGrpcRedoServiceTest.java b/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/redo/NamingGrpcRedoServiceTest.java index 150ff5d87c2..3d2030f9986 100644 --- a/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/redo/NamingGrpcRedoServiceTest.java +++ b/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/redo/NamingGrpcRedoServiceTest.java @@ -101,6 +101,7 @@ public void testCacheInstanceForRedo() { assertEquals(instance, actual.get()); assertFalse(actual.isRegistered()); assertFalse(actual.isUnregistering()); + assertTrue(actual.isExpectedRegistered()); } @Test @@ -136,6 +137,17 @@ public void testInstanceDeregister() { redoService.instanceDeregister(SERVICE, GROUP); InstanceRedoData actual = registeredInstances.entrySet().iterator().next().getValue(); assertTrue(actual.isUnregistering()); + assertFalse(actual.isExpectedRegistered()); + } + + @Test + public void testInstanceDeregistered() { + ConcurrentMap registeredInstances = getInstanceRedoDataMap(); + redoService.cacheInstanceForRedo(SERVICE, GROUP, new Instance()); + redoService.instanceDeregistered(SERVICE, GROUP); + InstanceRedoData actual = registeredInstances.entrySet().iterator().next().getValue(); + assertFalse(actual.isRegistered()); + assertTrue(actual.isUnregistering()); } @Test @@ -195,6 +207,14 @@ public void testSubscriberDeregister() { assertTrue(actual.isUnregistering()); } + @Test + public void testIsSubscriberRegistered() { + assertFalse(redoService.isSubscriberRegistered(SERVICE, GROUP, CLUSTER)); + redoService.cacheSubscriberForRedo(SERVICE, GROUP, CLUSTER); + redoService.subscriberRegistered(SERVICE, GROUP, CLUSTER); + assertTrue(redoService.isSubscriberRegistered(SERVICE, GROUP, CLUSTER)); + } + @Test public void testRemoveSubscriberForRedo() { ConcurrentMap subscribes = getSubscriberRedoDataMap(); diff --git a/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/redo/RedoScheduledTaskTest.java b/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/redo/RedoScheduledTaskTest.java index 51f5e1a241a..7d99862335c 100644 --- a/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/redo/RedoScheduledTaskTest.java +++ b/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/redo/RedoScheduledTaskTest.java @@ -62,7 +62,7 @@ public void setUp() throws Exception { @Test public void testRunRedoRegisterInstance() throws NacosException { - Set mockData = generateMockInstanceData(false, false); + Set mockData = generateMockInstanceData(false, false, true); when(redoService.findInstanceRedoData()).thenReturn(mockData); redoTask.run(); verify(clientProxy).doRegisterService(SERVICE, GROUP, INSTANCE); @@ -70,7 +70,7 @@ public void testRunRedoRegisterInstance() throws NacosException { @Test public void testRunRedoDeregisterInstance() throws NacosException { - Set mockData = generateMockInstanceData(true, true); + Set mockData = generateMockInstanceData(true, true, false); when(redoService.findInstanceRedoData()).thenReturn(mockData); redoTask.run(); verify(clientProxy).doDeregisterService(SERVICE, GROUP, INSTANCE); @@ -78,7 +78,7 @@ public void testRunRedoDeregisterInstance() throws NacosException { @Test public void testRunRedoRemoveInstanceRedoData() throws NacosException { - Set mockData = generateMockInstanceData(false, true); + Set mockData = generateMockInstanceData(false, true, false); when(redoService.findInstanceRedoData()).thenReturn(mockData); redoTask.run(); verify(redoService).removeInstanceForRedo(SERVICE, GROUP); @@ -87,16 +87,17 @@ public void testRunRedoRemoveInstanceRedoData() throws NacosException { @Test public void testRunRedoRegisterInstanceWithClientDisabled() throws NacosException { when(clientProxy.isEnable()).thenReturn(false); - Set mockData = generateMockInstanceData(false, false); + Set mockData = generateMockInstanceData(false, false, true); when(redoService.findInstanceRedoData()).thenReturn(mockData); redoTask.run(); verify(clientProxy, never()).doRegisterService(SERVICE, GROUP, INSTANCE); } - private Set generateMockInstanceData(boolean registered, boolean unregistering) { + private Set generateMockInstanceData(boolean registered, boolean unregistering, boolean expectedRegistered) { InstanceRedoData redoData = InstanceRedoData.build(SERVICE, GROUP, INSTANCE); redoData.setRegistered(registered); redoData.setUnregistering(unregistering); + redoData.setExpectedRegistered(expectedRegistered); Set result = new HashSet<>(); result.add(redoData); return result; @@ -104,7 +105,7 @@ private Set generateMockInstanceData(boolean registered, boole @Test public void testRunRedoRegisterSubscriber() throws NacosException { - Set mockData = generateMockSubscriberData(false, false); + Set mockData = generateMockSubscriberData(false, false, true); when(redoService.findSubscriberRedoData()).thenReturn(mockData); redoTask.run(); verify(clientProxy).doSubscribe(SERVICE, GROUP, CLUSTER); @@ -112,7 +113,7 @@ public void testRunRedoRegisterSubscriber() throws NacosException { @Test public void testRunRedoDeregisterSubscriber() throws NacosException { - Set mockData = generateMockSubscriberData(true, true); + Set mockData = generateMockSubscriberData(true, true, false); when(redoService.findSubscriberRedoData()).thenReturn(mockData); redoTask.run(); verify(clientProxy).doUnsubscribe(SERVICE, GROUP, CLUSTER); @@ -120,7 +121,7 @@ public void testRunRedoDeregisterSubscriber() throws NacosException { @Test public void testRunRedoRemoveSubscriberRedoData() throws NacosException { - Set mockData = generateMockSubscriberData(false, true); + Set mockData = generateMockSubscriberData(false, true, false); when(redoService.findSubscriberRedoData()).thenReturn(mockData); redoTask.run(); verify(redoService).removeSubscriberForRedo(SERVICE, GROUP, CLUSTER); @@ -129,16 +130,17 @@ public void testRunRedoRemoveSubscriberRedoData() throws NacosException { @Test public void testRunRedoRegisterSubscriberWithClientDisabled() throws NacosException { when(clientProxy.isEnable()).thenReturn(false); - Set mockData = generateMockSubscriberData(false, false); + Set mockData = generateMockSubscriberData(false, false, true); when(redoService.findSubscriberRedoData()).thenReturn(mockData); redoTask.run(); verify(clientProxy, never()).doSubscribe(SERVICE, GROUP, CLUSTER); } - private Set generateMockSubscriberData(boolean registered, boolean unregistering) { + private Set generateMockSubscriberData(boolean registered, boolean unregistering, boolean expectedRegistered) { SubscriberRedoData redoData = SubscriberRedoData.build(SERVICE, GROUP, CLUSTER); redoData.setRegistered(registered); redoData.setUnregistering(unregistering); + redoData.setExpectedRegistered(expectedRegistered); Set result = new HashSet<>(); result.add(redoData); return result; diff --git a/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/redo/data/BatchInstanceRedoDataTest.java b/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/redo/data/BatchInstanceRedoDataTest.java new file mode 100644 index 00000000000..07f8d5b23ad --- /dev/null +++ b/client/src/test/java/com/alibaba/nacos/client/naming/remote/gprc/redo/data/BatchInstanceRedoDataTest.java @@ -0,0 +1,56 @@ +/* + * Copyright 1999-2021 Alibaba Group Holding Ltd. + * + * Licensed 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 com.alibaba.nacos.client.naming.remote.gprc.redo.data; + +import com.alibaba.nacos.api.naming.pojo.Instance; +import org.junit.Test; + +import java.util.Collections; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; + +public class BatchInstanceRedoDataTest { + + @Test + @SuppressWarnings("all") + public void testEquals() { + BatchInstanceRedoData redoData1 = new BatchInstanceRedoData("a", "b"); + redoData1.setInstances(Collections.singletonList(new Instance())); + BatchInstanceRedoData redoData2 = new BatchInstanceRedoData("a", "b"); + redoData2.setInstances(Collections.singletonList(new Instance())); + assertTrue(redoData1.equals(redoData1)); + assertTrue(redoData1.equals(redoData2)); + redoData2.getInstances().get(0).setIp("1.1.1.1"); + assertFalse(redoData1.equals(null)); + assertFalse(redoData1.equals(redoData2)); + assertFalse(redoData1.equals(redoData2)); + } + + @Test + public void testHashCode() { + BatchInstanceRedoData redoData1 = new BatchInstanceRedoData("a", "b"); + redoData1.setInstances(Collections.singletonList(new Instance())); + BatchInstanceRedoData redoData2 = new BatchInstanceRedoData("a", "b"); + redoData2.setInstances(Collections.singletonList(new Instance())); + assertEquals(redoData1.hashCode(), redoData2.hashCode()); + redoData2.getInstances().get(0).setIp("1.1.1.1"); + assertNotEquals(redoData1.hashCode(), redoData2.hashCode()); + } +} \ No newline at end of file diff --git a/pom.xml b/pom.xml index 46f05658436..aebe88e32d1 100644 --- a/pom.xml +++ b/pom.xml @@ -88,7 +88,7 @@ - 2.2.1-RC + 2.2.1-SNAPSHOT UTF-8 UTF-8