refactor(FQDN): feather refactor on meta server side by Samunroyu · Pull Request #2328 · apache/incubator-pegasus

Samunroyu · 2025-11-27T13:23:53Z

This pr is mainly refactor the fqdn on meta server side.

Use marcos like GET_HOST_PORT and GET_HOST_PORTS to replace where partition_configuration objects apprea.
Refactor marco HEAD_INSERT_IP_AND_HOST_PORT_BY_DNS to process the last_drops objects in right way.
Fix the clang-tidy errors.

limowang · 2026-03-04T10:17:20Z

src/meta/meta_data.cpp


+    // TODO(yujingwei): use marco simplify the code, and the logical may should
+    // change
+    pc.__set_hp_secondaries({});


Since CLEAR_IP_AND_HOST_PORT(pc, secondaries) already performs the same steps internally, is there a reason to repeat them here?

There are any tests will fail when those code not repeat.
Support fqdn needs many pr, i will rejudge those code in the pr after.

Would it be possible to modify the implementation of the CLEAR_IP_AND_HOST_PORT macro?

limowang · 2026-03-04T10:17:36Z

src/rpc/rpc_host_port.h

 // '<field>' of 'obj'. The types of the fields are std::vector<rpc_address> and
 // std::vector<host_port>, respectively.
-#define HEAD_INSERT_IP_AND_HOST_PORT_BY_DNS(obj, field, hp)                                        \
+#define HEAD_INSERT_IP_AND_HOST_PORT_BY_DNS(obj, field, hp, target)                                \


#define HEAD_INSERT_IP_AND_HOST_PORT_BY_DNS(obj, field, hp, target)
do {
auto &_obj = (obj);
const auto &_hp = (hp);
auto &_target = (target);
_obj.field.insert(_obj.field.begin(), _hp.resolve());
if (!_obj._isset.hp##field) {
_obj._set_hp##field({});
}
obj.hp##field.insert(obj.hp##field.begin(), _hp);
_target = obj.hp##field;
DCHECK_EQ(_obj.field.size(), obj.hp##field.size());
} while (0)

Would this revised macro definition make the logic clearer?

i think it's quite clear. The macro will cause test failures without those change.
HEAD_INSERT_IP_AND_HOST_PORT_BY_DNS should be same as GET_HOSTS_PORT that us temporary variable to process fqdn.

limowang · 2026-03-11T08:56:55Z

src/meta/meta_data.cpp

    for (auto iter = drop_list.rbegin(); iter != drop_list.rend(); ++iter) {
-        if (pc.hp_last_drops.size() + 1 >= max_replica_count) {
+        // hp_last_drops is added in the steps bellow.
+        if (last_drops.size() + 1 >= max_replica_count) {


if (pc.hp_last_drops.size() + 1 >= max_replica_count)
The original implementation can be kept here. The preceding assertion guarantees that pc.hp_last_drops.size() is initially 0, so directly accessing hp_last_drops is safe. Later, the HEAD_INSERT_IP_AND_HOST_PORT_BY_DNS macro inserts the values synchronously, which avoids introducing more complex changes.

This pr refactes pc.hp_xxx to marcos.I think we shouldn't keep pc.hp_xx in the server code.

limowang · 2026-03-11T09:02:26Z

src/meta/meta_data.cpp

        }
        // similar to cc.drop_list, pc.last_drop is also a stack structure
-        HEAD_INSERT_IP_AND_HOST_PORT_BY_DNS(pc, last_drops, iter->node);
+        HEAD_INSERT_IP_AND_HOST_PORT_BY_DNS(pc, last_drops, iter->node, last_drops);


This part does not need to be modified

same as previous

limowang · 2026-03-11T09:17:47Z

src/rpc/rpc_host_port.h

    do {                                                                                           \
        auto &_obj = (obj);                                                                        \
        _obj.field.clear();                                                                        \
        _obj.__set_hp_##field({});                                                                 \


Suggested change

_obj.hp_##field.clear();

_obj.__isset.hp_##field = false;

Could we change it this way to avoid confusion caused by the repeated commands above?

_obj.__isset.hp_##field = false; will cause CHECK(pc.hp_last_drops.empty(), "last_drops of partition({}) must be empty", pid); failed.
i think keep hp_xxx empty in the partition_configuration that is acceptable.

limowang · 2026-03-11T13:17:35Z

src/meta/test/meta_data.cpp

        CLEAR_ALL;
        cc.dropped = {dropped_replica{node_list[0], dropped_replica::INVALID_TIMESTAMP, 5, 10, 12}};
        ASSERT_TRUE(construct_replica(view, rep.pid, 3));
-        ASSERT_EQ(node_list[0], pc.hp_primary);


Does this file not need to be modified, since the test version will always contain the hp_primary and hp_secondaries fields?

Copilot

Pull request overview

Refactors meta-server-side FQDN/host_port handling by standardizing access through host_port macros, adjusting last_drops processing, and cleaning up clang-tidy/style issues.

Changes:

Replaces direct hp_* field reads with GET_HOST_PORT / GET_HOST_PORTS across meta, client, and common code paths.
Refactors HEAD_INSERT_IP_AND_HOST_PORT_BY_DNS to also track an external “target” vector during insertion.
Applies misc. cleanups (includes, initialization, clang-tidy-related adjustments) and updates unit tests accordingly.

Reviewed changes

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

Show a summary per file

File	Description
src/rpc/rpc_host_port.h	Adjusts host_port conversion and refactors `HEAD_INSERT_IP_AND_HOST_PORT_BY_DNS` macro signature/behavior.
src/meta/test/meta_data.cpp	Updates tests to use host_port macros and narrows namespace usage.
src/meta/server_state.cpp	Refactors meta server state logic to read primary/secondaries/last_drops via macros.
src/meta/server_load_balancer.cpp	Uses macro-derived primary validation when registering proposals.
src/meta/partition_guardian.cpp	Refactors to macro-based host_port reads; removes `goto` and modernizes includes/types.
src/meta/meta_split_service.cpp	Updates child partition config to set hp_secondaries via macro-derived vector.
src/meta/meta_http_service.cpp	Uses macros for replica counting (primary/secondaries) in HTTP handler.
src/meta/meta_data.h	Tightens stateless config accessors and modernizes initialization defaults.
src/meta/meta_data.cpp	Refactors last_drops construction via updated macro; adjusts app_state initial hp_* fields.
src/meta/load_balance_policy.cpp	Uses macro-derived primary/secondaries for LB request generation and graph update.
src/meta/cluster_balance_policy.cpp	Uses macros for primary/secondaries when building migration info.
src/common/replication_other_types.h	Refactors partition config equality logic to use macro-derived host_port fields.
src/common/replication_common.cpp	Uses macros to evaluate membership and compute app/partition health stats.
src/client/replication_ddl_client.cpp	Uses macros when checking app readiness and listing health stats.
src/client/partition_resolver_simple.cpp	Uses macros to select routing host_port (primary or last_drops).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-12T15:44:20Z

src/rpc/rpc_host_port.h

+#define HEAD_INSERT_IP_AND_HOST_PORT_BY_DNS(obj, field, hp, target)                                \
    do {                                                                                           \
        auto &_obj = (obj);                                                                        \
        const auto &_hp = (hp);                                                                    \
+        auto &_target = (target);                                                                  \
        _obj.field.insert(_obj.field.begin(), _hp.resolve());                                      \
        if (!_obj.__isset.hp_##field) {                                                            \
            _obj.__set_hp_##field({_hp});                                                          \
+            _target.emplace_back(_hp);                                                             \
        } else {                                                                                   \
            _obj.hp_##field.insert(_obj.hp_##field.begin(), _hp);                                  \
+            _target = _obj.hp_##field;                                                             \
        }                                                                                          \
        DCHECK_EQ(_obj.field.size(), _obj.hp_##field.size());                                      \
    } while (0)


The macro updates _target inconsistently: in the !__isset branch it appends (emplace_back) whereas in the else branch it overwrites (_target = _obj.hp_##field). If the caller passes a non-empty _target (or reuses a buffer), this can diverge from _obj.hp_##field and/or duplicate entries. Make _target deterministic by assigning it from _obj.hp_##field in both branches (or remove the target param and let callers call GET_HOST_PORTS after mutation).

Copilot · 2026-03-12T15:44:21Z

src/meta/meta_split_service.cpp

    child_pc.secondaries = request.child_config.secondaries;
-    child_pc.__set_hp_secondaries(request.child_config.hp_secondaries);
+    std::vector<host_port> secondaries;
+    GET_HOST_PORTS(request.child_config, secondaries, secondaries);


child_pc.secondaries (rpc_address list) is copied from request.child_config.secondaries, while child_pc.hp_secondaries is derived via GET_HOST_PORTS(...) (which may source from either secondaries or hp_secondaries). If request.child_config only has one representation populated, child_pc.secondaries and child_pc.hp_secondaries can become inconsistent. To avoid mixed-source state, set both fields from the same source (e.g., derive rpc addresses from the computed secondaries host_port list, or use a helper/macro that sets both representations together).

Suggested change

GET_HOST_PORTS(request.child_config, secondaries, secondaries);

GET_HOST_PORTS(child_pc, secondaries, secondaries);

Copilot · 2026-03-12T15:44:21Z

src/meta/server_state.cpp

    for (auto &app_pair : _all_apps) {
        app_state &app = *(app_pair.second);
        for (const auto &pc : app.pcs) {
-            if (pc.hp_primary) {
-                node_state *ns = get_node_state(_nodes, pc.hp_primary, true);
+            host_port primary;
+            GET_HOST_PORT(pc, primary, primary);
+            if (primary) {
+                node_state *ns = get_node_state(_nodes, primary, true);
                ns->put_partition(pc.pid, true);
            }

-            for (const auto &secondary : pc.hp_secondaries) {
+            std::vector<host_port> secondaries;
+            GET_HOST_PORTS(pc, secondaries, secondaries);
+            for (const auto &secondary : secondaries) {


This introduces per-partition allocations/copies for secondaries (and similarly in other hot paths like consistency checks). If GET_HOST_PORTS copies from pc.hp_secondaries when __isset is true, this is avoidable overhead at scale. Consider a helper that returns a const std::vector<host_port>& when hp_* is set (and only materializes a temporary vector when conversion from rpc_address is needed), or add an iterator-style helper to avoid copying entirely.

Copilot · 2026-03-12T15:44:22Z

src/meta/meta_data.h

+        DCHECK(pc.__isset.hp_last_drops, "");
+        return pc.hp_last_drops;
+    }
+    std::vector<dsn::host_port> &hosts()
+    {
+        DCHECK(pc.__isset.hp_secondaries, "");
+        return pc.hp_secondaries;
+    }
+    [[nodiscard]] bool is_host(const host_port &node) const
+    {
+        DCHECK(pc.__isset.hp_secondaries, "");
+        return utils::contains(pc.hp_secondaries, node);
+    }
+    [[nodiscard]] bool is_worker(const host_port &node) const
+    {
+        DCHECK(pc.__isset.hp_last_drops, "");


These accessors now hard-require hp_last_drops / hp_secondaries to be set (__isset), whereas previously callers could safely interact with the default empty vectors. This can introduce new DCHECK failures in debug/test runs if any code path constructs/deserializes partition_configuration without setting these optional fields. Either (a) guarantee and document the invariant globally (ensure all construction/deserialization paths set these fields), or (b) make these accessors tolerant (e.g., avoid asserting on __isset and instead treat “unset” as empty).

Suggested change

DCHECK(pc.__isset.hp_last_drops, "");

return pc.hp_last_drops;

}

std::vector<dsn::host_port> &hosts()

{

DCHECK(pc.__isset.hp_secondaries, "");

return pc.hp_secondaries;

}

[[nodiscard]] bool is_host(const host_port &node) const

{

DCHECK(pc.__isset.hp_secondaries, "");

return utils::contains(pc.hp_secondaries, node);

}

[[nodiscard]] bool is_worker(const host_port &node) const

{

DCHECK(pc.__isset.hp_last_drops, "");

return pc.hp_last_drops;

}

std::vector<dsn::host_port> &hosts()

{

return pc.hp_secondaries;

}

[[nodiscard]] bool is_host(const host_port &node) const

{

return utils::contains(pc.hp_secondaries, node);

}

[[nodiscard]] bool is_worker(const host_port &node) const

{

Copilot · 2026-03-12T15:44:22Z

src/meta/meta_data.cpp

+    pc.__set_hp_secondaries({});
+    pc.__set_hp_last_drops({});


Calling __set_hp_secondaries({}) / __set_hp_last_drops({}) changes Thrift optional-field semantics by marking these fields as “set” even when empty. That can affect serialization/persistence and any logic keyed off __isset (including the GET_* macros). If the intent is only to satisfy new DCHECKs/access patterns, consider avoiding changing __isset behavior globally (e.g., relax the DCHECKs or only set these fields at the specific entry points that require them) and document the compatibility implications if this change is required.

Suggested change

pc.__set_hp_secondaries({});

pc.__set_hp_last_drops({});

Copilot · 2026-03-12T15:44:22Z

src/meta/meta_data.cpp

+    CHECK(last_drops.empty(), "last_drops of partition({}) must be empty", pid);
    for (auto iter = drop_list.rbegin(); iter != drop_list.rend(); ++iter) {
-        if (pc.hp_last_drops.size() + 1 >= max_replica_count) {
+        // hp_last_drops is added in the steps bellow.


Corrected spelling of 'bellow' to 'below'.

Suggested change

// hp_last_drops is added in the steps bellow.

// hp_last_drops is added in the steps below.

Copilot · 2026-03-12T15:44:22Z

src/meta/meta_data.cpp

    CLEAR_IP_AND_HOST_PORT(pc, secondaries);
    CLEAR_IP_AND_HOST_PORT(pc, last_drops);

+    // TODO(yujingwei): use marco simplify the code, and the logical may should


Corrected spelling of 'marco' to 'macro'.

Suggested change

// TODO(yujingwei): use marco simplify the code, and the logical may should

// TODO(yujingwei): use macro simplify the code, and the logical may should

github-actions bot added the cpp label Nov 27, 2025

Samunroyu force-pushed the dev/yjw/fqdn_client branch from 6f78fc5 to 37c315a Compare January 23, 2026 09:24

Samunroyu force-pushed the dev/yjw/fqdn_client branch from 3a2e5b6 to b0efe84 Compare February 26, 2026 07:14

refactor(FQDN): feather refactor on meta server side

b1d4b20

Samunroyu force-pushed the dev/yjw/fqdn_client branch from b0efe84 to b1d4b20 Compare February 26, 2026 07:18

Samunroyu changed the title ~~refactor(FQDN): feather refactor on client and host_port.h~~ refactor(FQDN): feather refactor on meta server side Feb 26, 2026

Samunroyu mentioned this pull request Feb 26, 2026

Support FQDN #2351

Open

4 tasks

Samunroyu marked this pull request as ready for review February 26, 2026 09:31

Samunroyu requested review from acelyc111 and empiredan and removed request for empiredan March 2, 2026 07:37

limowang reviewed Mar 6, 2026

View reviewed changes

limowang reviewed Mar 11, 2026

View reviewed changes

limowang suggested changes Mar 11, 2026

View reviewed changes

limowang reviewed Mar 11, 2026

View reviewed changes

acelyc111 requested a review from Copilot March 12, 2026 15:36

Copilot AI reviewed Mar 12, 2026

View reviewed changes

Copilot started reviewing on behalf of acelyc111 March 12, 2026 15:44 View session

	GET_HOST_PORTS(request.child_config, secondaries, secondaries);
	GET_HOST_PORTS(child_pc, secondaries, secondaries);

	// hp_last_drops is added in the steps bellow.
	// hp_last_drops is added in the steps below.

	// TODO(yujingwei): use marco simplify the code, and the logical may should
	// TODO(yujingwei): use macro simplify the code, and the logical may should

Conversation

Samunroyu commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Samunroyu Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Samunroyu commented Nov 27, 2025 •

edited

Loading

Samunroyu Mar 12, 2026 •

edited

Loading