Skip to content
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.

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

KAFKA-16368: Update default linger.ms to 5 for KIP-1030 #18080

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

jayteej
Copy link
Contributor

@jayteej jayteej commented Dec 6, 2024

KAFKA-16368: As part of KIP-1030 we are increasing the default linger.ms to 5ms. System tests updated and behaviour validated manually.

@github-actions github-actions bot added core Kafka Broker producer clients small Small PRs labels Dec 6, 2024
@jayteej jayteej force-pushed the KAFKA-16368-LINGER-MS branch 4 times, most recently from 05d0dd1 to 8308b32 Compare December 9, 2024 09:30
@divijvaidya
Copy link
Contributor

Similar comment as #18106 (comment). We need to be very explicit about the impact customers can expect on upgrade for this particular config.

@jayteej jayteej force-pushed the KAFKA-16368-LINGER-MS branch from 8308b32 to 641e0f9 Compare December 10, 2024 14:51
@jayteej
Copy link
Contributor Author

jayteej commented Dec 10, 2024

@divijvaidya done

Copy link
Contributor

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

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

The change look good. I am waiting for the tests to succeed before merging this in.

Copy link
Contributor

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

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

The change look good. I am waiting for the tests to succeed before merging this in.

@divijvaidya divijvaidya added the kip Requires or implements a KIP label Dec 11, 2024
@jayteej jayteej force-pushed the KAFKA-16368-LINGER-MS branch from 641e0f9 to a7b101e Compare December 12, 2024 11:07
@jayteej
Copy link
Contributor Author

jayteej commented Dec 13, 2024

These tests are passing locally so I'm not sure whats going on. Will try another rebase to trunk

@jayteej jayteej force-pushed the KAFKA-16368-LINGER-MS branch from a7b101e to 12f6bd8 Compare December 13, 2024 11:54
+ "batch size is under this <code>batch.size</code> setting.";
+ "This <code>linger.ms</code> setting defaults to 5, which means we'll wait 5ms for the accumulated "
+ "<code>batch.size</code> to fill as much as it can in this timeframe. This value previously defaulted"
+ "to 0, but over time we have observed that the IO overhead caused by smaller batches negates any latency gains.";
Copy link
Member

@ijuma ijuma Dec 22, 2024

Choose a reason for hiding this comment

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

The wording here is a little odd. I would write something like:

"This defaults to 5, which means the producer will wait for 5ms or until the record batch is of batch.size (whichever happens first) before sending the record batch. Note that broker backpressure can result in a higher effective linger time than this setting.

The default changed from 0 to 5 in Apache Kafka 4.0 as the efficiency gains from larger batches typically result in similar or lower producer latency despite the increased linger."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

+ "specified time waiting for more records to show up. This setting defaults to 0 (i.e. no delay). Setting <code>" + LINGER_MS_CONFIG + "=5</code>, "
+ "for example, would have the effect of reducing the number of requests sent but would add up to 5ms of latency to records sent in the absence of load.";
+ "specified time waiting for more records to show up. This setting defaults to 5 (i.e. 5ms delay). Increasing <code>" + LINGER_MS_CONFIG + "=50</code>, "
+ "for example, would have the effect of reducing the number of requests sent but would add up to 50ms of latency to records sent in the absence of load."
Copy link
Member

Choose a reason for hiding this comment

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

I don't follow, we seem to be saying that increasing from 5ms to 50ms would add up to 50ms of latency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not changed the wording here. This is the same example with different values. I can change the wording also if you think it helps or isn't grokable?

@@ -175,6 +175,9 @@ <h5><a id="upgrade_400_notable" href="#upgrade_400_notable">Notable changes in 4
</li>
<li>The deprecated <code>sendOffsetsToTransaction(Map&lt;TopicPartition, OffsetAndMetadata&gt;, String)</code> method has been removed from the Producer API.
</li>
<li>The default value of <code>linger.ms</code> has been changed from 0ms to 5ms. The impact of this is larger request batches will be filled by default at the expense of
time in the producer. It is generally observed that values lower than 5ms do not yield any latency benefits due to the IO overhead of smaller requests.
Copy link
Member

Choose a reason for hiding this comment

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

I would probably use similar wording to the one I suggested in a different comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jayteej jayteej force-pushed the KAFKA-16368-LINGER-MS branch from 12f6bd8 to 22f795c Compare December 23, 2024 15:33
@divijvaidya
Copy link
Contributor

We pushed out a commit e099fce two weeks ago which should make the testBumpTransactionalEpochWithTV2Enabled stable. This is the test which is failing in this build.

@jayteej can you please rebase from trunk one more time.

@jayteej
Copy link
Contributor Author

jayteej commented Jan 15, 2025

Known flakey failure

./gradlew clean :clients:test --tests 'org.apache.kafka.clients.ClientUtilsTest'

Gradle Test Run :clients:test > Gradle Test Executor 47 > ClientUtilsTest > testParseAndValidateAddressesWithReverseLookup() FAILED
    org.opentest4j.AssertionFailedError: Unexpected addresses [a96-7-128-175.deploy.static.akamaitechnologies.com, a23-215-0-138.deploy.static.akamaitechnologies.com, a23-215-0-136.deploy.static.akamaitechnologies.com, a23-192-228-84.deploy.static.akamaitechnologies.com, a23-192-228-80.deploy.static.akamaitechnologies.com, a96-7-128-198.deploy.static.akamaitechnologies.com, 2600:1408:ec00:36:0:0:1736:7f24, 2600:1406:bc00:53:0:0:b81e:94ce, 2600:1406:bc00:53:0:0:b81e:94c8, 2600:1406:3a00:21:0:0:173e:2e66, 2600:1406:3a00:21:0:0:173e:2e65, 2600:1408:ec00:36:0:0:1736:7f31] ==> expected: <true> but was: <false>
        at app//org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
        at app//org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
        at app//org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
        at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
        at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214)
        at app//org.apache.kafka.clients.ClientUtilsTest.testParseAndValidateAddressesWithReverseLookup(ClientUtilsTest.java:67)

@jayteej jayteej force-pushed the KAFKA-16368-LINGER-MS branch from d17b036 to 93019d4 Compare January 15, 2025 15:13
@divijvaidya divijvaidya merged commit 11c10fe into apache:trunk Jan 16, 2025
9 checks passed
divijvaidya pushed a commit that referenced this pull request Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-approved clients core Kafka Broker kip Requires or implements a KIP producer small Small PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants