-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Nacos supports fuzzy config listen. #11856
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #11856 +/- ##
=============================================
- Coverage 67.81% 66.61% -1.20%
- Complexity 8893 8945 +52
=============================================
Files 1237 1254 +17
Lines 40443 41399 +956
Branches 4286 4422 +136
=============================================
+ Hits 27425 27580 +155
- Misses 11046 11805 +759
- Partials 1972 2014 +42
... and 32 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Is this PR should be merged into Nacos 3.0 branch? @shiyiyue1102 |
// Check if the context is consistent with the server | ||
if (context.getIsConsistentWithServer().get()) { | ||
// Skip if a full synchronization is not needed | ||
if (!needAllSync) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
为什么不在前面直接返回,要在for循环来continue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
因为ClientWorker中包含所有的模糊监听上下文(多个模糊监听表达式),每个都有属于自身的isConsistentWithServer字段,所以需要在for循环里面continue。(可能存在有些已经与服务端同步的,有些没有同步的)
/** | ||
* Flag indicating whether the context is discarded. | ||
*/ | ||
private volatile boolean isDiscard = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isDiscard字段的存在是不是让逻辑更复杂了,取消订阅的时候直接删除订阅的数据是不是就可以了
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isDiscard如果为空时,代表此模糊监听上下文已经没有Listener,需要通知服务端取消订阅,让服务端不再进行通知。
如果去除该字段,直接用listener == null判断其实也可行。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不过原有普通订阅也有这个字段,保持一致没毛病
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
// Get the connection for the client | ||
Connection connection = connectionManager.getConnection(event.getClientId()); | ||
if (connection == null) { | ||
// If connection is not available, return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里增加一些日志更好,不然可能客户端任务订阅成了,但是服务端这边又没有日志验证
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ConfigExecutor.getClientConfigNotifierServiceExecutor() | ||
.schedule(retryTask, retryTask.tryTimes * 2L, TimeUnit.SECONDS); | ||
} else { | ||
// Client is already offline, ignore the task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
建议补充client 离线的 log,方便后续问题排查
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
// Execute the fuzzy listen operation | ||
ConfigBatchFuzzyListenResponse listenResponse = (ConfigBatchFuzzyListenResponse) requestProxy( | ||
rpcClient, configBatchFuzzyListenRequest); | ||
if (listenResponse != null && listenResponse.isSuccess()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里的设计是只要请求服务端成功就认为本地的context列表与服务端一致?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FuzzyListenContext的字段如下:
isConsistentWithServer:是否与服务端同步
isInitializing:是否在初始化中
当请求服务端成功,isConsistentWithServer = true,代表本地的context与服务端同步了(但是列表未一致),但是此时isInitializing = true,需要等到服务端将所有的配置推送到客户端后,才将isInitializing = false,代表已经完成了初始化,此时本地的context的配置列表才与服务端一致。
然后在断开连接时,会将isConsistentWithServer = false,代表需要重新同步。
目前的实现是垮了 namespace ,这个与 Nacos 的隔离理念是否不符 |
大佬,我没太理解这句话的意思,目前的实现客户端是不跨namespace的,服务端是跨namespace的,只是说request带了namespace。 |
嗯,我是担心不同namespace的监听数据冲突,我刚刚又仔细看了一下,确实客户端维护的 fuzzyListenContextMap 是每个ns都是单独的,服务端的唯一key也包含了namespace。 |
@KomachiSion 是否考虑先合并到2.x呢? |
以前单个配置监听定时批量监听的时候会返回change的配置列表,相当于有个兜底的与服务端对账,模糊监听的当时有考虑这个吗 |
} | ||
|
||
// Execute fuzzy listen operation for addition | ||
doExecuteConfigFuzzyListen(addContextMap, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里先分类成 addContextMap和removeContextMap,再调用两次doExecuteConfigFuzzyListen 看着有点绕,为什么不直接遍历fuzzyListenContextMap,执行一次doExecuteConfigFuzzyListen呢,反正doExecuteConfigFuzzyListen内部都会判断分类
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
确实,仔细看了下,这个确实有点复杂了,后续我更新下。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
/** | ||
* Set of fuzzy listening contexts. | ||
*/ | ||
private Set<Context> contexts = new HashSet<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里的context为什么不直接用FuzzyListenContext ,相当于两个类做同样的事情
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里理论上使用FuzzyListenContext是ok的,但是FuzzyListenContext 不单单是一个实体,还承载着通知listener的能力,所以这个请求里面就直接使用了一个内部类去存关键字段,这里是否有必要直接使用FuzzyListenContext我也有点纠结......
这里的ConfigBatchFuzzyListenRequest分别有两个作用:
|
…e-98/nacos into support-fuzzy-listen-config
目前对账的逻辑是每隔5分钟服务端会把所有配置重新推一遍,这个开销是不是太大了,如果多个客户端同时监听服务端多个配置,那推送的量是很大的,这块感觉需要优化一下 |
优化思路:客户端批量对比的时候将同一个groupPattern下的配置 md5 集合再计算一个 md5 提供给服务端,服务端如果对比发现有变更的话告诉客户端需要变更,客户端再将同一个groupPattern下的所有 md5 传给服务端,服务端再将不一致的重新推一编。这个只是一个简单的思路,应该还有更好的方案 |
后续有时间会处理一下。 |
What is the purpose of the change
Nacos supports fuzzy config listening.
Config a fuzzy listen scheme(配置模糊监听方案)
Brief changelog
Nacos supports fuzzy config listening.
Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
[ISSUE #123] Fix UnknownException when host config not exist
. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=true
to make sure basic checks pass. Runmvn clean install -DskipITs
to make sure unit-test pass. Runmvn clean test-compile failsafe:integration-test
to make sure integration-test pass.