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
