MINOR: fix flaky testRecordThreadIdleRatio#15987
Conversation
| synchronized (recordedIdleTimesMs) { | ||
| recordedIdleTimesMs.add(threadIdleTime); | ||
| } | ||
| recordedIdleTimesMs.add(threadIdleTime); |
There was a problem hiding this comment.
Nit: might be useful to add a comment that this is safe because numThreads in line 470 is 1.
| public CoordinatorEvent take() { | ||
| time.sleep(takeDelayMs); | ||
| return super.take(); | ||
| CoordinatorEvent event = super.take(); |
There was a problem hiding this comment.
IIUC, the fix suggests the flakyness is because super.take() returns null spuriously. I don't quite follow why that would happen though. Javadoc for EventAccumulator::take mentions it returns null only when the accumulator is closed. We also assert the value captured within doAnswer is 100, recordedIdleTimesMs.size() == 8 and mockRuntimeMetrics.recordThreadIdleTime() is invoked 8 times so where is the extra invocation coming from?
There was a problem hiding this comment.
That's correct and was my initial confusion as well. The event processor shuts down at the end of the test so we have the processor thread still running when we perform the assertions.
When we call
long diff = time.milliseconds() - startMs;
we may be at the point when we process the 8 events (diff == 800ms) or when the thread does another iteration over handleEvents(), which does another time.sleep() (diff == 900ms). This was causing the flakiness.
There was a problem hiding this comment.
we may be at the point when we process the 8 events (diff == 800ms) or when the thread does another iteration over handleEvents(), which does another time.sleep() (diff == 900ms). This was causing the flakiness.
Sorry, I'm still confused. Why would another iteration over handleEvents() invoke the doAnswer? In other words, shouldn't accumulator.take() block since there are no more events? I'd imagine EventAccumulator::take to block on condition.await() after 8 events have been processed.
There was a problem hiding this comment.
Nvm, I see your point - accumulator.take() may be invoked - the block on the delegate may happen after the timer has been incremented.
There was a problem hiding this comment.
IIUC, it's the ordering that matters so we can avoid the null check and increment time only after super.take() returns.
CoordinatorEvent event = super.take(); // blocks until there are new events
time.sleep(takeDelayMs); // only increment timer after we get unblocked
return event;There was a problem hiding this comment.
@gaurav-narula thanks for the feedback. that makes sense, i have incorporated your suggestion
gaurav-narula
left a comment
gaurav-narula
left a comment
There was a problem hiding this comment.
LGTM! Thanks for fixing this 🙏
dajac
left a comment
dajac
left a comment
There was a problem hiding this comment.
LGTM. I just triggered a new build as the last one failed.
|
I will merge it after QA complete |
commit 93238ae Author: Antoine Pourchet <antoine@responsive.dev> Date: Thu May 23 13:45:29 2024 -0600 KAFKA-15045: (KIP-924 pt. 10) Topic partition rack annotation simplified (apache#16034) This PR uses the new TaskTopicPartition structure to simplify the build process for the ApplicationState, which is the input to the new TaskAssignor#assign call. Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org> commit 4020307 Author: Kuan-Po (Cooper) Tseng <brandboat@gmail.com> Date: Fri May 24 02:51:26 2024 +0800 KAFKA-16795 Fix broken compatibility in kafka.tools.NoOpMessageFormatter, kafka.tools.DefaultMessageFormatter, and kafka.tools.LoggingMessageFormatter (apache#16020) This commit allows users to apply the scala version Formatters, but users will receive the warning messages about deprecation. This compatibility support will be removed from 4.0.0 Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit c3018ef Author: TingIāu "Ting" Kì <51072200+frankvicky@users.noreply.github.com> Date: Fri May 24 01:15:56 2024 +0800 KAFKA-16804: Replace archivesBaseName with archivesName (apache#16016) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Greg Harris <greg.harris@aiven.io> commit 0ba15ad Author: Edoardo Comar <ecomar@uk.ibm.com> Date: Thu May 23 17:17:56 2024 +0100 KAFKA-15905 Restarts of MirrorCheckpointTask should not permanently i… (apache#15910) * KAFKA-15905 Restarts of MirrorCheckpointTask should not permanently interrupt offset translation MirrorCheckpointTask reloads the last checkpoint at start, OffsetSyncStore stores OffsetSyncs before reading till end. If CheckpointTask cannot read checkpoints at startup, use previous OffsetSyncStore load logic, with warning log message about degraded offset translation. Also addresses KAFKA-16622 Mirromaker2 first Checkpoint not emitted until consumer group fully catches up once because the OffsetSyncStore store is populated before reading to log end. Co-Authored-By: Adrian Preston <prestona@uk.ibm.com> Reviewers: Greg Harris <greg.harris@aiven.io> commit 5a48984 Author: Viktor Somogyi-Vass <viktorsomogyi@gmail.com> Date: Thu May 23 17:36:39 2024 +0200 KAFKA-15649: Handle directory failure timeout (apache#15697) A broker that is unable to communicate with the controller will shut down after the configurable log.dir.failure.timeout.ms. The implementation adds a new event to the Kafka EventQueue. This event is deferred by the configured timeout and will execute the shutdown if the heartbeat communication containing the failed log dir is still pending with the controller. Reviewers: Igor Soarez <soarez@apple.com> commit 8d117a1 Author: Mickael Maison <mimaison@users.noreply.github.com> Date: Thu May 23 17:03:24 2024 +0200 KAFKA-16825: Update netty/jetty/jackson/zstd dependencies (apache#16038) Reviewers: Luke Chen <showuon@gmail.com> commit ab0cc72 Author: Mickael Maison <mimaison@users.noreply.github.com> Date: Thu May 23 16:01:45 2024 +0200 MINOR: Move parseCsvList to server-common (apache#16029) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 14b5c4d Author: Dongnuo Lyu <139248811+dongnuo123@users.noreply.github.com> Date: Thu May 23 02:27:00 2024 -0400 KAFKA-16793; Heartbeat API for upgrading ConsumerGroup (apache#15988) This patch implements the heartbeat api to the members that use the classic protocol in a ConsumerGroup. Reviewers: Jeff Kim <jeff.kim@confluent.io>, David Jacot <djacot@confluent.io> commit e692fee Author: Jeff Kim <kimkb2011@gmail.com> Date: Thu May 23 02:24:23 2024 -0400 MINOR: fix flaky testRecordThreadIdleRatio (apache#15987) DelayEventAccumulator should return immediately if there are no events in the queue. Also removed some unused fields inside EventProcessorThread. Reviewers: Gaurav Narula <gaurav_narula2@apple.com>, Chia-Ping Tsai <chia7712@gmail.com>, David Jacot <djacot@confluent.io> commit bef83ce Author: Nick Telford <nick.telford@gmail.com> Date: Thu May 23 05:34:31 2024 +0100 KAFKA-15541: Add iterator-duration metrics (apache#16028) Part of [KIP-989](https://cwiki.apache.org/confluence/x/9KCzDw). This new `StateStore` metric tracks the average and maximum amount of time between creating and closing Iterators. Iterators with very high durations can indicate to users performance problems that should be addressed. If a store reports no data for these metrics, despite the user opening Iterators on the store, it suggests those iterators are not being closed, and have therefore leaked. Reviewers: Matthias J. Sax <matthias@confluent.io>
DelayEventAccumulator should return immediately if there are no events in the queue. Also removed some unused fields inside EventProcessorThread. Reviewers: Gaurav Narula <gaurav_narula2@apple.com>, Chia-Ping Tsai <chia7712@gmail.com>, David Jacot <djacot@confluent.io>
DelayEventAccumulator should return immediately if there are no events in the queue. Also removed some unused fields inside EventProcessorThread. Reviewers: Gaurav Narula <gaurav_narula2@apple.com>, Chia-Ping Tsai <chia7712@gmail.com>, David Jacot <djacot@confluent.io>
DelayEventAccumulator should return immediately if there are no events in the queue. Also removed some unused fields inside EventProcessorThread. Reviewers: Gaurav Narula <gaurav_narula2@apple.com>, Chia-Ping Tsai <chia7712@gmail.com>, David Jacot <djacot@confluent.io>
4 participants
DelayEventAccumulator should return immediately if there are no events in the queue. Also removed some unused fields inside EventProcessorThread
Committer Checklist (excluded from commit message)