Re: [PR] [ISSUE #5428]Stop reading heartbeat load fields as part of the instance address (eventmesh)

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #5423:
URL: https://github.com/apache/eventmesh/pull/5423#discussion_r4321001505


##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/cluster/ClusterMembership.java:
##########
@@ -200,17 +200,34 @@ public String addressOf(String instanceId) {
         if (val == null) {
             return null;
         }
-        int sep = val.indexOf('|');
-        if (sep < 0) {
+        return addressFromHeartbeat(val);
+    }
+
+    /**
+     * Extracts {@code host:port} from a heartbeat value of the form
+     * {@code <timestamp>|<address>[|<active>|<inflow>|<outflow>|<cpu>]}.
+     *
+     * <p>The load fields were appended to the value in §3.2. Taking everything after the first
+     * separator therefore yields {@code host:port|0|0|0|0.42}, which {@link HttpForwarder} then
+     * turns into the illegal URL {@code http://host:port|0|0|0|0.42/internal/forward}, silently
+     * degrading cluster delivery to local-only delivery. Cut at the load separator instead.
+     *
+     * <p>Shared by {@link #addressOf} and {@link NacosMetaStore#namingRegister} so the parsing
+     * stays in one unit-testable place instead of two inline copies.
+     *
+     * @return the address, or {@code null} when the value carries no timestamp/address separator
+     */
+    static String addressFromHeartbeat(String value) {
+        if (value == null) {
+            return null;
+        }
+        int sep = value.indexOf('|');
+        if (sep <= 0) {
             return null;
         }
-        // Heartbeat value layout: "<ts>|<addr>|<active>|<inflow>|<outflow>|<cpu>" — §3.2 appends
-        // the load fields AFTER the address. Strip them so callers get a routable "host:port";
-        // otherwise HttpForwarder builds "http://host:port|0|0|0|0.42/internal/forward" and every
-        // cross-instance forward fails with an illegal-URL error once a LoadMeter is wired.
-        String rest = val.substring(sep + 1);
-        int addrSep = rest.indexOf('|');
-        return addrSep < 0 ? rest : rest.substring(0, addrSep);
+        String rest = value.substring(sep + 1);
+        int loadSep = rest.indexOf('|');
+        return loadSep < 0 ? rest : rest.substring(0, loadSep);
     }

Review Comment:
   `addressFromHeartbeat` can return an empty string for inputs like `"9500|"` or `"9500||5|..."` (empty address). That would propagate via `addressOf` and can still lead to malformed URLs / routing behavior. Consider treating an empty address segment as invalid and returning `null` when `rest` is empty or when the computed address substring is empty (e.g., `loadSep == 0`). This also keeps the method consistent with its `@return` contract describing invalid formats returning `null`.



##########
eventmesh-runtime/src/test/java/org/apache/eventmesh/runtime/cluster/ClusterMembershipLoadTest.java:
##########
@@ -107,6 +107,25 @@ void snapshotLoadHandlesPartialFields() {
         assertEquals(0L, load.outflowBytesPerSec);
     }
 
+    @Test
+    void addressOfIgnoresLoadFields() {
+        AtomicClock clock = new AtomicClock(10_000L);
+        InMemoryMetaStore meta = new InMemoryMetaStore();
+        meta.put("/em/instances/peer", "9500|h1:10105|5|5000|4000|0.42");
+        ClusterMembership m = new ClusterMembership(meta, "self", "self:10105", 15_000L, clock::get, new FencingToken());
+
+        assertEquals("h1:10105", m.addressOf("peer"),
+            "the load suffix must not become part of the forwarded address");
+    }
+
+    @Test
+    void addressFromHeartbeatHandlesBothFormats() {
+        assertEquals("h1:10105", ClusterMembership.addressFromHeartbeat("9500|h1:10105"));
+        assertEquals("h1:10105", ClusterMembership.addressFromHeartbeat("9500|h1:10105|5|5000|4000|0.42"));
+        assertNull(ClusterMembership.addressFromHeartbeat("9500"));
+        assertNull(ClusterMembership.addressFromHeartbeat(null));
+    }

Review Comment:
   The new shared parser is now used by both forwarding (`addressOf`) and Nacos registration. To cover the failure mode where the address segment is empty, add assertions for cases like `"9500|"` and `"9500||5|5000|4000|0.42"` (and/or `"|h1:10105"`). These are currently untested and would catch empty-address handling regressions (especially if you adjust `addressFromHeartbeat` to return `null` for empty addresses).



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org