dajac commented on code in PR #14705:
URL: https://github.com/apache/kafka/pull/14705#discussion_r1388535147
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/runtime/CoordinatorRuntime.java:
##########
@@ -659,9 +660,21 @@ public TopicPartition key() {
*/
@Override
public void run() {
+ try {
+ runAsync().get();
Review Comment:
Thanks for looking into this. Here is my take:
> That is correct, it may become a perf problem
I strongly disagree on blocking the event loop. It will not become a perf
problem. It is one. It is also an anti-pattern.
> Right now it is a functional problem
It is technically not a functional problem, at least not yet, because I
haven't not implemented the transactional offset commit in the new coordinator.
;)
> appendRecords has async interface, thus adding async stages under such an
interface can be done without inspection and understanding all callers (that's
what an interface is -- any compliant implementation is valid), but doing so
will break the current logic (so from the proper interface usage perspective it
is a bug in the caller, which this proposal fixes)
I will change this to not use appendRecords, this will make the contract
clear.
> now all of a sudden KIP-848 got a new work to do before release, just
because there is some independent work is going on in transaction area
This is incorrect. We knew about this and we always had an implementation in
mind which works. I will basically decouple the write in two stages: 1)
validate/prepare the transaction; and 2) update state and write. As we
discussed in the other PR, this is also required for the old coordinator to
work correctly.
> KIP-890 part2 design is still under discussion, the verification protocol
is likely to change, so any changes in KIP-890 protocol are going to have
ripple effects on KIP-848
I don't agree with this. As we just saw, we already failed to make it work
correctly for the existing coordinator so the dependency was already there.
Again, we can do better, I agree.
> the work needs to be duplicated in group coordinator (and the protocol is
going slightly different for different client versions) which becomes a likely
source of bugs
This is completely unrelated in my opinion as this is true for both the old
and the new coordinator.
Overall, I agree that we could do better but I think that it is not the
right time to change this. We are already under high time pressure and actually
changing this in the right way puts even more pressure. We should look for a
proper solution afterwards.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
