KAFKA-17779: Fix flaky RemoteLogManager test#17724
Merged
Merged
Conversation
Contributor
Author
Contributor
|
The patch LGTM, will wait for the CI to complete. |
Contributor
Author
Thanks for the help. Looks like CI successfully completed! |
kamalcph
approved these changes
Nov 10, 2024
kamalcph
left a comment
Contributor
There was a problem hiding this comment.
Thanks @wperlichek for refactoring the test!
chiacyu
pushed a commit
to chiacyu/kafka
that referenced
this pull request
Nov 30, 2024
Reviewers: Kamal Chandraprakash <kamal.chandraprakash@gmail.com>
eduwercamacaro
added a commit
to littlehorse-enterprises/kafka
that referenced
this pull request
Dec 17, 2024
* KAFKA-17926 Improve the documentation explaining why max.in.flight.requests.per.connection should not exceed 5 (apache#17719) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> * MINOR: Cleanup GroupCoordinatorRecordHelpers (apache#17718) Reviewers: Jeff Kim <jeff.kim@confluent.io>, Mickael Maison <mickael.maison@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com> * rebase to fix merge conflict (apache#17702) Fixes an issue with the TTD in the specific case where users don't specify an initial time for the driver and also don't specify a start timestamp for the TestInputTopic, then pipe input records without timestamps. This combination results in a slight mismatch in the expected timestamps for the piped records, which can be noticeable when writing tests with very small time deltas. The problem is that, while both the TTD and the TestInputTopic will be initialized to the "current time" when not otherwise specified, it's possible for some milliseconds to have passed between the creation of the TTD and the creation of the TestInputTopic. This can result in a TestInputTopic getting a start timestamp that's several ms larger than the driver's time, and ultimately causing the piped input records to have timestamps slightly in the future relative to the driver. In practice even those who hit this issue might not notice it if they aren't manipulating time in their tests, or are advancing time by enough to negate the several-milliseconds of difference. However we noticed a test fail due to this because we were testing a ttl-based processor and had advanced the driver time by only 1 millisecond past the ttl. The piped record should have been expired, but because it's timestamp was a few milliseconds longer than the driver's start time, this test ended up failing. Reviewers: Matthias Sax <mjsax@apache.org>, Bruno Cadonna <cadonna@apache.org>, Lucas Brutschy < lbrutschy@confluent.io> * KAFKA-17801: RemoteLogManager may compute inaccurate upperBoundOffset for aborted txns (apache#17676) Reviewers: Jun Rao <junrao@gmail.com> * MINOR: log fix in SnapshottableCoordinator (apache#17726) Reviewers: donaldzhu-cc, Chia-Ping Tsai <chia7712@gmail.com> * KAFKA-17570 Rewrite StressTestLog by Java (apache#17249) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> * MINOR: Delete unused member from KafkaAdminClient (apache#17729) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> * KAFKA-17970 Moving some share purgatory classes from core to share module (apache#17722) As part of PR: apache#17636 where purgatory has been moved from core to server-common hence move some existing classes used in Share Fetch to Share module. Reviewers: Chia-Ping Tsai <chia7712@gmail.com> * KAFKA-17837 Rewrite DeleteTopicTest (apache#17579) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> * MINOR: Move StopPartition to server-common (apache#17704) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> * KAFKA-15549 Bump swagger dependency version from 2.2.8 to 2.2.25 (apache#17730) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> * KAFKA-17872: Update consumed offsets on records with invalid timestamp (apache#17710) TimestampExtractor allows to drop records by returning a timestamp of -1. For this case, we still need to update consumed offsets to allows us to commit progress. Reviewers: Bill Bejeck <bill@confluent.io> * KAFKA-17925 Convert Kafka Client integration tests to use KRaft (apache#17670) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> * KAFKA-17779: Fix flaky RemoteLogManager test (apache#17724) Reviewers: Kamal Chandraprakash <kamal.chandraprakash@gmail.com> * KAFKA-17455: fixes stuck producer by polling again after pollDelayMs in NetworkUtils#awaitReady() * clarifies comments * attempts to add test * Adds a test but my changes to MockClient.java broke all sorts of stuff * test that passes on my branch and fails on trunk * addresses PR feedback: rename MockClient#setAdvanceTimeDuringPoll to advanceTimeDuringPoll() --------- Co-authored-by: PoAn Yang <payang@apache.org> Co-authored-by: David Jacot <djacot@confluent.io> Co-authored-by: A. Sophie Blee-Goldman <ableegoldman@gmail.com> Co-authored-by: Kamal Chandraprakash <kamal.chandraprakash@gmail.com> Co-authored-by: Jeff Kim <kimkb2011@gmail.com> Co-authored-by: TengYao Chi <kitingiao@gmail.com> Co-authored-by: Apoorv Mittal <apoorvmittal10@gmail.com> Co-authored-by: Mickael Maison <mimaison@users.noreply.github.com> Co-authored-by: Yung <yungyung7654321@gmail.com> Co-authored-by: Matthias J. Sax <matthias@confluent.io> Co-authored-by: Kirk True <kirk@kirktrue.pro> Co-authored-by: wperlichek <61857706+wperlichek@users.noreply.github.com> Co-authored-by: Colt McNealy <colt@littlehorse.io>
tedyu
pushed a commit
to tedyu/kafka
that referenced
this pull request
Jan 6, 2025
Reviewers: Kamal Chandraprakash <kamal.chandraprakash@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This test appears to be flaky because we mutated and reused the mocked log segments. Instead of using
thenReturn()and changing return value of an existing mock, or mutating the log segments within an existing mock return value, we can establish states we will need for the test and then conditionally return them usingthenAnswer()to ensure consistent/reliable mock behavior.I ran the whole test class 100 times locally using below command and saw 100% success rate. (Had to remove
@Flakyannotation locally and the import, assuming we don't want to remove the annotation in this PR until we see definitive proof this fixed the flakiness in healthy sample of subsequent builds.)N=100; I=0; while [ $I -lt $N ]; do ./gradlew core:test --tests "kafka.log.remote.RemoteLogManagerTest" --rerun --fail-fast; (( I=$I+1 )); echo "Completed run: $I"; sleep 1; doneUpdate: Ran
./gradlew checkstyleMain checkstyleTest spotlessChecklocally, success.Resources for others interested in learning about tiered storage:
Deep dive: https://developers.redhat.com/articles/2024/03/13/kafka-tiered-storage-deep-dive
KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-405%3A+Kafka+Tiered+Storage
@kamalcph @chia7712 FYI
Committer Checklist (excluded from commit message)