ETCD 与 Discovery 分离#52
Open
yousongyang wants to merge 10 commits into
Open
Conversation
owent
reviewed
May 20, 2026
owent
left a comment
owent
left a comment
Owner
There was a problem hiding this comment.
AICR problem for src/atframe/modules/etcd_module.cpp:255
owent
reviewed
May 20, 2026
owent
left a comment
owent
left a comment
Owner
There was a problem hiding this comment.
AICR problem for src/atframe/modules/etcd_module.cpp:137
Owner
AI Code Review
AI Code Review Summaryetcd_module 拆分及多context支持代码审查Target: ETCD 与 Discovery 分离 审查总结本次PR将 发现的问题
其他观察
Problems (3)
Code reference:
|
There was a problem hiding this comment.
Pull request overview
This PR separates the etcd client responsibilities from service discovery by introducing a new service_discovery_module that owns discovery/topology keepalives + watchers while refactoring etcd_module into a lower-level etcd cluster wrapper. The change also migrates configuration from atapp.etcd to atapp.service_discovery_etcd and updates unit tests accordingly.
Changes:
- Add
service_discovery_moduleand wire it intoatapp::appas the internal discovery module. - Refactor
etcd_moduleAPI/surface (no longer anappmodule) and adapt discovery nodes to carry acontext_ptr. - Update tests/config fixtures to use
service_discovery_etcdand the new discovery APIs.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/CMakeLists.txt |
Adds new service discovery module sources/headers to the build. |
src/atframe/modules/service_discovery_module.cpp |
New implementation: manages etcd watchers/keepalives and updates in-memory discovery/topology sets. |
include/atframe/modules/service_discovery_module.h |
New public API for service discovery module (callbacks, paths, snapshots, etc.). |
include/atframe/modules/etcd_module.h |
Refactors etcd_module into a non-module etcd wrapper used by service discovery. |
src/atframe/atapp.cpp |
Replaces internal etcd module with service discovery module; adds curl-multi setup; updates discovery send paths and CLI discovery listing. |
include/atframe/atapp.h |
Updates app API to expose get_service_discovery_module() and adds curl-multi state. |
src/atframe/connectors/atapp_connector_atbus.cpp |
Routes topology refresh signaling through service discovery module. |
include/atframe/atapp_conf.proto |
Renames config field to service_discovery_etcd. |
src/atframe/etcdcli/etcd_discovery.cpp |
Adds context_ptr_ tracking and updates copy/version APIs. |
include/atframe/etcdcli/etcd_discovery.h |
Updates discovery node APIs to accept/store context_ptr. |
test/case/atapp_upstream_forward_test.cpp |
Switches tests to use service discovery module/global discovery and new copy_from signature. |
test/case/atapp_topology_change_test.cpp |
Same migration for topology change tests. |
test/case/atapp_message_test.cpp |
Same migration for message tests. |
test/case/atapp_etcd_module_unit_test.cpp |
Updates topology storage tests to use service discovery module storage types. |
test/case/atapp_etcd_module_test.cpp |
Migrates etcd/discovery behavior tests to the new module APIs. |
test/case/atapp_etcd_cluster_test.cpp |
Uses service discovery module’s raw etcd ctx for cluster tests. |
test/case/atapp_error_recovery_test.cpp |
Migrates recovery tests to service discovery module/global discovery. |
test/case/atapp_downstream_send_test.cpp |
Migrates downstream send tests to service discovery module/global discovery. |
test/case/atapp_discovery_test.cpp |
Updates discovery node version/copy APIs with the new parameter. |
test/case/atapp_discovery_reconnect_test.cpp |
Migrates reconnect tests to service discovery module/global discovery. |
test/case/atapp_direct_connect_test.cpp |
Adjusts direct-connect behavior expectations + migrates discovery injection/module usage. |
test/case/atapp_test_etcd.yaml |
Renames config section to service_discovery_etcd. |
test/case/atapp_test_etcd_node1.yaml |
Renames config section to service_discovery_etcd. |
test/case/atapp_test_etcd_node2.yaml |
Renames config section to service_discovery_etcd. |
test/case/atapp_test_etcd_module.yaml |
Renames config section to service_discovery_etcd. |
test/case/atapp_test_etcd_module_node1.yaml |
Renames config section to service_discovery_etcd. |
test/case/atapp_test_etcd_module_node2.yaml |
Renames config section to service_discovery_etcd. |
test/case/atapp_test_etcd_module_node3.yaml |
Adds node3 config using service_discovery_etcd. |
test/case/atapp_configure_loader_test.yaml |
Renames config section to service_discovery_etcd. |
test/case/atapp_configure_loader_test.conf |
Renames config keys to service_discovery_etcd.*. |
test/case/atapp_configure_loader_test.env.txt |
Renames env vars to ATAPP_SERVICE_DISCOVERY_ETCD_*. |
test/case/atapp_configure_loader_test.cpp |
Updates loader tests to parse/check atapp.service_discovery_etcd. |
test/case/atapp_configure_expression_test.yaml |
Renames config section to service_discovery_etcd. |
test/case/atapp_configure_expression_test.conf |
Renames config keys to service_discovery_etcd.*. |
test/case/atapp_configure_expression_test.env.txt |
Renames env vars to ATAPP_SERVICE_DISCOVERY_ETCD_*. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
owent
reviewed
May 20, 2026
owent
left a comment
owent
left a comment
Owner
There was a problem hiding this comment.
AICR problem for src/atframe/atapp.cpp:4108
owent
reviewed
May 20, 2026
owent
left a comment
owent
left a comment
Owner
There was a problem hiding this comment.
AICR problem for src/atframe/modules/service_discovery_module.cpp:246
owent
reviewed
May 20, 2026
owent
left a comment
owent
left a comment
Owner
There was a problem hiding this comment.
AICR problem for src/atframe/modules/etcd_module.cpp:257
owent
reviewed
May 20, 2026
owent
left a comment
owent
left a comment
Owner
There was a problem hiding this comment.
AICR problem for src/atframe/modules/etcd_module.cpp:272
owent
reviewed
May 20, 2026
owent
left a comment
owent
left a comment
Owner
There was a problem hiding this comment.
AICR problem for src/atframe/modules/service_discovery_module.cpp:246
owent
reviewed
May 20, 2026
owent
left a comment
owent
left a comment
Owner
There was a problem hiding this comment.
AICR problem for src/atframe/modules/service_discovery_module.cpp:232
owent
reviewed
May 20, 2026
owent
left a comment
owent
left a comment
Owner
There was a problem hiding this comment.
AICR problem for include/atframe/modules/service_discovery_module.h:42
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
owent
reviewed
May 21, 2026
owent
left a comment
owent
left a comment
Owner
There was a problem hiding this comment.
AICR problem for src/atframe/modules/service_discovery_module.cpp:237
owent
reviewed
May 21, 2026
owent
left a comment
owent
left a comment
Owner
There was a problem hiding this comment.
AICR problem for src/atframe/modules/service_discovery_module.cpp:350
owent
reviewed
Jun 10, 2026
owent
requested changes
Jun 10, 2026
yousongyang
commented
Jun 11, 2026
owent
reviewed
Jun 16, 2026
owent
reviewed
Jun 16, 2026
owent
reviewed
Jun 16, 2026
Owner
|
TODO: 如果现在要支持多个cluster，状态类接口也需要支持external cluster，比如:
TODO: reload新增和减少 external_cluster_contexts_
TODO: 索引优化
可以先加个TODO，下一期再实现（注意要有单元测试）。 |
Comment on lines
+309
to
+340
| bool discovery_keepalives_watchers_inited = discovery_keepalives_watchers_inited_; | ||
| { | ||
| cluster_context_->tick(); | ||
| if (!cluster_context_->get_etcd_module().is_cluster_closing()) { | ||
| // 不是正在关闭状态 | ||
| if (discovery_enabled_ && !discovery_keepalives_watchers_inited) { | ||
| init_service_discovery_keepalives_watchers(cluster_context_, false); | ||
| discovery_keepalives_watchers_inited_ = true; | ||
| } | ||
| if (!discovery_enabled_ && discovery_keepalives_watchers_inited) { | ||
| // 不是正在关闭状态但不需要 discovery 了 | ||
| cluster_context_->reset_internal_watchers_and_keepalives(); | ||
| discovery_keepalives_watchers_inited_ = false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (const auto &external_cluster_context : external_cluster_contexts_) { | ||
| external_cluster_context->tick(); | ||
| if (!external_cluster_context->get_etcd_module().is_cluster_closing()) { | ||
| // 不是正在关闭状态 | ||
| if (discovery_enabled_ && !discovery_keepalives_watchers_inited) { | ||
| init_service_discovery_keepalives_watchers(external_cluster_context, false); | ||
| discovery_keepalives_watchers_inited_ = true; | ||
| } | ||
| if (!discovery_enabled_ && discovery_keepalives_watchers_inited) { | ||
| // 不是正在关闭状态但不需要 discovery 了 | ||
| external_cluster_context->reset_internal_watchers_and_keepalives(); | ||
| discovery_keepalives_watchers_inited_ = false; | ||
| } | ||
| } | ||
| } |
Comment on lines
+3
to
+5
| id: 0x00000802 | ||
| id_mask: 8.8.8.8 | ||
| name: "etcd-test-node2" |
| type_name: "etcd-multi-test" | ||
|
|
||
| bus: | ||
| listen: "ipv4://127.0.0.1:22303" |
Comment on lines
+67
to
+68
| file: "../log/etcd-test-node2.%N.log" | ||
| writing_alias: "../log/etcd-test-node2.log" |
Comment on lines
+6
to
+9
| atapp: | ||
| id: 0x00000901 | ||
| id_mask: 8.8.8.8 | ||
| name: "etcd-ext-disc-node1" |
| type_name: "etcd-ext-disc-test" | ||
|
|
||
| bus: | ||
| listen: "ipv4://127.0.0.1:22401" |
Comment on lines
+94
to
+95
| file: "../log/etcd-ext-disc-node2a.%N.log" | ||
| writing_alias: "../log/etcd-ext-disc-node2a.log" |
Comment on lines
+4865
to
+4872
| int app::command_handler_disable_discovery(atfw::util::cli::callback_param params) { | ||
| if (!internal_module_service_discovery_) { | ||
| const char *msg = "Service discovery module not initialized, skip command."; | ||
| FWLOGERROR("{}", msg); | ||
| add_custom_command_rsp(params, msg); | ||
| } else if (internal_module_etcd_->is_etcd_enabled()) { | ||
| internal_module_etcd_->disable_etcd(); | ||
| const char *msg = "Etcd context is disabled now."; | ||
| } else if (internal_module_service_discovery_->is_discovery_enabled()) { | ||
| internal_module_service_discovery_->disable_discovery(); | ||
| const char *msg = "Service discovery context is disabled now."; |
Comment on lines
+4883
to
+4889
| int app::command_handler_enable_discovery(atfw::util::cli::callback_param params) { | ||
| if (!internal_module_service_discovery_) { | ||
| const char *msg = "Service discovery module not initialized, skip command."; | ||
| FWLOGERROR("{}", msg); | ||
| add_custom_command_rsp(params, msg); | ||
| } else if (internal_module_etcd_->is_etcd_enabled()) { | ||
| const char *msg = "Etcd context is already enabled, skip command."; | ||
| } else if (internal_module_service_discovery_->is_discovery_enabled()) { | ||
| const char *msg = "Service discovery context is already enabled, skip command."; |
| const char *msg = "Etcd context is enabled now."; | ||
| internal_module_service_discovery_->enable_discovery(); | ||
| if (internal_module_service_discovery_->is_discovery_enabled()) { | ||
| const char *msg = "Service discovery context is enabled now."; |
Comment on lines
+302
to
+324
| LIBATAPP_MACRO_API int service_discovery_module::tick() { | ||
| // Slow down the tick interval of etcd module, it require http request which is very slow compared to atbus | ||
| if (tick_next_timepoint_ >= get_app()->get_last_tick_time()) { | ||
| return 0; | ||
| } | ||
| tick_next_timepoint_ = get_app()->get_last_tick_time() + tick_interval_; | ||
|
|
||
| bool discovery_keepalives_watchers_inited = discovery_keepalives_watchers_inited_; | ||
| { | ||
| cluster_context_->tick(); | ||
| if (!cluster_context_->get_etcd_module().is_cluster_closing()) { | ||
| // 不是正在关闭状态 | ||
| if (discovery_enabled_ && !discovery_keepalives_watchers_inited) { | ||
| init_service_discovery_keepalives_watchers(cluster_context_, false); | ||
| discovery_keepalives_watchers_inited_ = true; | ||
| } | ||
| if (!discovery_enabled_ && discovery_keepalives_watchers_inited) { | ||
| // 不是正在关闭状态但不需要 discovery 了 | ||
| cluster_context_->reset_internal_watchers_and_keepalives(); | ||
| discovery_keepalives_watchers_inited_ = false; | ||
| } | ||
| } | ||
| } |
Comment on lines
+411
to
415
| bool enabled = 1 [(atapp.protocol.CONFIGURE) = { default_value: "true" }]; | ||
| google.protobuf.Duration timeout = 101 [(atapp.protocol.CONFIGURE) = { default_value: "31s" }]; | ||
| google.protobuf.Duration ttl = 102 [(atapp.protocol.CONFIGURE) = { default_value: "10s" }]; | ||
| google.protobuf.Duration retry_interval = 103 [(atapp.protocol.CONFIGURE) = { default_value: "3s" }]; | ||
| } |
Comment on lines
+3
to
+5
| id: 0x00000802 | ||
| id_mask: 8.8.8.8 | ||
| name: "etcd-test-node2" |
| type_name: "etcd-multi-test" | ||
|
|
||
| bus: | ||
| listen: "ipv4://127.0.0.1:22303" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.