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

kirktrue · 2024-10-22T20:05:06Z

Removing "zk" from the list of quorum values to test in the integration and system tests. This does not remove all occurrences as not all tests support KRaft.

Committer Checklist (excluded from commit message)

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

SslProducerSendTest did not need to be updated

AndrewJSchofield · 2024-10-23T14:56:02Z

core/src/test/scala/integration/kafka/admin/ListOffsetsIntegrationTest.scala

@@ -66,7 +66,7 @@ class ListOffsetsIntegrationTest extends KafkaServerTestHarness {
  }

  @ParameterizedTest
-  @ValueSource(strings = Array("zk", "kraft"))
+  @ValueSource(strings = Array("kraft"))
  def testListMaxTimestampWithEmptyLog(quorum: String): Unit = {


Isn't the proper change here to remove the quorum parameter entirely?

I'd wondered that and wanted someone to raise the question.

I think it makes sense to remove it.

Happy to oblige :)

I have a question out to @cmccabe regarding his preference.

TestInfoUtils uses the parameters to determine if it isKraft(), isZkMigrationTest(), etc. Usages of those methods would need to be updated too.

Yes, I'm sure, but wouldn't AK 4.0 have all of the ZK migration tests removed, and wouldn't all tests be ZK?

There are some shenanigans done in the base test classes regarding the quorum param. Until we remove the ZK bits from that test infra, I think we actually need the quorum param :-/

I approve of the use of "shenanigans" to describe this 👍

…ProtocolOnly_KAFKA_XXXXX

… ZK parameters

kirktrue · 2024-10-29T23:51:49Z

@chia7712—do you have any advice for the best way to arrange these changes for review?

kirktrue · 2024-10-31T20:52:00Z

@mumrah—can you take a look at approach in this PR and let me know if it's correct? Thanks!

mumrah

@kirktrue the approach in the ducktape tests seems fine to me.

You might consider doing the system test changes in a separate PR since this one has grown quite large.

mumrah · 2024-11-01T15:51:46Z

core/src/test/scala/integration/kafka/admin/ListOffsetsIntegrationTest.scala

@@ -66,7 +66,7 @@ class ListOffsetsIntegrationTest extends KafkaServerTestHarness {
  }

  @ParameterizedTest
-  @ValueSource(strings = Array("zk", "kraft"))
+  @ValueSource(strings = Array("kraft"))
  def testListMaxTimestampWithEmptyLog(quorum: String): Unit = {


There are some shenanigans done in the base test classes regarding the quorum param. Until we remove the ZK bits from that test infra, I think we actually need the quorum param :-/

kirktrue · 2024-11-01T18:40:00Z

@mumrah @cmccabe—is there an existing Jira that PR overlaps with the changes in this PR? I want to make sure I'm not duplicating efforts and/or stepping on anyone’s work.

Ideally there would be two PRs: one for the integration tests and one for the system tests, as @mumrah suggested above.

Thanks!

kevin-wu24 · 2024-11-01T21:44:09Z

@mumrah @cmccabe—is there an existing Jira that PR overlaps with the changes in this PR? I want to make sure I'm not duplicating efforts and/or stepping on anyone’s work.

Ideally there would be two PRs: one for the integration tests and one for the system tests, as @mumrah suggested above.

Thanks!

Your system test changes are a superset of the changes I made in https://github.com/apache/kafka/pull/17638/files (involves just removing explicit quorum.zk parameterizations), which is tracked by https://issues.apache.org/jira/browse/KAFKA-17625. When I tried making the metadata_quorum default value to use KRaft I ran into some issues getting the tests to work properly, so I created some separate JIRA issues for those tests (The Kafka Streams work is already being tracked here: https://issues.apache.org/jira/browse/KAFKA-17609):

My PR also removes zk from quorum.all_non_upgrade and quorum.all so those ZK test variations would be removed, but I don't see that in this PR's diff.

kirktrue · 2024-11-02T23:14:57Z

Closing this PR in favor of #17669 and #17670.

kirktrue added 6 commits October 22, 2024 12:15

Upgraded ConsumerWithLegacyMessageFormatIntegrationTest

965ff03

Update LogAppendTimeTest.scala

753626a

Update FetchFromFollowerIntegrationTest.scala

270fde8

Update RemoteTopicCrudTest.scala

4277b23

Update ListOffsetsIntegrationTest.scala

61fec39

Updating BaseProducerSendTest and its subclass PlaintextProducerSendTest

cb06a79

SslProducerSendTest did not need to be updated

github-actions bot added core Kafka Broker tests Test fixes (including flaky tests) labels Oct 22, 2024

kirktrue added consumer kraft ctr Consumer Threading Refactor (KIP-848) clients ci-approved labels Oct 22, 2024

kirktrue requested review from chia7712 and showuon October 22, 2024 20:06

AndrewJSchofield suggested changes Oct 23, 2024

View reviewed changes

kirktrue added 14 commits October 23, 2024 10:58

Merge branch 'trunk' into kirktrue-remove-zookeeper-tests-part-1

9a46023

Updates to reduce boilerplate

18d46e8

Removed more uses of "zk" as a quorum test parameter

1f6b564

More clean up to remove ZK tests

c1df673

Updates for more tests

322aeb9

Merge branch 'trunk' into kirktrue-remove-zookeeper-tests-part-1

b52488c

Merge branch 'trunk' into kirktrue-remove-zookeeper-tests-part-1

eabc67c

The Great Migration

c789dfb

Fixed whitespace change

36535c6

Reverted removal of @nowarn

6400957

Temporarily added getTestQuorumAndGroupProtocolParametersClassicGroup…

7f0e4a1

…ProtocolOnly_KAFKA_XXXXX

Merge branch 'trunk' into kirktrue-remove-zookeeper-tests-part-1

e3e2c8c

Merge branch 'trunk' into kirktrue-remove-zookeeper-tests-part-1

a7dd66c

Support for changing the group.protocol from the default and removing…

999c528

… ZK parameters

github-actions bot added the tools label Oct 29, 2024

kirktrue added 3 commits October 29, 2024 13:22

Merge branch 'trunk' into kirktrue-remove-zookeeper-tests-part-1

1252f08

WIP system test updates

28248e9

Whitespace and other minor-ish changes

c02835e

kirktrue changed the title ~~[MIINOR] Refactor integration tests to remove Zookeeper, add group protocol parameter~~ [MIINOR] Refactor integration and system tests to remove Zookeeper, add group protocol parameter Oct 29, 2024

kirktrue added 2 commits October 29, 2024 16:58

Reverting tests to allow them to fail

e0900f5

Hopefully that's obvious enough :)

8d80b63

kirktrue changed the title ~~[MIINOR] Refactor integration and system tests to remove Zookeeper, add group protocol parameter~~ Refactor integration and system tests to remove Zookeeper, add group protocol parameter Oct 30, 2024

Merge branch 'trunk' into kirktrue-remove-zookeeper-tests-part-1

a113f6b

mumrah reviewed Nov 1, 2024

View reviewed changes

kirktrue mentioned this pull request Nov 1, 2024

MINOR: Use JDK 11 in Vagrant after dropping JDK 8 #17576

Closed

3 tasks

Merge branch 'trunk' into kirktrue-remove-zookeeper-tests-part-1

46c6f4a

Merge branch 'trunk' into kirktrue-remove-zookeeper-tests-part-1

f03f28c

kirktrue closed this Nov 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kirktrue commented Oct 22, 2024 •

edited

Loading

AndrewJSchofield Oct 23, 2024

kirktrue Oct 23, 2024

AndrewJSchofield Oct 23, 2024

kirktrue Oct 23, 2024

kirktrue Oct 23, 2024

AndrewJSchofield Oct 24, 2024

mumrah Nov 1, 2024

kirktrue Nov 1, 2024

kirktrue commented Oct 29, 2024

kirktrue commented Oct 31, 2024

mumrah left a comment

mumrah Nov 1, 2024

kirktrue commented Nov 1, 2024

kevin-wu24 commented Nov 1, 2024

kirktrue commented Nov 2, 2024

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

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

Conversation

kirktrue commented Oct 22, 2024 • edited Loading

Committer Checklist (excluded from commit message)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirktrue commented Oct 29, 2024

kirktrue commented Oct 31, 2024

mumrah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirktrue commented Nov 1, 2024

kevin-wu24 commented Nov 1, 2024

kirktrue commented Nov 2, 2024

kirktrue commented Oct 22, 2024 •

edited

Loading