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.

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

Optimize checkLocalConfig logic #11869

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

Conversation

PleaseGiveMeTheCoke
Copy link

Please do not create a Pull Request without creating an issue first.

What is the purpose of the change

issue:#11866

Brief changelog

XX

Verifying this change

XXXX

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
  • Run mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=true to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.

@CLAassistant
Copy link

CLAassistant commented Mar 25, 2024

CLA assistant check
All committers have signed the CLA.

@stone-98
Copy link
Contributor

LGTM

@KomachiSion
Copy link
Collaborator

Please sign CLA first.

@PleaseGiveMeTheCoke
Copy link
Author

Please sign CLA first.

done

@dongm2ez
Copy link

The PR title may need to be updated to English.

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();

Choose a reason for hiding this comment

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

= 号空格不一致

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

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

Choose a reason for hiding this comment

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

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

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));;

Choose a reason for hiding this comment

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

多余的分号

@PleaseGiveMeTheCoke PleaseGiveMeTheCoke changed the title 优化checkLocalConfig逻辑 Optimize checkLocalConfig logic Mar 29, 2024
Copy link
Collaborator

@KomachiSion KomachiSion left a comment

Choose a reason for hiding this comment

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

我重新读了一下逻辑, 这个重构后的逻辑颠覆了之前的设计理念。

之前之所以不使用变量,是为了在获取failover后, 再次进行文件存在的校验,换句话说进行了至少2次的文件存在校验。

而重构后的逻辑,只进行了一次校验,同时时间上也不太正确。

原: 是否存在 -》 存在读取 -〉 再次校验-》存在则使用,不存在则不使用。
新: 是否存在 -〉 是否删除 -> 存在读取 -> 删除则不使用。

这可能导致某些极端情况下和之前的客户端会有不同的行为, 我的建议是把校验逻辑和通用逻辑抽象成方法, 而不是只用变量存储,并且不建议修改顺序。

shiyiyue1102
shiyiyue1102 previously approved these changes Apr 1, 2024
@shiyiyue1102
Copy link
Collaborator

fix checkstyle pls.

@PleaseGiveMeTheCoke
Copy link
Author

我重新读了一下逻辑, 这个重构后的逻辑颠覆了之前的设计理念。

之前之所以不使用变量,是为了在获取failover后, 再次进行文件存在的校验,换句话说进行了至少2次的文件存在校验。

而重构后的逻辑,只进行了一次校验,同时时间上也不太正确。

原: 是否存在 -》 存在读取 -〉 再次校验-》存在则使用,不存在则不使用。 新: 是否存在 -〉 是否删除 -> 存在读取 -> 删除则不使用。

这可能导致某些极端情况下和之前的客户端会有不同的行为, 我的建议是把校验逻辑和通用逻辑抽象成方法, 而不是只用变量存储,并且不建议修改顺序。

好的,考虑的好细致,感谢提醒

2. 恢复之前的校验顺序
3. 抽象通用逻辑
@@ -866,44 +866,54 @@ 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


// 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

@PleaseGiveMeTheCoke
Copy link
Author

@KomachiSion 请问这个pr合并之前还有什么我需要改动的吗

@KomachiSion
Copy link
Collaborator

CICD通过的话就可以合并了

@PleaseGiveMeTheCoke
Copy link
Author

CICD通过的话就可以合并了

现在应该通过了?

@PleaseGiveMeTheCoke
Copy link
Author

PleaseGiveMeTheCoke commented May 31, 2024

CICD通过的话就可以合并了

hi,麻烦帮忙合并一下?
@KomachiSion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants