-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MINOR: Move Raft io thread implementation to Java #15119
MINOR: Move Raft io thread implementation to Java #15119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes.
try { | ||
super.shutdown(); | ||
} finally { | ||
client.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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
RaftIOThread
implementation into Java. I changed the name toKafkaRaftClientDriver
since 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)