KAFKA-14208: Should not wake-up with non-blocking coordinator discovery

[guozhangwang]

Today we may try to discover coordinator in both blocking (e.g. in poll) and non-blocking (e.g. in commitAsync) way. For the latter we would poll the underlying network client with timeout 0, and in this case we should not trigger wakeup since these are non-blocking calls and hence should not throw wake-ups.

In this PR I'm trying to fix it in a least intrusive way (a more general fix should be, potentially, to have two versions of ensureCoordinatorReady), since in our threading refactoring, the ensureCoordinatorReady function would not be called by the calling thread any more and only triggered by the background thread, and hence we would have a much simpler manner to ensure that non-blocking functions never throw wake-ups.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[guozhangwang]

Call @philipnee @showuon for reviews.

[hachikuji]

I wonder if this breaks the API in a different way. If the user calls wakeup(), would we expect the next blocking call to trigger a wakeup even if it were called with a timeout of zero? I wonder if it would be better to overload ensureCoordinatorReady with an additional flag?

[showuon]

would we expect the next blocking call to trigger a wakeup even if it were called with a timeout of zero?

Yes, I think as long as it's a blocking call, the wakeup exception should be thrown, even if it's zero timeout. I've checked existing javadoc in KafkaConsumer, all the methods, which declared wakeupException will be thrown, will still throw wakeupExceptions after this change. So, I think this PR change (1) won't break API, and (2) fixes the issue for commitAsync, which makes sense to me.

I wonder if it would be better to overload ensureCoordinatorReady with an additional flag?

Yes, as @guozhangwang mentioned in PR description:

In this PR I'm trying to fix it in a least intrusive way (a more general fix should be, potentially, to have two versions of ensureCoordinatorReady)

I think this fix is safer. But I don't have strong opinion about it, just want to raise the release timing issue here.

[hachikuji]

Yeah, I feel it's a bit slippery to leave the logic in place with just a TODO somewhere to go back and fix it. Too many times, the follow-up never happens. Is it really that much additional effort to add an overload?

protected synchronized boolean ensureCoordinatorReady(final Timer timer) {
  return ensureCoordinatorReady(timer, true);
}

private synchronized boolean ensureCoordinatorReady(final Timer timer, boolean checkWakeup) {
...

[showuon]

The change makes sense to me. Thanks.

[showuon]

would we expect the next blocking call to trigger a wakeup even if it were called with a timeout of zero?

Yes, I think as long as it's a blocking call, the wakeup exception should be thrown, even if it's zero timeout. I've checked existing javadoc in KafkaConsumer, all the methods, which declared wakeupException will be thrown, will still throw wakeupExceptions after this change. So, I think this PR change (1) won't break API, and (2) fixes the issue for commitAsync, which makes sense to me.

I wonder if it would be better to overload ensureCoordinatorReady with an additional flag?

Yes, as @guozhangwang mentioned in PR description:

In this PR I'm trying to fix it in a least intrusive way (a more general fix should be, potentially, to have two versions of ensureCoordinatorReady)

I think this fix is safer. But I don't have strong opinion about it, just want to raise the release timing issue here.

[showuon]

nit:

assertThrows(WakeupException.class, 
() -> coordinator.joinGroupIfNeeded(mockTime.timer(0)), 
"Should have woken up from joinGroupIfNeeded()")

[showuon]

nit: can be replaced with assertThrows

[hachikuji]

@showuon Since Guozhang is out sick, I raised #12626 which addresses your (and my) comments. Please take a look. I'll leave this open for now.

[hachikuji]

I'll go ahead and close this since we're going to merge #12626.