hachikuji commented on a change in pull request #9985: URL: https://github.com/apache/kafka/pull/9985#discussion_r565634630
########## File path: core/src/main/scala/kafka/raft/KafkaNetworkChannel.scala ########## @@ -34,6 +34,7 @@ import scala.collection.mutable object KafkaNetworkChannel { + val nonRoutableAddress = new InetSocketAddress("0.0.0.0", 0) Review comment: Let me suggest an alternative for the sake of argument. Currently, `RaftConfig.parseVoterConnections` return `Map<Integer, InetSocketAddress>`. This works for the case we're interested in, but there is a risk of our sentinel non-routable address leaking into unexpected cases (a common source of bugs in Kafkaland). Alternatively, what if we add something like this to `RaftConfig`: ```java public class RaftConfig { ... public Map<Integer, AddressSpec> quorumVoterConnections(); public static interface AddressSpec { } public static class InetAddressSpec implements AddressSpec { final InetSocketAddress address; } public static class UnknownAddressSpec implements AddressSpec { } } ``` The advantage is that this lets the type checker help us ensure that we are checking for a sentinel. ########## File path: raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java ########## @@ -208,8 +209,9 @@ public KafkaRaftClient( int fetchMaxWaitMs, OptionalInt nodeId, LogContext logContext, - Random random - ) { + Random random, + RaftConfig raftConfig + ) throws IOException { Review comment: Is anything in here throwing `IOException`? ########## File path: raft/src/test/java/org/apache/kafka/raft/MockNetworkChannel.java ########## @@ -25,20 +25,25 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Function; +import java.util.stream.Collectors; public class MockNetworkChannel implements NetworkChannel { private final AtomicInteger correlationIdCounter; + private final Map<Integer, InetSocketAddress> addressCache; Review comment: I don't think we are using the address here. Can we use `Set<Integer>`? Potentially we could even get rid of this collection. It was more useful when the RaftClient itself was expected to discover the voter endpoints. ########## File path: raft/src/main/java/org/apache/kafka/raft/RaftConfig.java ########## @@ -36,7 +36,9 @@ public static final String QUORUM_VOTERS_CONFIG = QUORUM_PREFIX + "voters"; public static final String QUORUM_VOTERS_DOC = "Map of id/endpoint information for " + "the set of voters in a comma-separated list of `{id}@{host}:{port}` entries. " + - "For example: `1@localhost:9092,2@localhost:9093,3@localhost:9094`"; + "For example: `1@localhost:9092,2@localhost:9093,3@localhost:9094.`" + + "If the voter endpoints are not known at startup, a non-routable address can be provided instead." + Review comment: Perhaps we can keep this as an internal feature for now. It is not something that a user would be able to leverage. We can document it in the class javadoc perhaps. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org