KAFKA-17925: Convert Kafka Client integration tests to use KRaft by kirktrue · Pull Request #17670 · apache/kafka

kirktrue · 2024-11-02T23:13:49Z

Update pertinent integration tests to use KRaft and not Zookeeper.

Committer Checklist (excluded from commit message)

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

Update pertinent integration tests to use KRaft and not Zookeeper.

chia7712 · 2024-11-03T09:04:44Z

it seems there are some failed tests caused by this PR

…on-tests

Update to only test CLASSIC group protocol

…on-tests

chia7712

@kirktrue thanks for this patch. overall LGTM

chia7712 · 2024-11-08T22:02:06Z

+    val gp = maybeGroupProtocolSpecified()
+
+    if (gp.isEmpty)
+      throw new IllegalStateException("Please specify the group.protocol configuration when creating a KafkaConsumer")


maybeGroupProtocolSpecified is based on the input arguments of test case. Maybe the error message should be "Please specify the groupProtocol=consumer when writing the test"

I updated the error message to Please specify the "groupProtocol" parameter when writing the test. I didn't use groupProtocol=consumer in the error message because the value of groupProtocol can be classic or consumer.

PTAL. Thanks!

chia7712 · 2024-11-08T22:54:17Z

-    def testTopicIdIsRemovedFromFetcherWhenControllerDowngrades(): Unit = {
+    @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumAndGroupProtocolNames)
+    @MethodSource(Array("getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly_ZK_implicit"))
+    def testTopicIdIsRemovedFromFetcherWhenControllerDowngrades(quorum: String, groupProtocol: String): Unit = {


This test case is for the ZK-only API (LeaderAndIsrRequest). Not sure if we plan to remove all ZK-only APIs in 4.0... cc @dajac.

There are around 20 integration tests that are effectively Zookeeper-only. In some cases, nothing in the test explicitly calls that dependency out. I introduced the @MethodSource getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly_ZK_implicit() which includes the test parameters quorum=zk and groupProtocol=classic. The idea is that someone more knowledgeable about the tests would review these and either a) remove the test, or b) update the test to run against Kraft, if possible.

The idea is that someone more knowledgeable about the tests would review these and either a) remove the test, or b) update the test to run against Kraft, if possible.

Yes, that will be a great reference, similar to #17727.

For example, FetchRequestTestDowngrade is for LeaderAndIsrRequest, which is ZK-only, so it's okay to remove it in a follow-up (https://issues.apache.org/jira/browse/KAFKA-17976).

chia7712 · 2024-11-09T20:13:40Z

@kirktrue Could you please fix the conflicts and comment (#17670 (comment))? I'd like to merge this PR

…on-tests

chia7712 · 2024-11-10T02:37:42Z

SaslPlainPlaintextConsumerTest, ConsumerBounceTest, TransactionsExpirationTest they pass on my local

lianetm · 2024-11-15T18:34:29Z

Hey folks, I noticed TransactionsExpirationTest.testTransactionAfterProducerIdExpires has become flaky on trunk recently, only for 2 combinations that I guess were introduced here? That test has been flaky before and fixed with KAFKA-14534
Should we reopen the issue o create a new one? Wonder if the context on that one might help.

https://ge.apache.org/scans/tests?search.buildOutcome=failure&search.relativeStartTime=P28D&search.rootProjectNames=kafka&search.tags=trunk&search.timeZoneId=America%2FToronto&tests.container=kafka.api.TransactionsExpirationTest&tests.test=testTransactionAfterProducerIdExpires(String%2C%20String%2C%20boolean)%5B3%5D

jolshan · 2024-11-15T22:17:55Z

@lianetm we were just looking at this from our end. I think it depends on what is failing. Looking more closely into this now.

jolshan · 2024-11-15T22:19:01Z

As an aside, if we introduce a new test and/or version of a test and we see it fail in the build, we should not merge the PR.
Even if it passes locally, we need to look at if it is flaky due to the change.
I see it failing here: https://github.com/apache/kafka/actions/runs/11760691711/job/32761651896

jolshan · 2024-11-15T22:48:46Z

Found the issue and will open a fix shortly -- it was on the transactions side and not the group coordinator side. :)

EDIT: PR here: #17831

lianetm · 2024-11-18T13:58:05Z

Great, thanks @jolshan!

chia7712 · 2024-11-22T18:07:47Z

As an aside, if we introduce a new test and/or version of a test and we see it fail in the build, we should not merge the PR.
Even if it passes locally, we need to look at if it is flaky due to the change.

You're right. I'll make sure to address those flaky tests next time.

…he#17670) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

* 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>

…he#17670) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

KAFKA-17925: Convert Kafka Client integration tests to use KRaft

0b576e6

Update pertinent integration tests to use KRaft and not Zookeeper.

github-actions Bot added core Kafka Broker tests Test fixes (including flaky tests) labels Nov 2, 2024

kirktrue mentioned this pull request Nov 2, 2024

Refactor integration and system tests to remove Zookeeper, add group protocol parameter #17581

Closed

3 tasks

kirktrue added kraft KIP-848 The Next Generation of the Consumer Rebalance Protocol clients ci-approved labels Nov 2, 2024

kirktrue self-assigned this Nov 2, 2024

Fixed stoopid tipos

e819a87

kirktrue added consumer ctr Consumer Threading Refactor (KIP-848) labels Nov 2, 2024

kirktrue marked this pull request as ready for review November 2, 2024 23:51

chia7712 approved these changes Nov 3, 2024

View reviewed changes

kirktrue added 9 commits November 4, 2024 11:35

Merge branch 'trunk' into KAFKA-17925-remove-zk-from-client-integrati…

a7c649b

…on-tests

Update EndToEndAuthorizationTest.scala

96b9532

Update to only test CLASSIC group protocol

Changing AuthorizerIntegrationTest to only test CLASSIC group protocol

72b8be7

Merge branch 'trunk' into KAFKA-17925-remove-zk-from-client-integrati…

c685ef3

…on-tests

Found more tests that use Consumers...

f53c221

Fixed more issues with group protocol

1a89586

More clean up for failing CONSUMER tests

2268e3c

Added a new temporary override for bug KAFKA-17964

1ca7caa

Fixed a swath of executeTieredStorageTest tests

4b66efb

github-actions Bot added the storage Pull requests that target the storage module label Nov 8, 2024

chia7712 reviewed Nov 8, 2024

View reviewed changes

Fixed missing group protocol in SaslClientsWithInvalidCredentialsTest

0083093

github-actions Bot added the tools label Nov 8, 2024

Fixed ordering of imports

eff9756

chia7712 mentioned this pull request Nov 9, 2024

KAFKA-17972: Set all tests inheriting from QuorumTestHarness to KRaft #17727

Closed

Added another set of override parameters for KAFKA-16176

79e02e8

kirktrue added 2 commits November 9, 2024 16:30

Merge branch 'trunk' into KAFKA-17925-remove-zk-from-client-integrati…

684f3c4

…on-tests

Update groupProtocolFromTestParameters() with a clearer error message

4de2217

chia7712 merged commit 42ea29c into apache:trunk Nov 10, 2024

kirktrue deleted the KAFKA-17925-remove-zk-from-client-integration-tests branch November 12, 2024 19:04

lianetm mentioned this pull request Nov 20, 2024

MINOR: enable consumer auth tests #17885

Merged

chiacyu pushed a commit to chiacyu/kafka that referenced this pull request Nov 30, 2024

KAFKA-17925 Convert Kafka Client integration tests to use KRaft (apac…

f51eb1a

…he#17670) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025

KAFKA-17925 Convert Kafka Client integration tests to use KRaft (apac…

e328023

…he#17670) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

lianetm mentioned this pull request Jan 21, 2025

KAFKA-18529: ConsumerRebootstrapTest should run for async consumer #18554

Merged

3 tasks

Conversation

kirktrue commented Nov 2, 2024

Committer Checklist (excluded from commit message)

Uh oh!

chia7712 commented Nov 3, 2024

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

kirktrue Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

kirktrue Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Nov 9, 2024

Uh oh!

chia7712 commented Nov 10, 2024

Uh oh!

lianetm commented Nov 15, 2024

Uh oh!

jolshan commented Nov 15, 2024

Uh oh!

jolshan commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jolshan commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lianetm commented Nov 18, 2024

Uh oh!

chia7712 commented Nov 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jolshan commented Nov 15, 2024 •

edited

Loading

jolshan commented Nov 15, 2024 •

edited

Loading