hachikuji commented on a change in pull request #9985:
URL: https://github.com/apache/kafka/pull/9985#discussion_r566373256
##########
File path: core/src/main/scala/kafka/raft/RaftManager.scala
##########
@@ -126,9 +126,13 @@ class KafkaRaftManager[T](
case spec: InetAddressSpec => {
netChannel.updateEndpoint(voterAddressEntry.getKey, spec)
}
+ case _: UnknownAddressSpec => {
Review comment:
nit: the braces in these `case` matches are not needed
##########
File path: raft/src/main/java/org/apache/kafka/raft/RaftConfig.java
##########
@@ -102,31 +120,23 @@ public boolean equals(Object obj) {
return false;
}
- final AddressSpec that = (AddressSpec) obj;
- return that.address().equals(address());
+ final InetAddressSpec that = (InetAddressSpec) obj;
+ return that.address.equals(address);
}
}
- public static class InetAddressSpec extends AddressSpec {
- private final InetSocketAddress address;
-
- public InetAddressSpec(InetSocketAddress address) {
- if (address.equals(UNROUTABLE_ADDRESS)) {
- throw new IllegalArgumentException("Address not routable");
- }
- this.address = address;
+ public static class UnknownAddressSpec implements AddressSpec {
+ private UnknownAddressSpec() {
Review comment:
Since we have a private constructor, I think we will only have the
`UNKNOWN_ADDRESS_SPEC_INSTANCE` instance. So can we rely on the default
equals/hashcode?
##########
File path: raft/src/main/java/org/apache/kafka/raft/RaftConfig.java
##########
@@ -89,8 +92,23 @@
private final int appendLingerMs;
private final Map<Integer, AddressSpec> voterConnections;
- public static abstract class AddressSpec {
- public abstract InetSocketAddress address();
+ public interface AddressSpec {
+ }
+
+ public static class InetAddressSpec implements AddressSpec {
+ public final InetSocketAddress address;
+
+ public InetAddressSpec(InetSocketAddress address) {
+ if (address.equals(NON_ROUTABLE_ADDRESS)) {
Review comment:
Maybe worth adding a null check here?
##########
File path: core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala
##########
@@ -1001,6 +1001,12 @@ class KafkaConfigTest {
assertEquals(expectedVoters, raftConfig.quorumVoterConnections())
}
+ @Test
+ def testNonRoutableAddressSpecException(): Unit = {
+ assertThrows(classOf[IllegalArgumentException],
+ () => new InetAddressSpec(new InetSocketAddress("0.0.0.0", 0)))
Review comment:
nit: I still think `RaftConfigTest` is a better home for this. It
doesn't involve `KafkaConfig` at all. We could also just delete the test since
it probably isn't buying us much.
----------------------------------------------------------------
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
