Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ISSUE #11890] Check instance existence before expiring its metadata #11932

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
+107 −6
Open

[ISSUE #11890] Check instance existence before expiring its metadata #11932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8e90445
Check instance existence before expiring its metadata
nkorange Apr 8, 2024
7e61d03
Add unit test for expired metadata cleaner
nkorange Apr 8, 2024
279853a
Fix check style
nkorange Apr 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 39 additions & 4 deletions naming/src/main/java/com/alibaba/nacos/naming/core/v2/cleaner/ExpiredMetadataCleaner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,20 @@

package com.alibaba.nacos.naming.core.v2.cleaner;

import com.alibaba.nacos.api.naming.pojo.Instance;
import com.alibaba.nacos.api.naming.pojo.ServiceInfo;
import com.alibaba.nacos.naming.core.v2.index.ServiceStorage;
import com.alibaba.nacos.naming.core.v2.metadata.ExpiredMetadataInfo;
import com.alibaba.nacos.naming.core.v2.metadata.NamingMetadataManager;
import com.alibaba.nacos.naming.core.v2.metadata.NamingMetadataOperateService;
import com.alibaba.nacos.naming.core.v2.pojo.InstancePublishInfo;
import com.alibaba.nacos.naming.core.v2.pojo.Service;
import com.alibaba.nacos.naming.misc.GlobalConfig;
import com.alibaba.nacos.naming.misc.GlobalExecutor;
import com.alibaba.nacos.naming.misc.Loggers;
import org.springframework.stereotype.Component;

import java.util.Iterator;
import java.util.concurrent.TimeUnit;

/**
Expand All @@ -42,10 +48,13 @@ public class ExpiredMetadataCleaner extends AbstractNamingCleaner {

private final NamingMetadataOperateService metadataOperateService;

private final ServiceStorage serviceStorage;

public ExpiredMetadataCleaner(NamingMetadataManager metadataManager,
NamingMetadataOperateService metadataOperateService) {
NamingMetadataOperateService metadataOperateService, ServiceStorage serviceStorage) {
this.metadataManager = metadataManager;
this.metadataOperateService = metadataOperateService;
this.serviceStorage = serviceStorage;
GlobalExecutor.scheduleExpiredClientCleaner(this, INITIAL_DELAY, GlobalConfig.getExpiredMetadataCleanInterval(),
TimeUnit.MILLISECONDS);
}
Expand All @@ -58,23 +67,49 @@ public String getType() {
@Override
public void doClean() {
long currentTime = System.currentTimeMillis();
for (ExpiredMetadataInfo each : metadataManager.getExpiredMetadataInfos()) {
Iterator<ExpiredMetadataInfo> it = metadataManager.getExpiredMetadataInfos().iterator();
while (it.hasNext()) {
ExpiredMetadataInfo each = it.next();
if (currentTime - each.getCreateTime() > GlobalConfig.getExpiredMetadataExpiredTime()) {
removeExpiredMetadata(each);
if (!removeExpiredMetadata(each)) {
it.remove();
}
}
}
}

private void removeExpiredMetadata(ExpiredMetadataInfo expiredInfo) {
private boolean removeExpiredMetadata(ExpiredMetadataInfo expiredInfo) {
Loggers.SRV_LOG.info("Remove expired metadata {}", expiredInfo);
if (null == expiredInfo.getMetadataId()) {
if (metadataManager.containServiceMetadata(expiredInfo.getService())) {
metadataOperateService.deleteServiceMetadata(expiredInfo.getService());
}
} else {
Instance instance = queryInstance(expiredInfo);
if (instance != null) {
Loggers.SRV_LOG.warn("Instance exists, abort removing metadata {}", expiredInfo);
return false;
}
if (metadataManager.containInstanceMetadata(expiredInfo.getService(), expiredInfo.getMetadataId())) {
metadataOperateService.deleteInstanceMetadata(expiredInfo.getService(), expiredInfo.getMetadataId());
}
}
return true;
}

private Instance queryInstance(ExpiredMetadataInfo expiredInfo) {
Instance instance = null;
String cluster = InstancePublishInfo.getClusterFromMetadataId(expiredInfo.getMetadataId());
String ip = InstancePublishInfo.getIpFromMetadataId(expiredInfo.getMetadataId());
int port = InstancePublishInfo.getPortFromMetadataId(expiredInfo.getMetadataId());
Service service = expiredInfo.getService();
ServiceInfo serviceInfo = serviceStorage.getPushData(service);
for (Instance each : serviceInfo.getHosts()) {
if (cluster.equals(each.getClusterName()) && ip.equals(each.getIp()) && port == each.getPort()) {
instance = each;
break;
}
}
return instance;
}
}
13 changes: 13 additions & 0 deletions naming/src/main/java/com/alibaba/nacos/naming/core/v2/pojo/InstancePublishInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.alibaba.nacos.naming.core.v2.pojo;

import com.alibaba.nacos.common.utils.InternetAddressUtil;
import com.alibaba.nacos.common.utils.NumberUtils;

import java.io.Serializable;
import java.util.HashMap;
Expand Down Expand Up @@ -126,4 +127,16 @@ public String toString() {
public static String genMetadataId(String ip, int port, String cluster) {
return ip + InternetAddressUtil.IP_PORT_SPLITER + port + InternetAddressUtil.IP_PORT_SPLITER + cluster;
}

public static String getClusterFromMetadataId(String metadataId) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这3个方法建议放置到 Cleaner中，作为私有方法

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这3个方法建议放置到 Cleaner中，作为私有方法

这几个方法只和metadataId的结构有关系，似乎应该放在生成metadataId的地方，并作为公共方法比较好？

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

但是只有这里有需要使用， 我觉得还是放cleaner里作为私有方法， 如果以后真需要再改。

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

但是只有这里有需要使用， 我觉得还是放cleaner里作为私有方法， 如果以后真需要再改。

问题是如果后面其他地方有需要用，比较难发现某个cleaner已经定义了一个私有方法？

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个问题你放在这里也可能是一样的， 这个必须通过review才可以，如果你一定要单独处理，要不就放到一个工具类里。

我的意见是暂时没有必要， 如果后续有需要，再做抽象即可， 不要为了不存在的需求做所谓的优化。

Copy link
Collaborator Author

@nkorange nkorange Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为啥生成metadataID的方法是在这个类，解析metadataID又要放在别的类呢？这个metadataID本来就算是InstancePublishInfo的一个属性。

当我们有解析metadataID的需求的时候，我觉得很自然会去找到生成metadataID的类去看metadataID的结构，所以这3个方法放在这个类也是很合理的。

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

因为只有你这个地方要解析， 没有别的地方要解析， 说白了要解析是你这个地方的特殊逻辑，当然要以私有方法方式支持。

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

要不然你就把所有metadataID的相关操作，都封装到一个工具类里面。

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

说白了， 现在这个PR只是为了修复这个问题的临时手段， 在临时手段中允许暂时的去解析metadataID，获取对应的数据，先修复问题，不考虑性能和处理手段，长期必须要合理处理这个机制，可能根本就不用去解析metadataId。

所以现在就应该把metadataID的解析工作，收敛到这个特殊的修复逻辑中， 不能发散成为一个通用手段。除非以后确定metadataID需要被拆分解析，如果到了那一步， 我反而倾向吧metadataID变为一个Object，而不是简单的字符串。

return metadataId.split(InternetAddressUtil.IP_PORT_SPLITER)[2];
}

public static String getIpFromMetadataId(String metadataId) {
return metadataId.split(InternetAddressUtil.IP_PORT_SPLITER)[0];
}

public static int getPortFromMetadataId(String metadataId) {
return NumberUtils.toInt(metadataId.split(InternetAddressUtil.IP_PORT_SPLITER)[1]);
}
}
57 changes: 55 additions & 2 deletions ...ng/src/test/java/com/alibaba/nacos/naming/core/v2/cleaner/ExpiredMetadataCleanerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,31 @@

package com.alibaba.nacos.naming.core.v2.cleaner;

import com.alibaba.nacos.api.common.Constants;
import com.alibaba.nacos.api.naming.pojo.Instance;
import com.alibaba.nacos.api.naming.pojo.ServiceInfo;
import com.alibaba.nacos.common.utils.ConcurrentHashSet;
import com.alibaba.nacos.naming.core.v2.index.ServiceStorage;
import com.alibaba.nacos.naming.core.v2.metadata.ExpiredMetadataInfo;
import com.alibaba.nacos.naming.core.v2.metadata.NamingMetadataManager;
import com.alibaba.nacos.naming.core.v2.metadata.NamingMetadataOperateService;
import com.alibaba.nacos.naming.core.v2.pojo.InstancePublishInfo;
import com.alibaba.nacos.naming.core.v2.pojo.Service;
import com.alibaba.nacos.sys.env.EnvUtil;
import junit.framework.TestCase;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;
import org.springframework.mock.env.MockEnvironment;

import java.util.ArrayList;
import java.util.List;
import java.util.Set;

import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

Expand All @@ -49,12 +59,18 @@ public class ExpiredMetadataCleanerTest extends TestCase {

@Mock
private ExpiredMetadataInfo expiredMetadataInfoMock;

@Mock
private ExpiredMetadataInfo existingMetadataInfoMock;

@Mock
private ServiceStorage serviceStorage;

@Before
public void setUp() throws Exception {
super.setUp();
EnvUtil.setEnvironment(new MockEnvironment());
expiredMetadataCleaner = new ExpiredMetadataCleaner(metadataManagerMock, metadataOperateServiceMock);
expiredMetadataCleaner = new ExpiredMetadataCleaner(metadataManagerMock, metadataOperateServiceMock, serviceStorage);

set.add(expiredMetadataInfoMock);

Expand All @@ -69,4 +85,41 @@ public void testDoClean() {
verify(metadataManagerMock).getExpiredMetadataInfos();
verify(metadataOperateServiceMock).deleteServiceMetadata(expiredMetadataInfoMock.getService());
}
}

@Test
public void testCleanExistingInstanceMetadata() {
String serviceName = "test.1";
String ip = "1.1.1.1";
int port = 7001;
ServiceInfo serviceInfo = new ServiceInfo();
serviceInfo.setName(serviceName);
Instance instance = new Instance();
instance.setIp(ip);
instance.setPort(port);
instance.setClusterName(Constants.DEFAULT_CLUSTER_NAME);
List<Instance> instances = new ArrayList<>();
instances.add(instance);
serviceInfo.setHosts(instances);

Service service = Service.newService(Constants.DEFAULT_NAMESPACE_ID, Constants.DEFAULT_GROUP, serviceName);
when(existingMetadataInfoMock.getService()).thenReturn(service);
when(existingMetadataInfoMock.getCreateTime()).thenReturn(0L);
when(existingMetadataInfoMock.getMetadataId())
.thenReturn(InstancePublishInfo.genMetadataId(ip, port, Constants.DEFAULT_CLUSTER_NAME));

set.add(existingMetadataInfoMock);

Assert.assertEquals(2, set.size());

when(serviceStorage.getPushData(service)).thenReturn(serviceInfo);

expiredMetadataCleaner.doClean();

verify(metadataManagerMock).getExpiredMetadataInfos();
verify(metadataOperateServiceMock).deleteServiceMetadata(expiredMetadataInfoMock.getService());
verify(metadataOperateServiceMock, never()).deleteServiceMetadata(service);
verify(serviceStorage).getPushData(service);

Assert.assertEquals(1, set.size());
}
}
Loading