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
KAFKA-14462; [13/N] CoordinatorEvent and CoordinatorEventProcessor #13666
Conversation
...p-coordinator/src/main/java/org/apache/kafka/coordinator/group/runtime/CoordinatorEvent.java
Show resolved
Hide resolved
...or/src/main/java/org/apache/kafka/coordinator/group/runtime/MultiThreadedEventProcessor.java
Show resolved
Hide resolved
...or/src/main/java/org/apache/kafka/coordinator/group/runtime/MultiThreadedEventProcessor.java
Outdated
Show resolved
Hide resolved
@kirktrue Thanks for your comments. I have addressed them. |
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 PR -- I mostly have nits and a few questions
...ator/src/main/java/org/apache/kafka/coordinator/group/runtime/CoordinatorEventProcessor.java
Outdated
Show resolved
Hide resolved
...or/src/main/java/org/apache/kafka/coordinator/group/runtime/MultiThreadedEventProcessor.java
Outdated
Show resolved
Hide resolved
...or/src/main/java/org/apache/kafka/coordinator/group/runtime/MultiThreadedEventProcessor.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/kafka/coordinator/group/runtime/MultiThreadedEventProcessorTest.java
Show resolved
Hide resolved
...rc/test/java/org/apache/kafka/coordinator/group/runtime/MultiThreadedEventProcessorTest.java
Show resolved
Hide resolved
...rc/test/java/org/apache/kafka/coordinator/group/runtime/MultiThreadedEventProcessorTest.java
Outdated
Show resolved
Hide resolved
...or/src/main/java/org/apache/kafka/coordinator/group/runtime/MultiThreadedEventProcessor.java
Show resolved
Hide resolved
...or/src/main/java/org/apache/kafka/coordinator/group/runtime/MultiThreadedEventProcessor.java
Show resolved
Hide resolved
...rc/test/java/org/apache/kafka/coordinator/group/runtime/MultiThreadedEventProcessorTest.java
Show resolved
Hide resolved
/** | ||
* A {{@link CoordinatorEvent}} processor. | ||
*/ | ||
public interface CoordinatorEventProcessor extends AutoCloseable { |
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.
Do we have any plans to make other coordinator event processors besides the multi-threaded one?
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.
Yep. I use a different implementation in unit tests (without threads) and I plan to use another one for simulation tests at some point.
...p-coordinator/src/main/java/org/apache/kafka/coordinator/group/runtime/CoordinatorEvent.java
Show resolved
Hide resolved
...ator/src/main/java/org/apache/kafka/coordinator/group/runtime/CoordinatorEventProcessor.java
Show resolved
Hide resolved
...rc/test/java/org/apache/kafka/coordinator/group/runtime/MultiThreadedEventProcessorTest.java
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.
thanks, LGTM!
*/ | ||
public synchronized void beginShutdown() { | ||
if (shuttingDown) { | ||
log.debug("Event processor is already shutting down."); |
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.
extremely minor nit that we may not even want to change -- but in theory, we could already be done "shutting down" right? Maybe not a huge complaint, but the log could be confusing if close() has already completed.
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.
That's right. I think that the aim of this log is to catch cases where this class would be shutdown more than once by mistake so it does not have to 100% correct.
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.
Left a small comment we don't need to necessarily change. Otherwise LGTM
Adds CoordinatorEvent, CoordinatorEventProcessor, and MultiThreadedEventProcessor.
Committer Checklist (excluded from commit message)