Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Code Review - fix(#4274): getReplicaAddresses excludes localPeer instead of leader

Overview

The PR correctly identifies and fixes a real bug: getReplicaAddresses() was filtering out localPeerId (the request-serving node) instead of the Raft leader, causing follower nodes to return an inconsistent cluster topology view. The fix is minimal and well-reasoned.

Fix (RaftHAServer.java) - Correct

final RaftPeerId leaderId = getLeaderId();
final RaftPeerId excludeId = leaderId != null ? leaderId : localPeerId;

The logic is sound. The fallback to localPeerId when no leader is elected (during an election) is pragmatic - it preserves the prior behavior for a transient state, avoiding new failure modes. One minor suggestion: a short inline comment would make the fallback intent explicit for future readers:

// Fall back to localPeerId during elections (no leader yet) to preserve prior behaviour
final RaftPeerId excludeId = leaderId != null ? leaderId : localPeerId;

Test (RaftHTTP2ServersIT.java) - Mostly Good, One Fragility

The regression test covers the three key invariants well. Two concerns:

1. String-based replica set comparison is order-sensitive

assertThat(replicaAddresses).isEqualTo(firstReplicaAddresses);

getReplicaAddresses() builds the comma-separated list by iterating raftGroup.getPeers(). If the peer order happens to differ between nodes, this assertion would produce a false failure even when the replica set is identical. The fix should use set comparison:

assertThat(Set.of(replicaAddresses.split(",")))
    .as("server %d: replicaAddresses must be consistent across all nodes", i)
    .isEqualTo(Set.of(firstReplicaAddresses.split(",")));

2. Missing non-empty assertion

In a 2-node cluster there should always be exactly one replica address. A bug that makes replicaAddresses consistently empty on every node would pass all current assertions (empty never contains leaderAddress, and "".equals("")). Adding this would close that gap:

assertThat(replicaAddresses).as("server %d: replicaAddresses must not be empty", i).isNotEmpty();

Documentation File (docs/4274-cluster-topology-consistency.md) - Should Be Removed

The committed markdown file duplicates what is already in the PR description. Once this PR is merged the file will be disconnected from the issue context and will just drift out of date alongside the code. The root-cause analysis belongs in the PR body and commit message, not in version control. Please remove this file from the PR.

Port Pattern Inconsistency (Minor)

Existing tests in this file use a hardcoded port pattern ("http://127.0.0.1:248" + serverIndex), while the new test uses the dynamic getServer(i).getHttpServer().getPort(). The dynamic approach is strictly better (no reliance on fixed port numbering), so this is actually a good change - worth noting that the existing tests could be improved the same way in a follow-up.

Summary

The core change is correct and the test is a good addition. Addressing the replicaAddresses string comparison fragility and removing the docs file are the two items worth fixing before merge.

Code Review

The fix is correct. The root cause is clearly identified: getReplicaAddresses() and getStats() were excluding localPeerId (the serving node) instead of the Raft leader, producing an inconsistent topology view from follower nodes. The change is minimal and well-scoped.

RaftHAServer.java - Correct fix, comment recommended

final RaftPeerId excludeId = leaderId != null ? leaderId : localPeerId;

The fallback to localPeerId during elections is pragmatic. A short inline comment would help future readers understand why this is intentional:

// During elections (no leader yet), fall back to localPeerId to preserve prior behaviour
final RaftPeerId excludeId = leaderId != null ? leaderId : localPeerId;

Since the semantics of this method were apparently non-obvious (the bug proves it), a brief Javadoc on getReplicaAddresses() stating "Returns the HTTP addresses of all non-leader peers" would also help prevent a recurrence.

RaftHTTP2ServersIT.java - Two test robustness issues

1. doesNotContain uses substring matching

assertThat(replicaAddresses).doesNotContain(leaderAddress);

AssertJ's doesNotContain on a String checks for a substring, not a list element. If leaderAddress is "127.0.0.1:2480" and a replica is "127.0.0.1:24800", the substring "127.0.0.1:2480" appears inside "127.0.0.1:24800", producing a false failure. Use a split-and-check instead:

assertThat(List.of(replicaAddresses.split(",")))
    .as("server %d: leaderAddress must not appear in replicaAddresses", i)
    .doesNotContain(leaderAddress);

2. Ordering-sensitive equality check

assertThat(replicaAddresses).isEqualTo(firstReplicaAddresses);

getReplicaAddresses() iterates raftGroup.getPeers() - if peer ordering ever differs between nodes, this would give a false failure even when the set of replicas is correct. Use set comparison:

assertThat(Set.of(replicaAddresses.split(",")))
    .as("server %d: replicaAddresses must be consistent across all nodes", i)
    .isEqualTo(Set.of(firstReplicaAddresses.split(",")));

3. Missing non-empty assertion

In a 2-node cluster there is always exactly one replica. A bug that returns an empty replicaAddresses consistently from all nodes would pass all current assertions (empty string never contains anything, "".equals("") is true). Add:

assertThat(replicaAddresses)
    .as("server %d: replicaAddresses must not be empty", i)
    .isNotEmpty();

docs/4274-cluster-topology-consistency.md - Remove this file

This file is a process log of the development session (including notes about AI review cycles and "loop exited with timeout state"). It duplicates the PR description, will become disconnected from context after merge, and is not useful as product documentation. The root-cause analysis belongs in the PR body and commit message. Please remove it before merging.

Summary

The core change is solid. The three test robustness items and the docs file are the things worth addressing before merge.

Code Review for PR 4280. Overview: This PR fixes a real bug in RaftHAServer - both getReplicaAddresses() and getStats() were excluding localPeerId from the replica list, when they should exclude the leader. On follower nodes this produced an inverted view. The fix is correct. Issue 1 - docs/4274-cluster-topology-consistency.md should not be committed, it is an AI process artifact; remove before merging. Issue 2 - replicaAddresses string comparison is order-dependent, fine for 2 nodes but a set-based comparison is more robust for 3+. Issue 3 - election-period topology inconsistency is worth a comment. Positives: fix is minimal and consistent across both methods, stepDown() at line 784 correctly keeps using localPeerId, regression test has proper try/finally and AssertJ assertions. Summary: correct and ready to merge after removing the docs artifact file.

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.15%. Comparing base (6e4de2c) to head (86fb190).

@@             Coverage Diff              @@
##               main    #4280      +/-   ##
============================================
- Coverage     64.16%   64.15%   -0.02%     
+ Complexity      445      444       -1     
============================================
  Files          1645     1645              
  Lines        127476   127480       +4     
  Branches      27324    27326       +2     
============================================
- Hits          81796    81781      -15     
- Misses        34145    34170      +25     
+ Partials      11535    11529       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.