[ISSUE #4344] cacheMap property optimization #4353

XiaoWeiKIN · 2020-11-27T08:05:18Z

#4344
The cacheMap attribute in ClientWorker now uese an instance lock, which can be reduced by putIfAbsent from ConcurrentHashMap.

CLAassistant · 2020-11-27T08:05:23Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ christophe-liwei
❌ wangxiaowei14227

wangxiaowei14227 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

KomachiSion

You need use nacos code style to reformat your change files first.

Please see style/codeStyle.md in source code

XiaoWeiKIN · 2020-11-27T09:11:12Z

为什么我没有修改其他的源代码，但是checkstyle却无法通过？

…

------------------ 原始邮件 ------------------ 发件人: "杨翊 SionYang"<notifications@github.com>; 发送时间: 2020年11月27日(星期五) 下午5:06 收件人: "alibaba/nacos"<nacos@noreply.github.com>; 抄送: "炮打资产阶级司令部"<2484713618@qq.com>; "Author"<author@noreply.github.com>; 主题: Re: [alibaba/nacos] issues #4344 (#4353) @KomachiSion requested changes on this pull request. You need use nacos code style to reformat your change files first. Please see style/codeStyle.md in source code — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

chuntaojun · 2020-11-27T09:24:12Z

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java

        }
-
+        // reset so that server not hang this check
+        lastCacheData.setInitializing(true);


if lastCacheData == null

you can't set CacheData.Initializing =true

lastCacheData not null

**CacheData cacheData = new CacheData(configFilterChainManager, agent.getName(), dataId, group, tenant);** if (lastCacheData == null) { //fix issue # 1317 if (enableRemoteSyncConfig) { String[] ct = getServerConfig(dataId, group, tenant, 3000L); cacheData.setContent(ct[0]); } **lastCacheData = cacheData;** } ``

There are some different with old logic.
In old logic, it seems that only not new cacheData need to set Initializing.
But in your new logic, all cacheData what ever old or new will set Initializing. It might cause bugs.

CacheData cacheFromMap = getCache(dataId, group); // multiple listeners on the same dataid+group and race condition,so double check again //other listener thread beat me to set to cacheMap if (null != cacheFromMap) { // the cache data from cache, means it is old cache cache = cacheFromMap; // set cache as old cache //reset so that server not hang this check cache.setInitializing(true); // set old cache Initializing } else { int taskId = cacheMap.get().size() / (int) ParamUtil.getPerTaskConfigSize(); cache.setTaskId(taskId); }

CacheData cacheData = new CacheData(configFilterChainManager, agent.getName(), dataId, group); CacheData lastCacheData = cacheMap.putIfAbsent(key, cacheData); if (lastCacheData == null) { // old cache non-exist ... lastCacheData = cacheData; // set old cache as new cache ... } lastCacheData.setInitializing(true); // set new cache Initializing,

chuntaojun · 2020-11-27T09:28:02Z

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java

        }
+
+        // reset so that server not hang this check
+        lastCacheData.setInitializing(true);


Same as above

KomachiSion · 2020-11-30T02:05:25Z

为什么我没有修改其他的源代码，但是checkstyle却无法通过？
…
------------------ 原始邮件 ------------------ 发件人: "杨翊 SionYang"<notifications@github.com>; 发送时间: 2020年11月27日(星期五) 下午5:06 收件人: "alibaba/nacos"<nacos@noreply.github.com>; 抄送: "炮打资产阶级司令部"<2484713618@qq.com>; "Author"<author@noreply.github.com>; 主题: Re: [alibaba/nacos] issues #4344 (#4353) @KomachiSion requested changes on this pull request. You need use nacos code style to reformat your change files first. Please see style/codeStyle.md in source code — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

您修改了这一个文件，然后用的idea的reformat功能，可能不是您手动而是idea自动触发的，由于codeStyle设置的不同，导致您改动了文件的其他地方的codestyle，所以checkstyle就通不过了，还是建议您看一下源码目录下 style/codeStyle.ms 设置一下codestyle。然后再手动把这个文件reformat一下，重新提交。

chuntaojun · 2020-11-30T08:58:50Z

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java

@@ -64,6 +64,7 @@
 *
 * @author Nacos
 */
+@SuppressWarnings("checkstyle:EmptyLineSeparator")


没有特殊原因，不要使用@SuppressWarnings，请按照规定修复code-style问题

KomachiSion · 2020-11-30T11:16:23Z

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java

@@ -65,9 +65,9 @@
 * @author Nacos
 */
 public class ClientWorker implements Closeable {
-    
+


请不要改变代码缩进，您真的正确导入nacos codeStyle了吗？正确导入的话reformat一次应该就不会出现这种改动了。

KomachiSion · 2020-12-01T01:28:23Z

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java

-        CacheData cache = getCache(dataId, group);
-        if (null != cache) {
-            return cache;
-        }


I think this part logic can't be remove.

why? putIfAbsent将会返回old value。

如果能get到old value，下面一堆逻辑其实都不需要执行了吧。

KomachiSion · 2020-12-01T01:33:48Z

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java

        }
-
+        // reset so that server not hang this check
+        lastCacheData.setInitializing(true);


There are some different with old logic.
In old logic, it seems that only not new cacheData need to set Initializing.
But in your new logic, all cacheData what ever old or new will set Initializing. It might cause bugs.

CacheData cacheFromMap = getCache(dataId, group); // multiple listeners on the same dataid+group and race condition,so double check again //other listener thread beat me to set to cacheMap if (null != cacheFromMap) { // the cache data from cache, means it is old cache cache = cacheFromMap; // set cache as old cache //reset so that server not hang this check cache.setInitializing(true); // set old cache Initializing } else { int taskId = cacheMap.get().size() / (int) ParamUtil.getPerTaskConfigSize(); cache.setTaskId(taskId); }

CacheData cacheData = new CacheData(configFilterChainManager, agent.getName(), dataId, group); CacheData lastCacheData = cacheMap.putIfAbsent(key, cacheData); if (lastCacheData == null) { // old cache non-exist ... lastCacheData = cacheData; // set old cache as new cache ... } lastCacheData.setInitializing(true); // set new cache Initializing,

XiaoWeiKIN · 2020-12-01T02:29:32Z

 public CacheData(ConfigFilterChainManager configFilterChainManager, String name, String dataId, String group,
            String tenant) {
        if (null == dataId || null == group) {
            throw new IllegalArgumentException("dataId=" + dataId + ", group=" + group);
        }
        this.name = name;
        this.configFilterChainManager = configFilterChainManager;
        this.dataId = dataId;
        this.group = group;
        this.tenant = tenant;
        listeners = new CopyOnWriteArrayList<ManagerListenerWrap>();
        this.isInitializing = true;
        this.content = loadCacheContentFromDiskLocal(name, dataId, group, tenant);
        this.md5 = getMd5String(content);
    }

new CacheData 的isInitializing 属性是true，所以我觉得可以减少一次判断

paderlol · 2020-12-02T14:58:06Z

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java

+        // multiple listeners on the same dataid+group and race condition
+        CacheData lastCacheData = cacheMap.putIfAbsent(key, cacheData);
+        if (lastCacheData == null) {
+            //fix issue # 1317


这里没有设置taskId

int taskId = cacheMap.size() / (int) ParamUtil.getPerTaskConfigSize();

这里develop分支也么有设置taskId，默认都是0。

这个需要设置的,不设置是无法进行长轮询的,这里应该是个bug，在2.0里面已经得到修复了

paderlol

addCacheDataIfAbsent(String dataId, String group, String tenant) 新增的时候没有设置taskId

int taskId = cacheMap.size() / (int) ParamUtil.getPerTaskConfigSize();

KomachiSion · 2020-12-07T05:31:44Z

@chuntaojun @paderlol Please double check again.

@TsingLiang Please make sure this refactor will not affect the runtime.

@christophe-liwei I found that your commit user is not your github id. So the CLA can't pass. Please re-head your commit and set your git user and email as your githubid and github email, otherwise CLA no sign we can't merge this PR.

KomachiSion · 2020-12-07T11:37:07Z

Still no right for commit log user, and CLA still pending. If you don't know how to fix this problem. You can close PR, and checkout a new branch from develop and config git user and email before you commit changes, and then re-submit a new PR with your new branch.

XiaoWeiKIN · 2020-12-08T01:34:19Z

Still no right for commit log user, and CLA still pending. If you don't know how to fix this problem. You can close PR, and checkout a new branch from develop and config git user and email before you commit changes, and then re-submit a new PR with your new branch.

如何签署cla

KomachiSion · 2020-12-09T11:48:02Z

Still no right for commit log user, and CLA still pending. If you don't know how to fix this problem. You can close PR, and checkout a new branch from develop and config git user and email before you commit changes, and then re-submit a new PR with your new branch.

如何签署cla

你已经签署了，但是你的git 提交记录还有你以前本地用户wangxiaowei14227的提交记录。

如果你不知道如何去掉那些提交，建议重新从develop重新拉一个分支，然后重新用github的user和email作为提交用户再提交一次，重新创建一个PR。这样就不会有问题。

XiaoWeiKIN · 2020-12-10T01:31:00Z

@KomachiSion 好的

优化锁的使用

a497d81

KomachiSion requested changes Nov 27, 2020

View reviewed changes

KomachiSion requested review from chuntaojun, yanlinly and TsingLiang November 27, 2020 09:06

chuntaojun requested changes Nov 27, 2020

View reviewed changes

checkstyle

9f524f8

chuntaojun requested changes Nov 30, 2020

View reviewed changes

chuntaojun changed the title ~~issues #4344~~ [ISSUE #4344] cacheMap property optimization Nov 30, 2020

chuntaojun added area/Client Related to Nacos Client SDK kind/refactor labels Nov 30, 2020

checkstyle format

f8a5861

KomachiSion requested changes Nov 30, 2020

View reviewed changes

checkstyle format

3da0f5a

KomachiSion requested changes Dec 1, 2020

View reviewed changes

old cacheData

4290f3d

KomachiSion previously approved these changes Dec 2, 2020

View reviewed changes

paderlol reviewed Dec 2, 2020

View reviewed changes

paderlol requested changes Dec 2, 2020

View reviewed changes

cacheData taskId

0121cfe

XiaoWeiKIN dismissed KomachiSion’s stale review via 0121cfe December 3, 2020 02:19

KomachiSion previously approved these changes Dec 7, 2020

View reviewed changes

KomachiSion requested review from paderlol, KomachiSion and chuntaojun December 7, 2020 05:29

edit username

6af4a75

XiaoWeiKIN dismissed KomachiSion’s stale review via 6af4a75 December 7, 2020 06:58

edit username

3e523df

XiaoWeiKIN closed this Dec 10, 2020

KomachiSion mentioned this pull request Dec 10, 2020

[ISSUE #4344] cacheMap property optimization #4445

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE #4344] cacheMap property optimization #4353

[ISSUE #4344] cacheMap property optimization #4353

XiaoWeiKIN commented Nov 27, 2020

CLAassistant commented Nov 27, 2020 •

edited

KomachiSion left a comment

XiaoWeiKIN commented Nov 27, 2020 via email

chuntaojun Nov 27, 2020

XiaoWeiKIN Nov 27, 2020

KomachiSion Dec 1, 2020

chuntaojun Nov 27, 2020

KomachiSion commented Nov 30, 2020

chuntaojun Nov 30, 2020

KomachiSion Nov 30, 2020

KomachiSion Dec 1, 2020

XiaoWeiKIN Dec 1, 2020

KomachiSion Dec 1, 2020

KomachiSion Dec 1, 2020

XiaoWeiKIN commented Dec 1, 2020

paderlol Dec 2, 2020

XiaoWeiKIN Dec 2, 2020

paderlol Dec 2, 2020

paderlol left a comment

KomachiSion commented Dec 7, 2020

KomachiSion commented Dec 7, 2020 •

edited

XiaoWeiKIN commented Dec 8, 2020

KomachiSion commented Dec 9, 2020

XiaoWeiKIN commented Dec 10, 2020

[ISSUE #4344] cacheMap property optimization #4353

[ISSUE #4344] cacheMap property optimization #4353

Conversation

XiaoWeiKIN commented Nov 27, 2020

CLAassistant commented Nov 27, 2020 • edited

KomachiSion left a comment

Choose a reason for hiding this comment

XiaoWeiKIN commented Nov 27, 2020 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KomachiSion commented Nov 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiaoWeiKIN commented Dec 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paderlol left a comment

Choose a reason for hiding this comment

KomachiSion commented Dec 7, 2020

KomachiSion commented Dec 7, 2020 • edited

XiaoWeiKIN commented Dec 8, 2020

KomachiSion commented Dec 9, 2020

XiaoWeiKIN commented Dec 10, 2020

CLAassistant commented Nov 27, 2020 •

edited

KomachiSion commented Dec 7, 2020 •

edited