KAFKA-17837: Rewrite DeleteTopicTest#17579
Merged
Merged
Conversation
chia7712
reviewed
Oct 26, 2024
chia7712
left a comment
Member
There was a problem hiding this comment.
@frankvicky thanks for this patch
| RecordBatch.NO_PARTITION_LEADER_EPOCH); | ||
| } | ||
|
|
||
| public static <B extends KafkaBroker> void verifyTopicDeletion(String topic, |
Member
There was a problem hiding this comment.
Could you move this method to ClusterInstance?
| String topic, | ||
| int partitionNumber, | ||
| long timeoutMs, | ||
| Optional<Integer> oldLeaderOpt, |
Member
There was a problem hiding this comment.
oldLeaderOpt and newLeaderOpt are never assigned
TaiJuWu
reviewed
Oct 31, 2024
| .findFirst(); | ||
| } | ||
|
|
||
| private KafkaBroker findFollower(Collection<KafkaBroker> idToBroker, int leaderId) { |
Collaborator
There was a problem hiding this comment.
How about to rename findAnyFollower?
Contributor
Author
There was a problem hiding this comment.
I think current name is clear enough.
Contributor
Author
|
The fail test is handling by #17645 |
chia7712
reviewed
Nov 4, 2024
| verifyTopicDeletion(topic, numPartitions, brokers().values()); | ||
| } | ||
|
|
||
| default <B extends KafkaBroker> void verifyTopicDeletion(String topic, int numPartitions, Collection<B> brokers) throws Exception { |
Member
There was a problem hiding this comment.
Could you please rename it to waitForTopicDeletion?
Additionally, could you please don't expose KafkaBroker? we will cleanup all inner interface later and that should not be used by tests anymore.
TaiJuWu
reviewed
Nov 5, 2024
chia7712
reviewed
Nov 7, 2024
chia7712
reviewed
Nov 8, 2024
chia7712
left a comment
Member
There was a problem hiding this comment.
@frankvicky thanks for this patch!
| .orElse(null); | ||
| } | ||
|
|
||
| private static int doWaitUntilLeaderIsElectedOrChanged(GetPartitionLeader getPartitionLeader, |
Member
There was a problem hiding this comment.
Could you please replace GetPartitionLeader by BiFunction<String, Integer, Optional<Integer>>?
| + " ms since a leader was not elected for partition " + topicPartition + ", leader is " + finalLeader)); | ||
| } | ||
|
|
||
| public static <B extends KafkaBroker> Map<TopicPartition, UpdateMetadataPartitionState> waitForAllPartitionsMetadata( |
Member
There was a problem hiding this comment.
Can this be replaced by ClusterInstance#waitForTopic?
| } | ||
| } | ||
|
|
||
| default void waitForTopicDeletion(String topic, int numPartitions) throws Exception { |
Member
There was a problem hiding this comment.
Could you please move all checks to ClusterInstance#waitForTopic? those checks should be enabled when the input partitions is equal to 0
| return records(Collections.singletonList(new SimpleRecord(timestamp, key, value)), magicValue, codec); | ||
| } | ||
|
|
||
| public static MemoryRecords singletonRecords(byte[] value, byte[] key) { |
Member
There was a problem hiding this comment.
this method is equal to MemoryRecords.withRecords(Compression.NONE, new SimpleRecord(key, value));, so we don't need to add this helper, right?
| } | ||
| } | ||
|
|
||
| public static MemoryRecords records(List<SimpleRecord> records, byte magicValue, Compression codec) { |
e5d9abb to
e563194
Compare
chiacyu
pushed a commit
to chiacyu/kafka
that referenced
this pull request
Nov 30, 2024
Reviewers: Chia-Ping Tsai <chia7712@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: Chia-Ping Tsai <chia7712@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
3 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.
JIRA: KAFKA-17837
Rewrite DeleteTopicTest as Java.
Committer Checklist (excluded from commit message)