KAFKA-19931 Align broker and controller behavior for the Admin.incrementalAlterConfigs API #21005

m1a2st · 2025-11-27T11:12:11Z

This PR updates the behavior of Admin.incrementalAlterConfigs() to be
consistent when invoked using bootstrap servers versus bootstrap
controllers. The following changes apply specifically when using a
bootstrap controller:

Null Config Values (DELETE Only)
A null config value is allowed only when used with
AlterConfigOp.OpType.DELETE.
Supplying null with any other operation type will now result in an
InvalidRequestException.
Duplicate Config Names
Providing duplicate configuration names within the same resource will
now fail with an InvalidRequestException.
Unknown Resource Types
Requests containing unknown resource types will now fail with an
InvalidRequestException.

These changes ensure the controller-based Admin API behaves consistently
and predictably, aligning with broker-side validation rules.

FYI: #20960

chia7712

@m1a2st could you take a look at the failed test testInvalidIncrementalAlterConfigsResources?

junrao

@m1a2st : Thanks for the PR. Left a comment.

junrao · 2025-12-01T19:30:34Z

core/src/main/scala/kafka/server/ControllerApis.scala

-          altersByName.put(config.name, new util.AbstractMap.SimpleEntry[AlterConfigOp.OpType, String](
-            AlterConfigOp.OpType.forId(config.configOperation), config.value))
+        if (!nullUpdates.isEmpty) {
+          throw new InvalidRequestException("Null value not supported for : " +


Would it be better to just set the error response here instead of throwing?

junrao · 2025-12-01T21:44:11Z

core/src/main/scala/kafka/server/ControllerApis.scala

-        } catch {
-          case t: Throwable => ApiError.fromThrowable(t)
+      try {
+        val nullUpdates = new util.ArrayList[String]()


Also, there are quite a few pre-processing checks in ConfigAdminManager. The doc says the following. Could we restructure the code between ConfigAdminManager and ControllerApis to (1) avoid duplicates logic in verification (2) prevent missing verification in one of the two places in the future?

// BROKER_LOGGER requests always go to a specific, constant broker or controller node. // // BROKER resource changes for a specific (non-default) resource go to either that specific // node (if using bootstrap.servers), or directly to the active controller (if using // bootstrap.controllers) // // All other requests go to the least loaded broker (if using bootstrap.servers) or the // active controller (if using bootstrap.controllers)

chia7712 · 2025-12-03T11:40:15Z

core/src/main/scala/kafka/server/ControllerApis.scala

+              resource)
+            ApiError.NONE
+          } catch {
+            case t: Throwable => ApiError.fromThrowable(t)


Why not throwing the exception directly? The onError method should handle it, right?

m1a2st · 2025-12-03T16:13:39Z

core/src/test/scala/unit/kafka/server/ControllerApisTest.scala

        setResourceType(ConfigResource.Type.BROKER.id()),
      new AlterConfigsResourceResponse().
-        setErrorCode(UNSUPPORTED_VERSION.code()).
+        setErrorCode(INVALID_REQUEST.code()).


The controller error code should match the broker error code.

chia7712

@m1a2st would you mind updating the upgrade.html to inform users about this behaviour change?

chia7712 · 2025-12-08T08:55:30Z

core/src/main/scala/kafka/server/ControllerApis.scala

+    configChanges: util.HashMap[ConfigResource, util.Map[String, Entry[AlterConfigOp.OpType, String]]],
+    response: IncrementalAlterConfigsResponseData
+  ): Unit = {
+    if (!duplicateResources.contains(configResource)) {


Could we use configChanges instead?

I meant that the configChanges collection could be used to check the duplicates, right?

The old logic worked in three steps:

The first occurrence of a configResource is added to the map.

The second occurrence of the same configResource is removed from the map, added to duplicateValue, and written to the response.

The third and any subsequent occurrences of that configResource are ignored entirely.

Given this behavior, I think we cannot rely solely on a map to implement this logic.

chia7712 · 2025-12-08T14:17:34Z

@m1a2st I noticed another inconsistent behavior.

The following code fails on the broker but succeeds on the controller.

  admin.incrementalAlterConfigs(Map.of(
                    new ConfigResource(ConfigResource.Type.TOPIC, "chia"),
                    List.of(new AlterConfigOp(new ConfigEntry("cleanup.policy", "compact"), OpType.SET),
                            new AlterConfigOp(new ConfigEntry("cleanup.policy", "compact"), OpType.SET),
                            new AlterConfigOp(new ConfigEntry("flush.ms", "11000"), OpType.SET))
            )

chia7712 · 2025-12-08T14:33:08Z

The following code fails on the broker but succeeds on the controller.

It appears this behaviour gets fixed also. @m1a2st would you mind updating the PR description and add the integration test ?

junrao

@m1a2st : Thanks for the PR. Since this is a behavior change, it's probably better to do a KIP so that more people are aware of it.

m1a2st · 2025-12-15T12:17:38Z

Thanks for @junrao comment, I have started a discussion thread.

FYI: https://lists.apache.org/thread/lqn0xw0zdfwl9m990otjvswzqh5bmy8v

chia7712 · 2025-12-19T17:22:33Z

@m1a2st I noticed another inconsistency related to this patch. The controller does not validate the dynamic configurations when handling requests, which results in a misleading success response. By contrast, the broker checks the dynamic configurations immediately upon receiving the request, allowing users to catch invalid requests early. Would you mind fixing this as well?

# Conflicts: # docs/upgrade.html

m1a2st · 2025-12-20T13:35:57Z

I noticed another inconsistency related to this patch. The controller does not validate the dynamic configurations when handling requests, which results in a misleading success response. By contrast, the broker checks the dynamic configurations immediately upon receiving the request, allowing users to catch invalid requests early. Would you mind fixing this as well?

I also added this inconsistent behavior to the KIP, and I can align it as well.

completed the feature

1fcfe35

github-actions bot added triage PRs from the community core Kafka Broker labels Nov 27, 2025

updated the test

cc33873

chia7712 reviewed Nov 27, 2025

View reviewed changes

fix test

f5f78e7

github-actions bot removed the triage PRs from the community label Nov 28, 2025

m1a2st changed the title ~~KAFKA-19931 inconsistency in handling the null config value [WIP]~~ KAFKA-19931 inconsistency in handling the null config value Nov 28, 2025

junrao reviewed Dec 1, 2025

View reviewed changes

m1a2st added 4 commits December 2, 2025 18:43

Merge branch 'trunk' into KAFKA-19931

59d3be2

temp version

c8d913f

temp version

f8d01df

test in the kafka CI

a165eb5

chia7712 reviewed Dec 3, 2025

View reviewed changes

m1a2st added 4 commits December 3, 2025 20:57

Merge branch 'trunk' into KAFKA-19931

48184c2

addressed by comments

1ff2a4c

fix the fail test

da21c8b

add new test for ConfigAdminManagerTest

fdbb81c

m1a2st commented Dec 3, 2025

View reviewed changes

chia7712 reviewed Dec 8, 2025

View reviewed changes

m1a2st added 4 commits December 8, 2025 19:52

Merge branch 'trunk' into KAFKA-19931

a0198d9

addressed by comments

d6ae031

update the addConfigChangesOrHandleDuplicate logic

138e8e8

update the related logic

72e787b

m1a2st added 2 commits December 8, 2025 22:33

revert the error logic

bfd3860

remove unused exception throwing

2a4c9ea

m1a2st changed the title ~~KAFKA-19931 inconsistency in handling the null config value~~ KAFKA-19931 Inconsistency in handling the null config value, duplicate value, error code Dec 8, 2025

junrao reviewed Dec 12, 2025

View reviewed changes

m1a2st added 2 commits December 20, 2025 20:55

Merge branch 'trunk' into KAFKA-19931

d388063

# Conflicts: # docs/upgrade.html

resolve conflict

6df68ba

m1a2st changed the title ~~KAFKA-19931 Inconsistency in handling the null config value, duplicate value, error code~~ KAFKA-19931 Align broker and controller behavior for the Admin.incrementalAlterConfigs API Dec 20, 2025

m1a2st added 12 commits December 21, 2025 00:16

add new validator

bb8c33a

fix the fail test

16ca216

temp

ca12d22

temp patch

ac4b124

revert unused change

b07102e

Merge branch 'trunk' into KAFKA-19931

500b7ae

check the resource operation type

8d1cf15

check null value

d896bfb

check delete opType

a690de7

Merge branch 'trunk' into KAFKA-19931

e7251e2

Merge branch 'trunk' into KAFKA-19931

374c301

update the logic

59264ff

KAFKA-19931 Align broker and controller behavior for the Admin.incrementalAlterConfigs API #21005

Are you sure you want to change the base?

KAFKA-19931 Align broker and controller behavior for the Admin.incrementalAlterConfigs API #21005

Uh oh!

Conversation

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

Uh oh!

Uh oh!

chia7712 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!

junrao Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

junrao Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

chia7712 Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

m1a2st Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

chia7712 Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

m1a2st Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Dec 8, 2025

Uh oh!

chia7712 commented Dec 8, 2025

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

m1a2st commented Dec 15, 2025

Uh oh!

chia7712 commented Dec 19, 2025

Uh oh!

m1a2st commented Dec 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

m1a2st commented Nov 27, 2025 •

edited by github-actions bot

Loading