KAFKA-15764: Missing Tests for Transactions by jolshan · Pull Request #14702 · apache/kafka

jolshan · 2023-11-04T16:56:24Z

I ran this test 40 times without KAFKA-15653 with and without compression enabled.
With compression it failed 39/40 times and without it passed 40/40 times.

With the KAFKA-15653 and compression it passed 40/40 times locally

jolshan · 2023-11-08T18:41:34Z

    props.put(KafkaConfig.AutoLeaderRebalanceEnableProp, false.toString)
    props.put(KafkaConfig.GroupInitialRebalanceDelayMsProp, "0")
    props.put(KafkaConfig.TransactionsAbortTimedOutTransactionCleanupIntervalMsProp, "200")
+    props.put(KafkaConfig.NumNetworkThreadsProp, 2.toString)


I need to see if changing these values causes issues for the other tests.

I ran the other tests about 20 times locally and didn't see issues.

hachikuji · 2023-11-29T18:53:13Z

+      transactionalCompressionProducers += createTransactionalProducer("transactional-compression-producer-" + i.toString,  compressionType = "snappy")
+    }
+
+    // KAFKA-15653 is triggered more easily with replication factor 1


Do you know why?

This made sense to me before, but I'm trying to remember why. 😅

Is it possible that when we are building and sending the response we could receive the callback request and that means it will be handled on a different thread? Vs when acks=all, we free the thread after sending out the verification, so it can be available to receive the callback?

Not sure I follow that. Whether there are any replicas or not, wouldn't the request thread will be freed after sending out the verification? One way I could see the case being more likely is if we added additional request threads. I don't think there's any affinity in the callback to the original request thread, is there? So more available request threads probably means a greater chance a separate thread would handle the callback. Does that make sense?

Sorry I think I got confused about when the response is being sent. I will say that adding more request threads didn't work and I had to lower them to get it to reproduce frequently?

I can try it the other way around, but iirc, there were a lot of combinations that were not as consistent as it is now.

My take is that this test should probably just test compression of transactional data with the default settings. I don't think it necessarily needs to hit the case from KAFKA-15653. It seems like it doesn't do so reliably anyway. Are there lower level tests where we can validate callback safety?

I didn't see it fail much at all with the default settings. It was like once in 60 times or less which was not super useful. There are lower level tests for the callback using the correct requestLocal.

Yeah, what I'm saying is that this test seems more useful as a general validation of transactional produce with compression. From that perspective, the default settings are the most useful to test. For KAFKA-15653, the lower level tests seem sufficient.

I set back the default settings and it seems to trigger consistently again anyway. Maybe I just tricked myself into thinking I needed the other configs.

… in the test.

hachikuji

LGTM!

jolshan · 2023-12-06T17:46:23Z

I've restarted the build every day since the approval and have yet to get a clean build 😵‍💫

jolshan · 2023-12-15T17:35:08Z

I'm convinced this PR is cursed. Still failing builds. :(

jolshan · 2023-12-15T17:40:05Z

Given that this failed on a storage test (storage tests shouldn't be affected here), and that it was able to build on some of the many other runs I tried, I am going to merge this after 12 rebuilds. 😵

This reverts commit ed7ad6d.

lucasbru · 2023-12-17T09:29:41Z

@jolshan This PR might indeed be cursed. We have been seeing a lot of failures of TransactionsWithTieredStoreTest.testTransactionsWithCompression on trunk, and it seems to start with this PR. I see how this PR can influence the test via the change in TestUtils. The bad part is that it seems to kill the Gradle Executors completely.

#15029

https://ge.apache.org/scans/tests?search.relativeStartTime=P28D&search.rootProjectNames=kafka&search.timeZoneId=Europe%2FBerlin&tests.container=org.apache.kafka.tiered.storage.integration.TransactionsWithTieredStoreTest&tests.test=testTransactionsWithCompression(String)%5B2%5D

This reverts commit ed7ad6d. We have been seeing a lot of failures of TransactionsWithTieredStoreTest.testTransactionsWithCompression on trunk, and it seems to start with this PR. I see how this PR can influence the test via the change in TestUtils. The bad part is that sometimes seems to kill the Gradle Executors completely. So I'd suggest reverting the change before investigating further to stabilize CI. Reviewers: Bruno Cadonna <cadonna@apache.org>

I ran this test 40 times without KAFKA-15653 with and without compression enabled. With compression it failed 39/40 times and without it passed 40/40 times. With the KAFKA-15653 and compression it passed 40/40 times locally Reviewers: Jason Gustafson <jason@confluent.io>

…pache#15029) This reverts commit ed7ad6d. We have been seeing a lot of failures of TransactionsWithTieredStoreTest.testTransactionsWithCompression on trunk, and it seems to start with this PR. I see how this PR can influence the test via the change in TestUtils. The bad part is that sometimes seems to kill the Gradle Executors completely. So I'd suggest reverting the change before investigating further to stabilize CI. Reviewers: Bruno Cadonna <cadonna@apache.org>

I ran this test 40 times without KAFKA-15653 with and without compression enabled. With compression it failed 39/40 times and without it passed 40/40 times. With the KAFKA-15653 and compression it passed 40/40 times locally Reviewers: Jason Gustafson <jason@confluent.io>

…pache#15029) This reverts commit ed7ad6d. We have been seeing a lot of failures of TransactionsWithTieredStoreTest.testTransactionsWithCompression on trunk, and it seems to start with this PR. I see how this PR can influence the test via the change in TestUtils. The bad part is that sometimes seems to kill the Gradle Executors completely. So I'd suggest reverting the change before investigating further to stabilize CI. Reviewers: Bruno Cadonna <cadonna@apache.org>

I ran this test 40 times without KAFKA-15653 with and without compression enabled. With compression it failed 39/40 times and without it passed 40/40 times. With the KAFKA-15653 and compression it passed 40/40 times locally Reviewers: Jason Gustafson <jason@confluent.io>

…pache#15029) This reverts commit ed7ad6d. We have been seeing a lot of failures of TransactionsWithTieredStoreTest.testTransactionsWithCompression on trunk, and it seems to start with this PR. I see how this PR can influence the test via the change in TestUtils. The bad part is that sometimes seems to kill the Gradle Executors completely. So I'd suggest reverting the change before investigating further to stabilize CI. Reviewers: Bruno Cadonna <cadonna@apache.org>

jolshan force-pushed the kafka-15764 branch 2 times, most recently from 7e64548 to 1c7a52b Compare November 6, 2023 18:45

jolshan added 2 commits November 8, 2023 10:38

Add test for compression

2adeb63

add note

eb40adf

jolshan force-pushed the kafka-15764 branch from a443faa to eb40adf Compare November 8, 2023 18:39

jolshan marked this pull request as ready for review November 8, 2023 18:40

jolshan commented Nov 8, 2023

View reviewed changes

jolshan changed the title ~~DRAFT: KAFKA-15764: Missing Tests for Transactions~~ KAFKA-15764: Missing Tests for Transactions Nov 8, 2023

jolshan commented Nov 8, 2023

View reviewed changes

Comment thread core/src/test/scala/integration/kafka/api/TransactionsTest.scala Outdated

Fix fencing tests

ecb1286

hachikuji reviewed Nov 29, 2023

View reviewed changes

Set configs back and make replication factor the same as other topics…

829d52f

… in the test.

hachikuji approved these changes Nov 30, 2023

View reviewed changes

Merge branch 'trunk' of github.com:apache/kafka into kafka-15764

16c6dc7

Merge branch 'trunk' of github.com:apache/kafka into kafka-15764

31a0257

jolshan merged commit ed7ad6d into apache:trunk Dec 15, 2023

lucasbru added a commit to lucasbru/kafka that referenced this pull request Dec 17, 2023

Revert "KAFKA-15764: Missing Tests for Transactions (apache#14702)"

6252724

This reverts commit ed7ad6d.

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

jolshan commented Dec 6, 2023

Uh oh!

jolshan commented Dec 15, 2023

Uh oh!

jolshan commented Dec 15, 2023

Uh oh!

lucasbru commented Dec 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jolshan commented Nov 4, 2023 •

edited

Loading