MINOR: Improve docs for retries & cleanup #19595

lianetm · 2025-04-29T17:32:05Z

Improve docs for retries config, mainly to clarify the expected
behaviour on retries=0 Also remove unused funcs and fix typo.

clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java

clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java

mjsax · 2025-04-29T23:17:59Z

clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java

+    private static final String RETRIES_DOC = "Number of times to retry a request that fails with a transient error."
+            + " Setting a value greater than zero will cause the client to resend any record whose send fails with a potentially transient error. "
+            + " Requests will be retried this many times until they succeed, fail with a non-transient error, or the <code> DELIVERY_TIMEOUT_MS_CONFIG </code> expires."
+            + " Note that this automatic retry is no different than if the client resent the record upon receiving the error."


Is this actually correct? I mean, internal retires are idempotent (at least by default, as long as idempotency is not disabled), while application retires are not idempotent.

well I would say it's correct saying that automatic retries won't mess around/change the request (and that would be exactly the same as if the client resent the request, without messing around with it). Assuming no one changes the request (us internally, or the client app), both retries on retriables would behave the same right? Makes sense?

that automatic retries won't mess around/change the request (and that would be exactly the same as if the client resent the request, without messing around with it

I am confused. I read "the client resent the request" as the application code calls send(...) again? Do I miss understand? If "the client resent the request" is not meaning app calls send(...) again, that what does it mean?

Same page on the meaning (I also take it as "the client resent the request" == "application code calls send(...)").

And totally agree with you that this is wrong from the idempotence point of view, my point was only that I read it as making a valid point about the automatic retries not doing any magic, just resend. I changed the sentence, let me know

mjsax · 2025-04-29T23:19:20Z

clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java

+            + " Requests will be retried this many times until they succeed, fail with a non-transient error, or the <code> DELIVERY_TIMEOUT_MS_CONFIG </code> expires."
+            + " Note that this automatic retry is no different than if the client resent the record upon receiving the error."
+            + " Setting a value of zero will disable this automatic retry behaviour, so that the transient errors will be propagated to the application to be handled."
+            + " Users should generally prefer to leave this config unset and instead use <code>" + DELIVERY_TIMEOUT_MS_CONFIG + "</code> to control"
            + " retry behavior."
            + "<p>"
            + "Enabling idempotence requires this config value to be greater than 0."


Given that idempotent producer is enabled by default, might be worth to rephrase this, too?

well the trick is that there is a diff between user enabling idemp explicitly (then it does "require this config value to be greater than 0.", will throw otherwise), vs. user not setting the prop, in which case idemp is enabled by default as you said, but it does not required the retries >0 (If retries=zero it won't fail, it simply disables idempotence automatically and the retries=0 is respected). Makes sense?

If retries=zero it won't fail, it simply disables idempotence automatically and the retries=0 is respected.

That is very unexpected to me -- I would expect a config error. What is the reason for this? Sound like a footgun to me, if changing retries has this side effect. (Side effect are IMHO in general most likely a bad thing.)

yeah, I agree with the feeling, but as it's often the case, it was a matter of a trade off, the reason explained here https://issues.apache.org/jira/browse/KAFKA-13673.

Thanks. Interesting.

Yunyung · 2025-04-30T01:38:16Z

Thanks for the PR. I left just one minor.

Yunyung · 2025-04-30T01:31:54Z

clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java

@@ -100,7 +100,7 @@ public class ProducerConfig extends AbstractConfig {
                                                 + "similar or lower producer latency despite the increased linger.";

    /** <code>partitioner.adaptive.partitioning.enable</code> */
-    public static final String PARTITIONER_ADPATIVE_PARTITIONING_ENABLE_CONFIG = "partitioner.adaptive.partitioning.enable";
+    public static final String PARTITIONER_ADAPTIVE_PARTITIONING_ENABLE_CONFIG = "partitioner.adaptive.partitioning.enable";
    private static final String PARTITIONER_ADPATIVE_PARTITIONING_ENABLE_DOC =


Also typo.

Suggested change

private static final String PARTITIONER_ADPATIVE_PARTITIONING_ENABLE_DOC =

private static final String PARTITIONER_ADAPTIVE_PARTITIONING_ENABLE_DOC =

I'll leave of the adaptive typo changes out, it probably makes sense to address them together when the public config can be fixed.

mingyen066 · 2025-04-30T07:12:05Z

clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java

@@ -415,7 +415,7 @@ public KafkaProducer(Properties properties, Serializer<K> keySerializer, Seriali
            this.transactionManager = configureTransactionState(config, logContext);
            // There is no need to do work required for adaptive partitioning, if we use a custom partitioner.
            boolean enableAdaptivePartitioning = partitionerPlugin.get() == null &&
-                config.getBoolean(ProducerConfig.PARTITIONER_ADPATIVE_PARTITIONING_ENABLE_CONFIG);


@m1a2st found that this is public interface and I found someone has previously renamed this config. The earlier conclusion was that we should submit a trivial KIP to deprecate this config.
#17908

uh totally, good catch. Removed all the changes on this

FrankYang0529 · 2025-04-30T07:25:29Z

clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java

@@ -88,8 +88,9 @@ public class CommonClientConfigs {
        "If provided, the backoff per host will increase exponentially for each consecutive connection failure, up to this maximum. After calculating the backoff increase, 20% random jitter is added to avoid connection storms.";

    public static final String RETRIES_CONFIG = "retries";
-    public static final String RETRIES_DOC = "Setting a value greater than zero will cause the client to resend any request that fails with a potentially transient error." +
-        " It is recommended to set the value to either zero or `MAX_VALUE` and use corresponding timeout parameters to control how long a client should retry a request.";
+    public static final String RETRIES_DOC = "It is recommended to set the value to either `MAX_VALUE` or zero, and use corresponding timeout parameters to control how long a client should retry a request." +


nit: We can change '`' to <code>, so we can have syntax on document.

Suggested change

public static final String RETRIES_DOC = "It is recommended to set the value to either `MAX_VALUE` or zero, and use corresponding timeout parameters to control how long a client should retry a request." +

public static final String RETRIES_DOC = "It is recommended to set the value to either <code>MAX_VALUE</code> or zero, and use corresponding timeout parameters to control how long a client should retry a request." +

before:

after:

lianetm · 2025-04-30T15:50:01Z

Thank you all for the feedback! Comments addressed and answers inline.

docs and cleanup

da223d9

lianetm requested a review from mjsax April 29, 2025 17:32

github-actions bot added producer clients small Small PRs labels Apr 29, 2025

mjsax reviewed Apr 29, 2025

View reviewed changes

clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java Outdated Show resolved Hide resolved

mjsax reviewed Apr 29, 2025

View reviewed changes

clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java Outdated Show resolved Hide resolved

update around retry time boundary

634b4e8

mjsax reviewed Apr 29, 2025

View reviewed changes

clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java Outdated Show resolved Hide resolved

mjsax reviewed Apr 29, 2025

View reviewed changes

Yunyung reviewed Apr 30, 2025

View reviewed changes

mingyen066 reviewed Apr 30, 2025

View reviewed changes

FrankYang0529 reviewed Apr 30, 2025

View reviewed changes

address comments

f6ea2cb

fix misleading retry sentence

033b4f9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: Improve docs for retries & cleanup #19595

MINOR: Improve docs for retries & cleanup #19595

lianetm commented Apr 29, 2025 •

edited by github-actions bot

Loading

mjsax Apr 29, 2025

lianetm Apr 30, 2025

mjsax Apr 30, 2025

lianetm May 1, 2025

mjsax Apr 29, 2025

lianetm Apr 30, 2025 •

edited

Loading

mjsax Apr 30, 2025

lianetm May 1, 2025

mjsax May 1, 2025

Yunyung commented Apr 30, 2025

Yunyung Apr 30, 2025

lianetm Apr 30, 2025

mingyen066 Apr 30, 2025 •

edited

Loading

lianetm Apr 30, 2025 •

edited

Loading

FrankYang0529 Apr 30, 2025

lianetm commented Apr 30, 2025

	private static final String PARTITIONER_ADPATIVE_PARTITIONING_ENABLE_DOC =
	private static final String PARTITIONER_ADAPTIVE_PARTITIONING_ENABLE_DOC =

	public static final String RETRIES_DOC = "It is recommended to set the value to either `MAX_VALUE` or zero, and use corresponding timeout parameters to control how long a client should retry a request." +
	public static final String RETRIES_DOC = "It is recommended to set the value to either <code>MAX_VALUE</code> or zero, and use corresponding timeout parameters to control how long a client should retry a request." +

MINOR: Improve docs for retries & cleanup #19595

Are you sure you want to change the base?

MINOR: Improve docs for retries & cleanup #19595

Conversation

lianetm commented Apr 29, 2025 • edited by github-actions bot Loading

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

lianetm Apr 30, 2025 • edited Loading

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

Yunyung commented Apr 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mingyen066 Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

lianetm Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lianetm commented Apr 30, 2025

lianetm commented Apr 29, 2025 •

edited by github-actions bot

Loading

lianetm Apr 30, 2025 •

edited

Loading

mingyen066 Apr 30, 2025 •

edited

Loading

lianetm Apr 30, 2025 •

edited

Loading