ETCD 与 Discovery 分离 by yousongyang · Pull Request #52 · owent/libatapp

yousongyang · 2026-05-20T06:36:17Z

No description provided.

owent

AICR problem for src/atframe/modules/etcd_module.cpp:255

owent

AICR problem for src/atframe/modules/etcd_module.cpp:137

owent · 2026-05-20T06:38:24Z

AI Code Review

Updated: 2026-05-21 09:19:13 UTC | Commit: e2d2598

AI Code Review Summary

service_discovery_module architecture review

Target: ETCD 与 Discovery 分离
Author: @owent
Branch: main
Reviewers: @owent

Review Summary

Reviewed PR introducing service_discovery_module and refactoring etcd_module for multi-cluster support.

Architecture Changes

service_discovery_module wraps etcd_module, adding service_discovery_cluster_context for cluster isolation
Curl multi context moved from etcd_module to app level (shared resource)
context_ptr mechanism added to etcd_discovery_node for cluster ownership tracking
Discovery/topology callbacks moved to service_discovery_module

Findings (3 total)

medium: Keepalive actor lifecycle gap in stop flow (line 581)
low: Proto field rename breaks backward compatibility (atapp_conf.proto:579)
info: Method naming inconsistency is_etcd_enable vs is_etcd_enabled (etcd_module.h:61)

Verified Areas

Null checks present where needed (get_app(), get_bus_node())
Thread safety via recursive_mutex for callback lists
Member initialization with default values (context_ptr = 0)
Stop flow: returns 1 during cleanup, 0 after completion
Initialization order: setup_atbus() precedes setup_curl_multi()

Missing Context

Requested full implementations of app::init() and service_discovery_module.cpp to verify initialization order and callback safety. Follow-up pass pending.

Problems (3)

#	Severity	Category	Location	Message
0	MEDIUM	resource_lifecycle	`src/atframe/modules/service_discovery_module.cpp:581`	`reset_internal_watchers_and_keepalives()` clears keepalive actor lists but doesn't call `etcd_cluster::remove_keepalive()` on each actor. Actors remain registered in cluster until `cluster_.reset()` in `etcd_module::stop()` callback. If `etcd_module_.stop()` returns 1 (cleanup in progress), keepalive tick callbacks might fire after context's lists are cleared, potentially accessing dangling context references.
1	LOW	api_compatibility	`include/atframe/atapp_conf.proto:579`	Proto field renamed from `etcd` to `service_discovery_etcd` (field 401). This breaks backward compatibility for existing configurations. Users with configs using the old `etcd` field will get parse errors or silently ignored fields. While intentional for refactoring, migration documentation is needed.
2	INFO	convention	`include/atframe/modules/etcd_module.h:61`	Method renamed from `is_etcd_enabled()` (old) to `is_etcd_enable()` (new). Grammatically awkward - should be `is_etcd_enabled` or `etcd_is_enabled`. Not a correctness issue but creates naming inconsistency across API.

Code reference: `src/atframe/modules/service_discovery_module.cpp:581`

}

LIBATAPP_MACRO_API int service_discovery_module::service_discovery_cluster_context::stop() {
  reset_internal_watchers_and_keepalives();
  return etcd_module_.stop();

Code reference: `include/atframe/atapp_conf.proto:579`

  atbus_configure bus = 301;

  atapp_etcd service_discovery_etcd = 401;

  atapp_worker_pool worker_pool = 501;

Code reference: `include/atframe/modules/etcd_module.h:61`

  LIBATAPP_MACRO_API static std::string generate_etcd_path(const std::string &path);

  LIBATAPP_MACRO_API bool is_etcd_enable() const;

 private:

Powered by AICodeReviewer*

Open Issues (3)

[MEDIUM] resource_lifecycle — src/atframe/modules/service_discovery_module.cpp:581 (new in e2d2598)
[LOW] api_compatibility — include/atframe/atapp_conf.proto:579 (new in e2d2598)
[INFO] convention — include/atframe/modules/etcd_module.h:61 (new in e2d2598)

Resolved (2)

The following previously reported issues are no longer present:

~~bf01f30b5c08949f~~ ✅ Resolved
~~233da4a44933542a~~ ✅ Resolved

Copilot

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_module and wire it into atapp::app as the internal discovery module.
Refactor etcd_module API/surface (no longer an app module) and adapt discovery nodes to carry a context_ptr.
Update tests/config fixtures to use service_discovery_etcd and 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

AICR problem for src/atframe/atapp.cpp:4108

owent

AICR problem for src/atframe/modules/service_discovery_module.cpp:246

owent

AICR problem for src/atframe/modules/etcd_module.cpp:257

owent

AICR problem for src/atframe/modules/etcd_module.cpp:272

owent

AICR problem for src/atframe/modules/service_discovery_module.cpp:246

owent

AICR problem for src/atframe/modules/service_discovery_module.cpp:232

owent

AICR problem for include/atframe/modules/service_discovery_module.h:42

+  etcd_discovery_node::ptr_t local_cache_by_id = global_discovery_.get_node_by_id(node.node_discovery.id());
+  etcd_discovery_node::ptr_t local_cache_by_name = global_discovery_.get_node_by_name(node.node_discovery.name());
+  etcd_discovery_node::ptr_t new_inst;
+


Copilot

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated 4 comments.

Copilot

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated 5 comments.

owent

AICR problem for src/atframe/modules/service_discovery_module.cpp:237

owent

AICR problem for src/atframe/modules/service_discovery_module.cpp:350

Copilot

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated 4 comments.

+int app::setup_curl_multi() {
+  // init curl
+  int res = curl_global_init(CURL_GLOBAL_ALL);
+  if (res) {
+    FWLOGERROR("init cURL failed, errcode: {}", res);
+    return -1;
+  }
+  curl_global_init_ = true;
+
+  atfw::util::network::http_request::curl_share_options share_options;
+  atfw::util::network::http_request::curl_multi_options multi_options;
+  multi_options.ev_loop = get_bus_node()->get_evloop();
+  // Share DNS cache, connection, TLS sessions and etc.
+  atfw::util::network::http_request::create_curl_share(share_options, multi_options.share_context);
+  atfw::util::network::http_request::create_curl_multi(get_bus_node()->get_evloop(), curl_multi_);
+  if (!curl_multi_) {
+    FWLOGERROR("create curl multi instance failed.");
+    curl_global_cleanup();
+    curl_global_init_ = false;
+    return -1;
+  }
+  return 0;
+}


Copilot

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated 7 comments.

+    atfw::util::network::http_request::destroy_curl_multi(curl_multi_);
+  }
+  if (curl_global_init_) {
+    curl_global_cleanup();
+  }


+int app::setup_curl_multi() {
+  // init curl
+  int res = curl_global_init(CURL_GLOBAL_ALL);
+  if (res) {
+    FWLOGERROR("init cURL failed, errcode: {}", res);
+    return -1;
+  }
+  curl_global_init_ = true;
+


+      maybe_update_internal_keepalive_discovery_metadata_(false) {}
+
+LIBATAPP_MACRO_API int service_discovery_module::init() {
+  int ret = cluster_context_.init(*get_app(), get_app()->get_origin_configure().service_discovery_etcd(), nullptr);


+}
+
+LIBATAPP_MACRO_API int service_discovery_module::reload() {
+  return cluster_context_.reload(get_app()->get_origin_configure().service_discovery_etcd(), nullptr);


+      }
+    } else {
+      if (local_cache_by_id && protobuf_equal(local_cache_by_id->get_discovery_info(), node.node_discovery) &&
+          local_cache_by_id->get_context_ptr() == node.context_ptr) {


+      if (changed) {
+        local_cache_iter->second.info = topology_info.storage.info;
+        info_ptr = local_cache_iter->second.info;
+      }


-  LIBATAPP_MACRO_API const std::shared_ptr<::atframework::atapp::etcd_module> &get_etcd_module() const noexcept;
+  LIBATAPP_MACRO_API const std::shared_ptr<::atframework::atapp::service_discovery_module> &
+  get_service_discovery_module() const noexcept;



owent

AICR problem for src/atframe/modules/service_discovery_module.cpp:581

owent · 2026-05-21T09:19:07Z

+  return etcd_module_.reload(conf, log_conf);
+}
+
+LIBATAPP_MACRO_API int service_discovery_module::service_discovery_cluster_context::stop() {


MEDIUM
·
resource_lifecycle
reset_internal_watchers_and_keepalives() clears keepalive actor lists but doesn't call etcd_cluster::remove_keepalive() on each actor. Actors remain registered in cluster until cluster_.reset() in etcd_module::stop() callback. If etcd_module_.stop() returns 1 (cleanup in progress), keepalive tick callbacks might fire after context's lists are cleared, potentially accessing dangling context references.

Location: src/atframe/modules/service_discovery_module.cpp:581

Referenced code (src/atframe/modules/service_discovery_module.cpp:581):

} LIBATAPP_MACRO_API int service_discovery_module::service_discovery_cluster_context::stop() { reset_internal_watchers_and_keepalives(); return etcd_module_.stop();

Suggested fix:
Either call etcd_cluster::remove_keepalive() for each actor in reset_internal_watchers_and_keepalives(), or ensure the stop flow guarantees no callbacks fire between clearing the lists and cluster reset.

owent

AICR problem for include/atframe/atapp_conf.proto:579

owent · 2026-05-21T09:19:09Z

  atbus_configure bus = 301;

-  atapp_etcd etcd = 401;
+  atapp_etcd service_discovery_etcd = 401;


LOW
·
api_compatibility
Proto field renamed from etcd to service_discovery_etcd (field 401). This breaks backward compatibility for existing configurations. Users with configs using the old etcd field will get parse errors or silently ignored fields. While intentional for refactoring, migration documentation is needed.

Location: include/atframe/atapp_conf.proto:579

Referenced code (include/atframe/atapp_conf.proto:579):

atbus_configure bus = 301; atapp_etcd service_discovery_etcd = 401; atapp_worker_pool worker_pool = 501;

owent

AICR problem for include/atframe/modules/etcd_module.h:61

owent · 2026-05-21T09:19:11Z

-
-  LIBATAPP_MACRO_API etcd_watcher::watch_event_fn_t_ptr create_discovery_watcher_callback_list_wrapper();
-  LIBATAPP_MACRO_API etcd_watcher::watch_event_fn_t_ptr create_topology_watcher_callback_list_wrapper();
+  LIBATAPP_MACRO_API bool is_etcd_enable() const;


INFO
·
convention
Method renamed from is_etcd_enabled() (old) to is_etcd_enable() (new). Grammatically awkward - should be is_etcd_enabled or etcd_is_enabled. Not a correctness issue but creates naming inconsistency across API.

Location: include/atframe/modules/etcd_module.h:61

Referenced code (include/atframe/modules/etcd_module.h:61):

LIBATAPP_MACRO_API static std::string generate_etcd_path(const std::string &path); LIBATAPP_MACRO_API bool is_etcd_enable() const; private:

Copilot AI review requested due to automatic review settings May 20, 2026 06:36