You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/05/31 14:34:44 UTC
[GitHub] [apisix] Demogorgon314 opened a new pull request #4347: optimize: use events optimize nacos service discovery
Demogorgon314 opened a new pull request #4347:
URL: https://github.com/apache/apisix/pull/4347
### What this PR does / why we need it:
#4346
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [x] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #4347: perf: use events optimize nacos service discovery
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #4347:
URL: https://github.com/apache/apisix/pull/4347#discussion_r642733798
##########
File path: apisix/discovery/nacos.lua
##########
@@ -311,7 +322,17 @@ local function fetch_full_registry(premature)
})
end
end
+ local new_apps_md5sum = ngx.md5(core.json.encode(up_apps))
+ local old_apps_md5sum = ngx.md5(core.json.encode(applications));
Review comment:
```suggestion
local old_apps_md5sum = ngx.md5(core.json.encode(applications))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Demogorgon314 commented on a change in pull request #4347: perf: use events optimize nacos service discovery
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on a change in pull request #4347:
URL: https://github.com/apache/apisix/pull/4347#discussion_r642852507
##########
File path: apisix/discovery/nacos.lua
##########
@@ -311,7 +322,17 @@ local function fetch_full_registry(premature)
})
end
end
+ local new_apps_md5sum = ngx.md5(core.json.encode(up_apps))
+ local old_apps_md5sum = ngx.md5(core.json.encode(applications));
+ if new_apps_md5sum == old_apps_md5sum then
+ return
+ end
applications = up_apps
+ local ok, err = events.post(events_list._source, events_list.updating, applications)
+ if not ok then
+ log.error("post_event failure with ", events_list._source,
+ ", update application error: ", err)
Review comment:
fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4347: perf: use events optimize nacos service discovery
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4347:
URL: https://github.com/apache/apisix/pull/4347#discussion_r642571656
##########
File path: apisix/discovery/nacos.lua
##########
@@ -344,6 +365,18 @@ function _M.init_worker()
error('invalid nacos configuration: ' .. err)
return
end
+
+ events = require("resty.worker.events")
+ events_list = events.event_list(
+ "discovery_nacos_update_application",
+ "updating"
Review comment:
Need to add `workers(4);` to https://github.com/apache/apisix/blob/0d7fa659d557a83224879cd1bfa823deff606bf7/t/discovery/nacos.t#L23 to check if it works
Don't forget to update https://github.com/apache/apisix/blob/master/docs/en/latest/discovery/nacos.md#service-discovery-via-nacos
##########
File path: apisix/discovery/nacos.lua
##########
@@ -80,8 +83,16 @@ local schema = {
}
-local _M = {}
+local _M = {
+ version = 0.2,
+}
+local function discovery_nacos_callback(data, event, source, pid)
+ applications = data
+ log.notice("update local variable application, event is: ", event,
+ "source: ", source, "server pid:", pid,
Review comment:
Need to fix the indent.
Please update other similar places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers merged pull request #4347: perf: use events optimize nacos service discovery
Posted by GitBox <gi...@apache.org>.
tokers merged pull request #4347:
URL: https://github.com/apache/apisix/pull/4347
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #4347: perf: use events optimize nacos service discovery
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #4347:
URL: https://github.com/apache/apisix/pull/4347#discussion_r642734218
##########
File path: apisix/discovery/nacos.lua
##########
@@ -311,7 +322,17 @@ local function fetch_full_registry(premature)
})
end
end
+ local new_apps_md5sum = ngx.md5(core.json.encode(up_apps))
+ local old_apps_md5sum = ngx.md5(core.json.encode(applications));
+ if new_apps_md5sum == old_apps_md5sum then
+ return
+ end
applications = up_apps
+ local ok, err = events.post(events_list._source, events_list.updating, applications)
+ if not ok then
+ log.error("post_event failure with ", events_list._source,
+ ", update application error: ", err)
Review comment:
Bad indent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #4347: perf: use events optimize nacos service discovery
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on pull request #4347:
URL: https://github.com/apache/apisix/pull/4347#issuecomment-851613561
I think additional test cases are needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4347: perf: use events optimize nacos service discovery
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4347:
URL: https://github.com/apache/apisix/pull/4347#discussion_r643611910
##########
File path: docs/en/latest/discovery/nacos.md
##########
@@ -27,7 +27,7 @@ This is experimental discovery module for Nacos.
The performance of this module needs to be improved:
-1. avoid synchroning configuration in each workers. You can refer the implementation in `consul_kv.lua`.
+1. ~~avoid synchroning configuration in each workers. You can refer the implementation in `consul_kv.lua`.~~
Review comment:
We can directly remove it.
##########
File path: apisix/discovery/nacos.lua
##########
@@ -80,8 +83,16 @@ local schema = {
}
-local _M = {}
+local _M = {
+ version = 0.2,
Review comment:
We don't need to add the version number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Demogorgon314 commented on a change in pull request #4347: perf: use events optimize nacos service discovery
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on a change in pull request #4347:
URL: https://github.com/apache/apisix/pull/4347#discussion_r643616241
##########
File path: apisix/discovery/nacos.lua
##########
@@ -80,8 +83,16 @@ local schema = {
}
-local _M = {}
+local _M = {
+ version = 0.2,
Review comment:
Thx. I fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org