MINOR: Move Raft io thread implementation to Java #15119
Conversation
| try { | ||
| super.shutdown(); | ||
| } finally { | ||
| client.close(); |
There was a problem hiding this comment.
Should this type close the KafkaRaftClient? In the raft module we generally establish close/shutdown ownership based on if the type created the object or if it is was passed in through a function/constructor.
Take a look at KafkaRaftClient for an example. KafkaRaftClient doesn't close ReplicatedLog, NetworkChannel, etc because they were passed through the constructor. In the other hand KafkaRaftClient does close kafkaRaftMetrics and memoryPool because they were created by KafkaRaftClient.
There was a problem hiding this comment.
Yes, I debated it. It is nice to have the driver own shutdown since it is in control of the poll loop. Given that, it seemed safer for it to also close the client once it knows that shutdown is complete.
In a future patch, my thought was to have the driver own creation of the client as well. We can replace RaftManager with a builder which constructs the client in the context of the driver. That would establish clear ownership and resolve the issue.
There was a problem hiding this comment.
Okay. How about documenting this decision in the type's Java Doc and the private field KafkaRaftClient? That way future readers understand that this is the exception and not the rule.
There was a problem hiding this comment.
I added some comments in the latest commit. Let me know if they address the comment.
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClientDriver.java
Outdated
Show resolved
Hide resolved
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientDriverTest.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClientDriver.java
Outdated
Show resolved
Hide resolved
This patch moves the `RaftIOThread` implementation into Java. I changed the name to `KafkaRaftClientDriver` since the main thing it does is drive the calls to `poll()`. There shouldn't be any changes to the logic. Reviewers: José Armando García Sancio <[email protected]>
This patch moves the `RaftIOThread` implementation into Java. I changed the name to `KafkaRaftClientDriver` since the main thing it does is drive the calls to `poll()`. There shouldn't be any changes to the logic. Reviewers: José Armando García Sancio <[email protected]>
This patch moves the `RaftIOThread` implementation into Java. I changed the name to `KafkaRaftClientDriver` since the main thing it does is drive the calls to `poll()`. There shouldn't be any changes to the logic. Reviewers: José Armando García Sancio <[email protected]>
This patch moves the `RaftIOThread` implementation into Java. I changed the name to `KafkaRaftClientDriver` since the main thing it does is drive the calls to `poll()`. There shouldn't be any changes to the logic. Reviewers: José Armando García Sancio <[email protected]>
This patch moves the
RaftIOThreadimplementation into Java. I changed the name toKafkaRaftClientDriversince the main thing it does is drive the calls topoll(). There shouldn't be any changes to the logic.Committer Checklist (excluded from commit message)