feat(disvocery): Add zookeeper discovery - #12772
Conversation
dongjiang1989
changed the title
7bfba46 to
01f70df
Compare
dosubot
Bot
added
size:L
Baoyuantop
left a comment
Baoyuantop
left a comment
There was a problem hiding this comment.
Hi @dongjiang1989, thanks for your contribution. Could you add documentation and testing for this feature? You can refer to other service discovery modules for reference.
Thanks @Baoyuantop |
3993d37 to
ea3c33e
Compare
dosubot
Bot
added
size:XL
f25602f to
3435db2
Compare
|
@Baoyuantop Please review docs, thanks |
|
Hi @dongjiang1989, additional tests are needed to verify that the function works correctly. |
Got it. I'll add UT soon |
Baoyuantop
added
the
wait for update
|
Hi @dongjiang1989, any update? |
9ce82a8 to
836a97b
Compare
dosubot
Bot
added
size:XXL
448b4c1 to
f81575e
Compare
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
80c05ef to
00618b0
Compare
Thanks @Baoyuantop |
| end | ||
|
|
||
| -- Scheduled fetch of all service instances (full cache update)） | ||
| local function fetch_all_services() |
There was a problem hiding this comment.
Do not fetch all services from ZooKeeper. This can cause significant network overhead when there are many services in ZooKeeper. It is recommended to refer to the Nacos implementation and use a timer to iterate through the currently configured Upstream, Route, and Service resources of APISIX, collecting all currently used service_names (with discovery_type set to zookeeper). Only perform ZooKeeper queries and cache updates on these "in-use" services.
| log.info("zookeeper_conf:", core.json.encode(zookeeper_conf)) | ||
| -- Start the timer | ||
| if not fetch_timer then | ||
| fetch_timer = ngx.timer.every(zookeeper_conf.fetch_interval, fetch_all_services) |
There was a problem hiding this comment.
It is recommended that only privileged processes request data from ZooKeeper and write it to the shared dict, while other processes read the data from the shared dict.
Signed-off-by: jiangdong <jiangdong@iflytek.com>
|
Hi @dongjiang1989, following up on the previous review comments. Please let us know if you have any updates. Thank you. |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions. |
|
Hi @dongjiang1989, following up on the previous review comments. Please let us know if you have any updates. Thank you. |
moonming
left a comment
moonming
left a comment
There was a problem hiding this comment.
Hi @dongjiang1989, thank you for the Zookeeper discovery implementation!
ZooKeeper integration has been requested by the community (#3231, #6687), so there's clear demand. However, at 1088 lines across 12 files, this is a substantial addition.
Key considerations:
- Maintenance burden: Adding a new discovery module means ongoing maintenance. Are you or your team willing to help maintain this long-term?
- Dependency: Does this introduce new Lua/C library dependencies? If so, they need to be added to
.requirements. - Rebase needed: This PR has been inactive for a while and likely has conflicts.
- Test completeness: Please ensure tests cover: connection failures, session expiry, node add/remove events, reconnection logic, and authentication (if supported).
If you're still actively working on this, please rebase and let us know. We'd need to evaluate the maintenance commitment before merging a new discovery module. Thank you!
|
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
3 participants
Description
Which issue(s) this PR fixes:
Fixes #3231 #6687
Add zookeeper discovery
Checklist