MINOR: improve the min.insync.replicas doc #20237

showuon · 2025-07-24T08:37:59Z

Along with the change: #17952
(KIP-966),
the semantics of min.insync.replicas config has small change, and add
some constraints. We should document them clearly.

Reviewers: Jun Rao [email protected], Calvin Liu [email protected],
Mickael Maison [email protected], Paolo Patierno
[email protected], Federico Valeri [email protected], Chia-Ping
Tsai [email protected]

showuon · 2025-07-24T08:40:16Z

@CalvinConfluent @mumrah @cmccabe, please take a look. Thanks.

ppatierno

LGTM

chia7712 · 2025-07-24T14:39:16Z


> Task :clients:checkstyleMain
[ant:checkstyle] [ERROR] /home/chia7712/project/kafka/clients/src/main/java/org/apache/kafka/common/config/TopicConfig.java:185:223: ';' is preceded with whitespace. [NoWhitespaceBefore]

> Task :clients:checkstyleMain FAILED

@showuon could you please fix the build error

fvaleri

LGTM. Thanks.

showuon · 2025-07-28T07:09:36Z

@chia7712 @mimaison , could you have a look when available? I think we should add this in v4.1.0 doc. Thanks.

mimaison · 2025-07-29T20:00:18Z

Thanks for the PR!
While this is an improvement, I still find the new description not very clear. I'll try to make suggestions in the next couple of days.

CalvinConfluent

Thanks for the PR! LGTM

docs/ops.html

mimaison

Thanks for the PR. I made a few suggestions

clients/src/main/java/org/apache/kafka/common/config/TopicConfig.java

docs/ops.html

mimaison · 2025-07-30T17:17:39Z

docs/ops.html

-    all the ELR fields will be cleaned.</p>
+    Note that when the ELR feature is enabled:
+    <ul>
+      <li>The override of <code>min.insync.replicas</code> in broker-level will be removed.</li>


will be removed -> is removed

We probably also want to warn the users that if they previously only set min.insync.replicas at the broker level, once they enable ELR, this info will be lost and they need to reset that at the cluster level.

Is the above correct @CalvinConfluent ?

Added
The previously set <code>min.insync.replicas</code> value at the broker-level config will be removed.

mimaison · 2025-07-30T17:18:32Z

docs/ops.html

+      <li>The override of <code>min.insync.replicas</code> in broker-level will be removed.</li>
+      <li>The alter of <code>min.insync.replicas</code> config in broker-level is not allowed.</li>
+      <li>The removal of <code>min.insync.replicas</code> config in cluster-level is not allowed.</li>
+      <li>If <code>min.insync.replicas</code> is updated for a topic, the ELR field will be cleaned.</li>


Can we switch to present tense? Also what does will be cleaned means exactly?

It means the ELR state in the controller side will be empty(reset) because the new updated min ISR makes the existing ELR state invalid. @CalvinConfluent , is my understanding correct?

Is it hard to understand for users? Maybe change to
If <code>min.insync.replicas</code> is updated for a topic, the ELR state will be reset.?

mimaison · 2025-07-30T17:18:43Z

docs/ops.html

+      <li>The alter of <code>min.insync.replicas</code> config in broker-level is not allowed.</li>
+      <li>The removal of <code>min.insync.replicas</code> config in cluster-level is not allowed.</li>
+      <li>If <code>min.insync.replicas</code> is updated for a topic, the ELR field will be cleaned.</li>
+      <li>If the cluster default <code>min.insync.replicas</code> is updated, all the ELR fields will be cleaned.</li>


docs/ops.html

junrao

@showuon : Thanks for the updated PR. A couple of minor comments.

junrao · 2025-07-31T16:07:05Z

docs/ops.html

+  <ul>
+    <li>The cluster-level <code>min.insync.replicas</code> config will be added if there is not any. The value is the same as the static config in the active controller.</li>
+    <li>The previously set <code>min.insync.replicas</code> value at the broker-level config will be removed.</li>
+    <li>The override of <code>min.insync.replicas</code> in broker-level will be removed.</li>


The above two line are basically the same. We can just keep one. But it will be useful to inform the user to reset min.insync.replicas as the cluster level if it's previously set only at the broker level.

Removed the L4506, and updated to:
The previously set <code>min.insync.replicas</code> value at the broker-level config will be removed. Please set in the cluster-level if necessary.

docs/ops.html

chia7712 · 2025-07-31T17:48:59Z

docs/ops.html

+    <li>The alteration of <code>min.insync.replicas</code> config in broker-level is not allowed.</li>
+    <li>The removal of <code>min.insync.replicas</code> config in cluster-level is not allowed.</li>
+    <li>If <code>min.insync.replicas</code> is updated for a topic, the ELR field will be cleaned.</li>
+    <li>If the cluster default <code>min.insync.replicas</code> is updated, all the ELR fields will be cleaned.</li>


nit: A ClearElrRecord is created even when the min.insync.replicas value remains unchanged. Perhaps we could either document the behavior or consider skipping the record generation in such case.

Good point. Added:
If the cluster-level <code>min.insync.replicas</code> is updated, even if the value is unchanged, all the ELR state will be cleaned.

junrao

@showuon : Thanks for the updated PR. A couple of more comments.

junrao · 2025-08-01T16:30:28Z

docs/ops.html

+  <p>Note that when the ELR feature is enabled:</p>
+  <ul>
+    <li>The cluster-level <code>min.insync.replicas</code> config will be added if there is not any. The value is the same as the static config in the active controller.</li>
+    <li>The removal of <code>min.insync.replicas</code> config in cluster-level is not allowed.</li>


in cluster-level => at cluster-level Ditto below.

Updated in the commit: 7095773.

junrao · 2025-08-01T16:34:55Z

docs/ops.html

+    <li>The cluster-level <code>min.insync.replicas</code> config will be added if there is not any. The value is the same as the static config in the active controller.</li>
+    <li>The removal of <code>min.insync.replicas</code> config in cluster-level is not allowed.</li>
+    <li>If the cluster-level <code>min.insync.replicas</code> is updated, even if the value is unchanged, all the ELR state will be cleaned.</li>
+    <li>The previously set <code>min.insync.replicas</code> value at the broker-level config will be removed. Please set in the cluster-level if necessary.</li>


Should we point out that users may need to take some actions with ELR enabled in upgrade.html too?

Good point! In the Notable changes in 4.1.0 section, added:

The KIP-966 part 1: Eligible Leader Replicas(ELR) will be enabled by default on the new clusters. After the ELR feature enabled, the previously set min.insync.replicas value at the broker-level config will be removed. Please set at the cluster-level if necessary. For further details, please refer to here.

mimaison

LGTM

junrao

@showuon : Thanks for the updated PR. LGTM

chia7712 · 2025-08-04T16:22:31Z

@mimaison Should we backport it to 4.1?

mimaison · 2025-08-04T16:25:23Z

Yes this needs backporting to 4.1. Go ahead if you can, otherwise I'll do it tomorrow morning my time. Thanks!

Along with the change: #17952 ([KIP-966](https://cwiki.apache.org/confluence/display/KAFKA/KIP-966%3A+Eligible+Leader+Replicas)), the semantics of `min.insync.replicas` config has small change, and add some constraints. We should document them clearly. Reviewers: Jun Rao <[email protected]>, Calvin Liu <[email protected]>, Mickael Maison <[email protected]>, Paolo Patierno <[email protected]>, Federico Valeri <[email protected]>, Chia-Ping Tsai <[email protected]>

chia7712 · 2025-08-04T16:30:01Z

Yes this needs backporting to 4.1. Go ahead if you can, otherwise I'll do it tomorrow morning my time. Thanks!

done cdc7a4e

Along with the change: apache#17952 ([KIP-966](https://cwiki.apache.org/confluence/display/KAFKA/KIP-966%3A+Eligible+Leader+Replicas)), the semantics of `min.insync.replicas` config has small change, and add some constraints. We should document them clearly. Reviewers: Jun Rao <[email protected]>, Calvin Liu <[email protected]>, Mickael Maison <[email protected]>, Paolo Patierno <[email protected]>, Federico Valeri <[email protected]>, Chia-Ping Tsai <[email protected]>

improve the min.insync.replicas doc

b8b4ff4

github-actions bot added kraft clients small Small PRs labels Jul 24, 2025

showuon mentioned this pull request Jul 24, 2025

[Bug]: Endless brokers pods restarts when changing min.insync.replicas with Eligible Leader Replicas (ELR) feature enabled strimzi/strimzi-kafka-operator#11685

Closed

ppatierno approved these changes Jul 24, 2025

View reviewed changes

fix checkstyle error

8bc3747

fvaleri approved these changes Jul 25, 2025

View reviewed changes

mimaison self-requested a review July 30, 2025 13:41

mimaison mentioned this pull request Jul 30, 2025

MINOR: update elr doc for the min.isr config update rule #20268

Closed

CalvinConfluent approved these changes Jul 30, 2025

View reviewed changes

chia7712 reviewed Jul 30, 2025

View reviewed changes

docs/ops.html Outdated Show resolved Hide resolved

mimaison reviewed Jul 30, 2025

View reviewed changes

CalvinConfluent reviewed Jul 30, 2025

View reviewed changes

docs/ops.html Outdated Show resolved Hide resolved

update the doc

eb4bbb5

junrao reviewed Jul 31, 2025

View reviewed changes

chia7712 reviewed Jul 31, 2025

View reviewed changes

improve the doc

b9cc4ff

junrao reviewed Aug 1, 2025

View reviewed changes

update the doc

7095773

mimaison approved these changes Aug 4, 2025

View reviewed changes

junrao approved these changes Aug 4, 2025

View reviewed changes

chia7712 approved these changes Aug 4, 2025

View reviewed changes

chia7712 merged commit 657b496 into apache:trunk Aug 4, 2025
27 checks passed

MINOR: improve the min.insync.replicas doc #20237

MINOR: improve the min.insync.replicas doc #20237

Uh oh!

Conversation

showuon commented Jul 24, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

showuon commented Jul 24, 2025

Uh oh!

ppatierno left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Jul 24, 2025

Uh oh!

fvaleri left a comment

Choose a reason for hiding this comment

Uh oh!

showuon commented Jul 28, 2025

Uh oh!

mimaison commented Jul 29, 2025

Uh oh!

CalvinConfluent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

junrao left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrao left a comment

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!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chia7712 commented Aug 4, 2025

Uh oh!

mimaison commented Aug 4, 2025

Uh oh!

chia7712 commented Aug 4, 2025

Uh oh!

showuon commented Jul 24, 2025 •

edited by github-actions bot

Loading