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

Jump to bottom

Optimize checkLocalConfig logic #11869

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Optimize checkLocalConfig logic #11869

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bf32067
优化checkLocalConfig逻辑
PleaseGiveMeTheCoke Mar 25, 2024
46b5cc9
修改不规范的等号和分号
PleaseGiveMeTheCoke Mar 29, 2024
90a9eaa
1. 多次校验文件是否存在
PleaseGiveMeTheCoke Apr 2, 2024
8c109d8
format
PleaseGiveMeTheCoke Apr 2, 2024
dabda8a
Merge branch 'alibaba:develop' into develop
PleaseGiveMeTheCoke Apr 29, 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
42 changes: 18 additions & 24 deletions client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,42 +866,36 @@ public void checkLocalConfig(CacheData cacheData) {
final String group = cacheData.group;
final String tenant = cacheData.tenant;
final String envName = cacheData.envName;

Copy link
Collaborator

Choose a reason for hiding this comment

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

麻烦使用nacos-code-style进行reformat， 不需要删除缩紧

Copy link
Author

Choose a reason for hiding this comment

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

done

// Check if a failover file exists for the specified dataId, group, and tenant.
File file = LocalConfigInfoProcessor.getFailoverFile(envName, dataId, group, tenant);

// If not using local config info and a failover file exists, load and use it.
if (!cacheData.isUseLocalConfigInfo() && file.exists()) {

// not using local config info, but a failover file exists
boolean failOverFileCreated = !cacheData.isUseLocalConfigInfo() && file.exists();

// using local config info, but there is a change in local configuration
boolean failOverFileChanged= cacheData.isUseLocalConfigInfo() && file.exists() && cacheData.getLocalConfigInfoVersion() != file.lastModified();
Copy link

Choose a reason for hiding this comment

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

= 号空格不一致

看上去是个负优化，原本比较清晰的逻辑，现在要看懂得不断的上下去找逻辑。当一个变量需要大段的注释去解释它的作用的时候要么是变量名字起的不好，要么是就不该用变量

Copy link
Author

Choose a reason for hiding this comment

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

感谢提出建议，我对 = 和 ; 的问题都做了相应修改

可能我作为Nacos的初学者经验不足，我一开始看到这段逻辑的时候觉得很迷惑，不太知道是否使用本地配置、文件是否存在、版本号是否一致等条件组合在一起的含义，后来仔细阅读才得以明确。于是我按照我阅读代码得到的理解把if括号中的内容提取成变量，相应的注释也提取成变量的注释，消除了冗余的逻辑，并且用一个比较直观的名字来命名这个变量，其实这个注释完全可以不要的

Copy link

@dongm2ez dongm2ez Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

可以思考一下，failOverFileCreated 和 failOverFileChanged 对于使用的地方都是一样的处理逻辑，而 failOverFileDeleted 又是另一段逻辑，那是不是可以更抽象一下。


// using local config info, but the failover file is deleted
boolean failOverFileDeleted = cacheData.isUseLocalConfigInfo() && !file.exists();

if (failOverFileCreated || failOverFileChanged) {
// load and use the file content
String content = LocalConfigInfoProcessor.getFailover(envName, dataId, group, tenant);
final String md5 = MD5Utils.md5Hex(content, Constants.ENCODE);
cacheData.setUseLocalConfigInfo(true);
cacheData.setLocalConfigInfoVersion(file.lastModified());
cacheData.setContent(content);
LOGGER.warn(
"[{}] [failover-change] failover file created. dataId={}, group={}, tenant={}, md5={}, content={}",
envName, dataId, group, tenant, md5, ContentUtils.truncateContent(content));
return;
"[{}] [failover-change] failover file {}. dataId={}, group={}, tenant={}, md5={}, content={}",
failOverFileCreated ? "created" : "changed", envName, dataId, group, tenant, md5, ContentUtils.truncateContent(content));;
Copy link

Choose a reason for hiding this comment

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

多余的分号

}
// If use local config info, but the failover file is deleted, switch back to server config.
if (cacheData.isUseLocalConfigInfo() && !file.exists()) {

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
Author

Choose a reason for hiding this comment

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

done

if (failOverFileDeleted) {
// switch back to server config.
cacheData.setUseLocalConfigInfo(false);
LOGGER.warn("[{}] [failover-change] failover file deleted. dataId={}, group={}, tenant={}", envName,
dataId, group, tenant);
return;
}

// When the failover file content changes, indicating a change in local configuration.
if (cacheData.isUseLocalConfigInfo() && file.exists()
&& cacheData.getLocalConfigInfoVersion() != file.lastModified()) {
String content = LocalConfigInfoProcessor.getFailover(envName, dataId, group, tenant);
final String md5 = MD5Utils.md5Hex(content, Constants.ENCODE);
cacheData.setUseLocalConfigInfo(true);
cacheData.setLocalConfigInfoVersion(file.lastModified());
cacheData.setContent(content);
LOGGER.warn(
"[{}] [failover-change] failover file changed. dataId={}, group={}, tenant={}, md5={}, content={}",
envName, dataId, group, tenant, md5, ContentUtils.truncateContent(content));
}
}

Expand Down