Skip to content

KAFKA-18206: EmbeddedKafkaCluster must set features #18189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 17 commits into from
Feb 5, 2025
Merged

KAFKA-18206: EmbeddedKafkaCluster must set features #18189

merged 17 commits into from
Feb 5, 2025

Conversation

brandboat
Copy link
Member

@brandboat brandboat commented Dec 15, 2024 •
edited
Loading

related to KAFKA-18206, set features in EmbeddedKafkaCluster in both streams and connect module, note that this PR also fix potential transaction with empty records in sendPrivileged method as transaction version 2 doesn't allow this kind of scenario.

Committer Checklist (excluded from commit message)

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

@github-actions github-actions bot added tests Test fixes (including flaky tests) small Small PRs labels Dec 15, 2024
@brandboat brandboat marked this pull request as draft December 15, 2024 03:27
@brandboat brandboat marked this pull request as ready for review December 27, 2024 15:20
@mjsax
Copy link
Member

mjsax commented Jan 11, 2025

note that this PR also fix potential transaction with empty records in sendPrivileged method as transaction version 2 doesn't allow this kind of scenario.

Should we split this into it's own PR?

@brandboat
Copy link
Member Author

Should we split this into it's own PR?

Hi @mjsax, sorry I forgot to mention that this was handling in a different issue, here is the PR: #18448

@jolshan
Copy link
Member

jolshan commented Jan 15, 2025

We have merged 18448 so we should be able to proceed here :)

@brandboat
Copy link
Member Author

brandboat commented Jan 16, 2025 •
edited
Loading

I'm not sure why, but it seems like a lot of tests have become more flaky than before in this PR. One failed test in CI (https://github.com/apache/kafka/actions/runs/12810813472) caught my attention: MirrorConnectorsIntegrationTransactionsTest #testOffsetTranslationBehindReplicationFlow. The error message is:

java.lang.RuntimeException: java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.InvalidTxnStateException: The producer attempted a transactional operation in an invalid state.

I expected this failed test should to be mitigated after merging PR #18448, so I'll spend some time investigating this.

@brandboat brandboat changed the title [DO NOT MERGE] KAFKA-18206: EmbeddedKafkaCluster must set features KAFKA-18206: EmbeddedKafkaCluster must set features Jan 20, 2025
@brandboat
Copy link
Member Author

This #18189 (comment) seems due to TV_2 return incorrect error, and should be handled correctly in #18604.

@jolshan
Copy link
Member

jolshan commented Jan 22, 2025 •
edited
Loading

@brandboat I just merged my PR and will restart the tests here. You may need to rebase, but we can try as is for now.

@brandboat
Copy link
Member Author

Bunch of ci failed in this PR since ClearElrRecord isn't corrected handled, but will be fixed after #18708 merged.

@brandboat brandboat changed the title KAFKA-18206: EmbeddedKafkaCluster must set features (WIP) KAFKA-18206: EmbeddedKafkaCluster must set features Jan 27, 2025
@brandboat brandboat changed the title (WIP) KAFKA-18206: EmbeddedKafkaCluster must set features KAFKA-18206: EmbeddedKafkaCluster must set features Jan 30, 2025
@jolshan
Copy link
Member

jolshan commented Jan 30, 2025

The build is looking better! Maybe we just need to look in to the KRaftClusterTest failure?

@brandboat
Copy link
Member Author

The build is looking better! Maybe we just need to look in to the KRaftClusterTest failure?

Yes, I'm currently investigating the root cause of this test failure. Thanks for the heads-up!

@brandboat
Fix testAdminClientApisAuthenticationFailure
9cf45ac 
after ec49a60, if ELR is enabled,
a cluster-level min.insync.replicas will be set to 1, and this PR
aims to update all feature version to the latest.
@github-actions github-actions bot added the core Kafka Broker label Jan 31, 2025
@brandboat
Copy link
Member Author

Finally got an AC 🎉 , perhaps we can move on?

jolshan
jolshan reviewed Feb 4, 2025
View reviewed changes
core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala
@@ -696,7 +696,8 @@ class KRaftClusterTest {
new AlterConfigOp(new ConfigEntry("max.connections.per.ip", "60"), OpType.SET))))))
validateConfigs(admin, Map(new ConfigResource(Type.BROKER, "") -> Seq(
("log.roll.ms", "1234567"),
("max.connections.per.ip", "60"))), exhaustive = true)
("max.connections.per.ip", "60"),
("min.insync.replicas", "1"))), exhaustive = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we make this change?

Copy link
Member Author

@brandboat brandboat Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after ec49a60, if ELR is enabled, a cluster-level min.insync.replicas will be set to 1, and this PR will enable ELR by default in all integration tests that use TestKitNodes.java

@jolshan
Copy link
Member

jolshan commented Feb 4, 2025

I'm also curious why the tests are returning 1 if there are just flaky tests. Has this always been the case?

@brandboat
Copy link
Member Author

I'm also curious why the tests are returning 1 if there are just flaky tests. Has this always been the case?

Hi @jolshan, pardon me, did you mean EagerConsumerCoordinatorTest.testOutdatedCoordinatorAssignment? If so, yes it it a flaky test, and we already have a JIRA ticket for it: https://issues.apache.org/jira/browse/KAFKA-15900

jolshan
jolshan approved these changes Feb 5, 2025
View reviewed changes
Copy link
Member

@jolshan jolshan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your persistence here @brandboat !

@jolshan jolshan merged commit b99be96 into apache:trunk Feb 5, 2025
9 checks passed
jolshan pushed a commit that referenced this pull request Feb 5, 2025
@brandboat @jolshan
KAFKA-18206: EmbeddedKafkaCluster must set features (#18189)
4c9d335 
related to KAFKA-18206, set features in EmbeddedKafkaCluster in both streams and connect module, note that this PR also fix potential transaction with empty records in sendPrivileged method as transaction version 2 doesn't allow this kind of scenario.

Reviewers: Justine Olshan <[email protected]>
@jolshan
Copy link
Member

jolshan commented Feb 5, 2025

Also pushed to 4.0. Thanks!

@brandboat brandboat deleted the KAFKA-18206 branch February 6, 2025 01:57
pdruley pushed a commit to pdruley/kafka that referenced this pull request Feb 12, 2025
@brandboat @pdruley
KAFKA-18206: EmbeddedKafkaCluster must set features (apache#18189)
d4110dc 
related to KAFKA-18206, set features in EmbeddedKafkaCluster in both streams and connect module, note that this PR also fix potential transaction with empty records in sendPrivileged method as transaction version 2 doesn't allow this kind of scenario.

Reviewers: Justine Olshan <[email protected]>
manoj-mathivanan pushed a commit to manoj-mathivanan/kafka that referenced this pull request Feb 19, 2025
@brandboat @manoj-mathivanan
KAFKA-18206: EmbeddedKafkaCluster must set features (apache#18189)
e7c5163 
related to KAFKA-18206, set features in EmbeddedKafkaCluster in both streams and connect module, note that this PR also fix potential transaction with empty records in sendPrivileged method as transaction version 2 doesn't allow this kind of scenario.

Reviewers: Justine Olshan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jolshan jolshan jolshan approved these changes

Assignees
No one assigned
Labels
connect core Kafka Broker small Small PRs tests Test fixes (including flaky tests)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brandboat @mjsax @jolshan