You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "smengcl (via GitHub)" <gi...@apache.org> on 2026/10/08 10:05:30 UTC
[PR] RATIS-2082. setConfiguration should not ignore address changes of existing peers [ratis]
smengcl opened a new pull request, #1630:
URL: https://github.com/apache/ratis/pull/1630
Generated-by: Claude Code (Opus 5.5)
## What changes were proposed in this pull request?
A `setConfiguration` request with the same peer IDs and priorities but different addresses is treated as "no change", so the addresses stored in the Raft log and `raft-meta.conf` can never be updated.
`RaftConfigurationImpl.hasNoChange` compares only IDs and priorities. This patch adds an overload that also compares peer addresses and uses it in `setConfigurationAsync`. `RaftPeer.equals` is not changed.
Running servers keep their existing connections. The new addresses are used after a restart.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2082
## How was this patch tested?
Added `RaftReconfigurationBaseTest.testAddressChangeRequest`.
`testNoChangeRequest` now sends the peers of the current configuration, since `cluster.getPeers()` returns bind addresses that differ from the configured ones.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2082. setConfiguration should not ignore address changes of existing peers [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1630:
URL: https://github.com/apache/ratis/pull/1630#issuecomment-6129282249
Instead of using setConf, how about add a startup option to run RaftMetaConfCommand before startup?
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2082. setConfiguration should not ignore address changes of existing peers [ratis]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #1630:
URL: https://github.com/apache/ratis/pull/1630#issuecomment-6090883991
> > ... restart the peers ...
>
> If restarting is needed, could we just use raftMetaConf command (RaftMetaConfCommand)?
>
> Using setConf may have some unnecessary complication since there are two addresses for the same peer at the same time. I am not sure if the code can handle them correctly.
@szetszwo Thanks, that is a fair concern, and I agree that changing addresses at runtime is tricky. To be clear about the scope: this patch only changes what is recorded. It does not change which addresses running servers connect to.
**What it does:** Today a `setConfiguration` with the same IDs and priorities but different addresses gets a [success reply while the request is discarded](https://github.com/apache/ratis/blob/1c2f23488b1ef357ef4523823d201acd67f03406/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java#L1494-L1499), because `hasNoChange` compares only IDs and priorities. With the patch it is committed and written by every peer like any other configuration.
**What it does not do:** With gRPC, running servers keep their connections. `PeerProxyMap` [keeps the first address](https://github.com/apache/ratis/blob/1c2f23488b1ef357ef4523823d201acd67f03406/ratis-common/src/main/java/org/apache/ratis/util/PeerProxyMap.java#L131-L147) it has for a peer ID, and the lookups in `RaftConfigurationImpl`, the majority checks and the log appenders all go by peer ID. The recorded addresses are reported to clients, for example in GroupInfo. A server uses a recorded address only for a peer it has no address for yet, for example when it starts without peers in its group and reads them from Raft storage.
**Two addresses for the same peer:** I believe this can already happen on master. A request that changes membership or priority and also has a different address for an existing peer already creates a joint configuration with both addresses, since the new configuration is [built from the peers in the request](https://github.com/apache/ratis/blob/1c2f23488b1ef357ef4523823d201acd67f03406/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java#L534-L535). So the patch should not add a new state. It only stops skipping the case where nothing else changed.
I agree `raftMetaConf` is a reasonable option when a restart is needed anyway. I leaned towards `setConfiguration` for the Ozone case because Ozone SCM [starts with an empty group](https://github.com/apache/ozone/blob/d067ba38e7aea917db7efd290b4af53987ec985a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisServerImpl.java#L119-L123) and recovers its peers from Raft storage, so the stored addresses are what it uses after a restart. With `setConfiguration` that change is agreed by the group, instead of being edited by hand on each peer.
If it helps, I am happy to add tests for both behaviours: running servers keep working with the new addresses recorded, and servers restarted from storage use them. And if you still think the offline approach is the safer choice, I am fine with closing this and using `raftMetaConf` instead. Thanks for the careful review.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2082. setConfiguration should not ignore address changes of existing peers [ratis]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #1630:
URL: https://github.com/apache/ratis/pull/1630#discussion_r4223912931
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftConfigurationImpl.java:
##########
@@ -303,6 +303,13 @@ public String toString() {
}
boolean hasNoChange(Collection<RaftPeer> newMembers, Collection<RaftPeer> newListeners) {
+ return hasNoChange(newMembers, newListeners, false);
+ }
+
+ /**
+ * @param checkAddresses Should a peer with the same id but different addresses be considered as a change?
+ */
+ boolean hasNoChange(Collection<RaftPeer> newMembers, Collection<RaftPeer> newListeners, boolean checkAddresses) {
Review Comment:
Replaced the boolean overload with a named method, `hasNoChangeIncludingAddresses`. The flag is now private.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1492,7 +1492,7 @@ public CompletableFuture<RaftClientReply> setConfigurationAsync(SetConfiguration
}
// return success with a null message if the new conf is the same as the current
- if (current.hasNoChange(serversInNewConf, listenersInNewConf)) {
+ if (current.hasNoChange(serversInNewConf, listenersInNewConf, true)) {
Review Comment:
The call site now uses `hasNoChangeIncludingAddresses(...)` instead of a boolean literal.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java:
##########
@@ -663,6 +666,51 @@ void runTestNoChangeRequest(CLUSTER cluster) throws Exception {
}
}
+ /**
+ * When a request changes only the addresses of the existing peers,
+ * make sure the new configuration is committed and persisted.
+ */
+ @Test
+ public void testAddressChangeRequest() throws Exception {
+ runWithNewCluster(3, this::runTestAddressChangeRequest);
+ }
+
+ void runTestAddressChangeRequest(CLUSTER cluster) throws Exception {
+ final RaftServer.Division leader = RaftTestUtil.waitForLeader(cluster);
+ final long confIndex = leader.getRaftConf().getLogEntryIndex();
+
+ // the same peers with an equivalent address
+ final List<RaftPeer> newPeers = leader.getRaftConf().getCurrentPeers().stream()
+ .map(p -> RaftPeer.newBuilder(p).setAddress(p.getAddress().replace(NetUtils.LOCALHOST, "127.0.0.1")).build())
+ .collect(Collectors.toList());
Review Comment:
Added `TestRaftConfiguration.testHasNoChangeIncludingAddresses`, which covers each address field for members and listeners.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java:
##########
@@ -663,6 +666,51 @@ void runTestNoChangeRequest(CLUSTER cluster) throws Exception {
}
}
+ /**
+ * When a request changes only the addresses of the existing peers,
+ * make sure the new configuration is committed and persisted.
+ */
+ @Test
+ public void testAddressChangeRequest() throws Exception {
+ runWithNewCluster(3, this::runTestAddressChangeRequest);
+ }
+
+ void runTestAddressChangeRequest(CLUSTER cluster) throws Exception {
+ final RaftServer.Division leader = RaftTestUtil.waitForLeader(cluster);
+ final long confIndex = leader.getRaftConf().getLogEntryIndex();
+
+ // the same peers with an equivalent address
+ final List<RaftPeer> newPeers = leader.getRaftConf().getCurrentPeers().stream()
+ .map(p -> RaftPeer.newBuilder(p).setAddress(p.getAddress().replace(NetUtils.LOCALHOST, "127.0.0.1")).build())
+ .collect(Collectors.toList());
+ try (RaftClient client = cluster.createClient(leader.getId())) {
+ assertTrue(client.admin().setConfiguration(newPeers).isSuccess());
+ assertTrue(leader.getRaftConf().getLogEntryIndex() > confIndex);
+ assertAddresses(newPeers, cluster);
+
+ // the same request again has no change
+ final RaftConfiguration conf = leader.getRaftConf();
+ assertTrue(client.admin().setConfiguration(newPeers).isSuccess());
+ Assertions.assertSame(conf, leader.getRaftConf());
+ }
+
+ cluster.restart(false);
+ RaftTestUtil.waitForLeader(cluster);
+ assertAddresses(newPeers, cluster);
+ }
+
+ void assertAddresses(List<RaftPeer> expected, MiniRaftCluster cluster) throws Exception {
+ JavaUtils.attempt(() -> {
+ for (RaftServer.Division d : cluster.iterateDivisions()) {
+ final RaftConfiguration persisted = ((RaftStorageImpl) d.getRaftStorage()).readRaftConfiguration();
Review Comment:
Not changed. `ServerState` is typed to `RaftStorageImpl`, so the cast cannot fail in the test cluster.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2082. setConfiguration should not ignore address changes of existing peers [ratis]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #1630:
URL: https://github.com/apache/ratis/pull/1630#issuecomment-6184235040
Thanks @szetszwo for the feedback. Yeah let's try startup option route instead. Closing this one.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2082. setConfiguration should not ignore address changes of existing peers [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1630:
URL: https://github.com/apache/ratis/pull/1630#issuecomment-6067192109
@smengcl , Updating address in runtime is tricky in general. When the peers do not have the correct addresses at startup, how are they supposed to talk to each others?
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2082. setConfiguration should not ignore address changes of existing peers [ratis]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #1630:
URL: https://github.com/apache/ratis/pull/1630#issuecomment-6068637693
> @smengcl , Updating address in runtime is tricky in general. When the peers do not have the correct addresses at startup, how are they supposed to talk to each others?
@szetszwo Thanks for taking a look. The use case is the Ozone IPv6 work (HDDS-15763, design in apache/ozone#10981), which I should have mentioned earlier.
An existing SCM or OM HA group may have IPv4 literal peer addresses stored in the Raft log and `raft-meta.conf`. To move such a group to IPv6 without recreating it (HDDS-16136), the plan is:
1. Start IPv6 listeners on all peers while the IPv4 addresses stay reachable. The group keeps running on the old addresses.
2. Send one `setConfiguration` with the same peer IDs and the new IPv6 addresses, and wait for it to commit.
3. Update the Ozone configuration and restart the peers, then remove the IPv4 addresses.
Step 2 does not work today. The request returns success but nothing is written, so the IPv4 addresses stay in the stored configuration.
---
On your question, the peers always have working addresses at startup in this procedure, because the old addresses stay reachable until the new ones are committed and the peers have been restarted. This patch is not meant to repair a group whose peers cannot reach each other. Without a leader the request cannot be committed, and the offline `raftMetaConf` command is still the way to fix that.
The patch also does not change the connections of running servers. `PeerProxyMap` keeps the address it already has for a peer ID.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2082. setConfiguration should not ignore address changes of existing peers [ratis]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl closed pull request #1630: RATIS-2082. setConfiguration should not ignore address changes of existing peers
URL: https://github.com/apache/ratis/pull/1630
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2082. setConfiguration should not ignore address changes of existing peers [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1630:
URL: https://github.com/apache/ratis/pull/1630#issuecomment-6129233367
> ... this patch only changes what is recorded. It does not change which addresses running servers connect to.
This will confusing users: When calling setConf to change address, I guess users will have the expectation that everything will change to use the new address. i.e.
- The machine with the old address will restart the network server to use the new address, and
- all the connections will reconnect to the new 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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2082. setConfiguration should not ignore address changes of existing peers [ratis]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1630:
URL: https://github.com/apache/ratis/pull/1630#discussion_r4217743712
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftConfigurationImpl.java:
##########
@@ -303,6 +303,13 @@ public String toString() {
}
boolean hasNoChange(Collection<RaftPeer> newMembers, Collection<RaftPeer> newListeners) {
+ return hasNoChange(newMembers, newListeners, false);
+ }
+
+ /**
+ * @param checkAddresses Should a peer with the same id but different addresses be considered as a change?
+ */
+ boolean hasNoChange(Collection<RaftPeer> newMembers, Collection<RaftPeer> newListeners, boolean checkAddresses) {
Review Comment:
The new overload introduces a boolean parameter that changes semantic behavior, which can be error-prone at call sites. To make misuse less likely, prefer an explicit API shape (e.g., two clearly named methods or an enum/strategy parameter) so it’s impossible to accidentally invert the meaning.
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java:
##########
@@ -663,6 +666,51 @@ void runTestNoChangeRequest(CLUSTER cluster) throws Exception {
}
}
+ /**
+ * When a request changes only the addresses of the existing peers,
+ * make sure the new configuration is committed and persisted.
+ */
+ @Test
+ public void testAddressChangeRequest() throws Exception {
+ runWithNewCluster(3, this::runTestAddressChangeRequest);
+ }
+
+ void runTestAddressChangeRequest(CLUSTER cluster) throws Exception {
+ final RaftServer.Division leader = RaftTestUtil.waitForLeader(cluster);
+ final long confIndex = leader.getRaftConf().getLogEntryIndex();
+
+ // the same peers with an equivalent address
+ final List<RaftPeer> newPeers = leader.getRaftConf().getCurrentPeers().stream()
+ .map(p -> RaftPeer.newBuilder(p).setAddress(p.getAddress().replace(NetUtils.LOCALHOST, "127.0.0.1")).build())
+ .collect(Collectors.toList());
Review Comment:
The production change compares not only `getAddress()` but also `getAdminAddress()`, `getClientAddress()`, and `getDataStreamAddress()`. This test currently modifies only `setAddress(...)`, so regressions in the other address fields would not be caught. Consider extending this test (or adding a second case) that changes one of those other address fields to ensure the new comparison logic is fully covered.
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java:
##########
@@ -663,6 +666,51 @@ void runTestNoChangeRequest(CLUSTER cluster) throws Exception {
}
}
+ /**
+ * When a request changes only the addresses of the existing peers,
+ * make sure the new configuration is committed and persisted.
+ */
+ @Test
+ public void testAddressChangeRequest() throws Exception {
+ runWithNewCluster(3, this::runTestAddressChangeRequest);
+ }
+
+ void runTestAddressChangeRequest(CLUSTER cluster) throws Exception {
+ final RaftServer.Division leader = RaftTestUtil.waitForLeader(cluster);
+ final long confIndex = leader.getRaftConf().getLogEntryIndex();
+
+ // the same peers with an equivalent address
+ final List<RaftPeer> newPeers = leader.getRaftConf().getCurrentPeers().stream()
+ .map(p -> RaftPeer.newBuilder(p).setAddress(p.getAddress().replace(NetUtils.LOCALHOST, "127.0.0.1")).build())
+ .collect(Collectors.toList());
+ try (RaftClient client = cluster.createClient(leader.getId())) {
+ assertTrue(client.admin().setConfiguration(newPeers).isSuccess());
+ assertTrue(leader.getRaftConf().getLogEntryIndex() > confIndex);
+ assertAddresses(newPeers, cluster);
+
+ // the same request again has no change
+ final RaftConfiguration conf = leader.getRaftConf();
+ assertTrue(client.admin().setConfiguration(newPeers).isSuccess());
+ Assertions.assertSame(conf, leader.getRaftConf());
+ }
+
+ cluster.restart(false);
+ RaftTestUtil.waitForLeader(cluster);
+ assertAddresses(newPeers, cluster);
+ }
+
+ void assertAddresses(List<RaftPeer> expected, MiniRaftCluster cluster) throws Exception {
+ JavaUtils.attempt(() -> {
+ for (RaftServer.Division d : cluster.iterateDivisions()) {
+ final RaftConfiguration persisted = ((RaftStorageImpl) d.getRaftStorage()).readRaftConfiguration();
Review Comment:
This test hard-casts the storage to `RaftStorageImpl`, which tightly couples the test to a specific implementation and can break if `MiniRaftCluster` uses a different `RaftStorage` implementation in some environments. Prefer using an existing test utility (e.g., `RaftStorageTestUtils`) or reading the configuration via a public/abstracted API based on the storage directory instead of downcasting.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1492,7 +1492,7 @@ public CompletableFuture<RaftClientReply> setConfigurationAsync(SetConfiguration
}
// return success with a null message if the new conf is the same as the current
- if (current.hasNoChange(serversInNewConf, listenersInNewConf)) {
+ if (current.hasNoChange(serversInNewConf, listenersInNewConf, true)) {
Review Comment:
Passing a literal boolean here makes the intent easy to miss and creates a 'boolean trap'. Consider replacing the `true` with a named constant (e.g., `CHECK_ADDRESSES`) or using a dedicated method name (e.g., `hasNoChangeIncludingAddresses(...)`) so call sites are self-describing.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2082. setConfiguration should not ignore address changes of existing peers [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1630:
URL: https://github.com/apache/ratis/pull/1630#issuecomment-6085815599
> ... restart the peers ...
If restarting is needed, could we just use raftMetaConf command (RaftMetaConfCommand)?
Using setConf may have some unnecessary complication since there are two addresses for the same peer at the same time. I am not sure if the code can handle them correctly.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org