Repository navigation
fix(inkless:controller): skip unchanged control plane connection string record - #852
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The legacyAlterConfigs path lacks regression coverage for suppressing unchanged control-plane connection strings while preserving records for other unchanged broker configs and implicit deletion when the connection string is omitted.
1 open issue
What changed in this PR
This PR reduces redundant controller metadata records when clients reapply an unchanged Inkless control-plane connection string, while preserving reload behavior for other broker configs.
Changes:
- Skip unchanged control-plane connection string records in both incremental and legacy config updates.
- Add incremental-update tests for unchanged and changed connection strings, other broker configs, and topic configs.
| File | Description |
|---|---|
metadata/src/test/java/org/apache/kafka/controller/ConfigurationControlManagerTest.java |
Add regression tests for incremental config record generation. |
metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java |
Exempt the unchanged control-plane connection string from broker reload records in both update paths. |
Review effort: Balanced
jeqo
force-pushed
the
jeqo/fix-cp-config
branch
from
November 4, 2026 14:10
bfe48e1 to
f7b0012
Compare
jeqo
marked this pull request as ready for review
November 4, 2026 14:16
gqmelo
reviewed
Nov 4, 2026
…ng record `incrementalAlterConfig` and `legacyAlterConfigs` emit a `ConfigRecord` for every `SET` on a `BROKER` resource, even when the value is unchanged. KAFKA-14136 added this so brokers reload an SSL keystore or truststore file that changed on disk. A client that re-applies `inkless.control.plane.connection.string` on a timer therefore appends one metadata record per tick. The reason for KAFKA-14136 does not apply to this key: - It is not one of the reloadable file configs (`ssl.keystore.location`, `ssl.truststore.location`). - It is valid only on the default broker resource, and the file reload runs only for per-broker resources. - On the default resource, a broker diffs the replayed properties against its current config and calls no reconfigurable when nothing changed (`DynamicBrokerConfig.processReconfiguration`). An unchanged record is already a no-op. Skip the record for this key only, in both alter paths. Other `BROKER` configs, including the read and write connection strings, keep emitting a record. Topic configs already skip unchanged values. `DynamicBrokerConfigTest` pins that re-applying an unchanged value does not reconfigure. Assisted-by: Claude Sonnet 5.5
jeqo
force-pushed
the
jeqo/fix-cp-config
branch
from
November 4, 2026 15:07
61e443d to
0eb93ea
Compare
gqmelo
approved these changes
Nov 4, 2026
gqmelo
pushed a commit
that referenced
this pull request
Nov 5, 2026
…ng record (#852) `incrementalAlterConfig` and `legacyAlterConfigs` emit a `ConfigRecord` for every `SET` on a `BROKER` resource, even when the value is unchanged. KAFKA-14136 added this so brokers reload an SSL keystore or truststore file that changed on disk. A client that re-applies `inkless.control.plane.connection.string` on a timer therefore appends one metadata record per tick. The reason for KAFKA-14136 does not apply to this key: - It is not one of the reloadable file configs (`ssl.keystore.location`, `ssl.truststore.location`). - It is valid only on the default broker resource, and the file reload runs only for per-broker resources. - On the default resource, a broker diffs the replayed properties against its current config and calls no reconfigurable when nothing changed (`DynamicBrokerConfig.processReconfiguration`). An unchanged record is already a no-op. Skip the record for this key only, in both alter paths. Other `BROKER` configs, including the read and write connection strings, keep emitting a record. Topic configs already skip unchanged values. `DynamicBrokerConfigTest` pins that re-applying an unchanged value does not reconfigure. Assisted-by: Claude Sonnet 5.5
gqmelo
added a commit
that referenced
this pull request
Nov 5, 2026
On 4.3, `incrementalAlterConfig`, `legacyAlterConfigs`, `ReplicationControlManager#createTopics`, `Controller#createTopics`, and `ControllerApis#createTopics` take a `forwarded` flag, and `MetadataDelta` has no public constructor, only `MetadataDelta.Builder`. The tests added by #832, #835, #839, #845, #846, and #852 used the main signatures. Assisted-by: Claude Opus 5.5
gqmelo
pushed a commit
that referenced
this pull request
Nov 5, 2026
…ng record (#852) `incrementalAlterConfig` and `legacyAlterConfigs` emit a `ConfigRecord` for every `SET` on a `BROKER` resource, even when the value is unchanged. KAFKA-14136 added this so brokers reload an SSL keystore or truststore file that changed on disk. A client that re-applies `inkless.control.plane.connection.string` on a timer therefore appends one metadata record per tick. The reason for KAFKA-14136 does not apply to this key: - It is not one of the reloadable file configs (`ssl.keystore.location`, `ssl.truststore.location`). - It is valid only on the default broker resource, and the file reload runs only for per-broker resources. - On the default resource, a broker diffs the replayed properties against its current config and calls no reconfigurable when nothing changed (`DynamicBrokerConfig.processReconfiguration`). An unchanged record is already a no-op. Skip the record for this key only, in both alter paths. Other `BROKER` configs, including the read and write connection strings, keep emitting a record. Topic configs already skip unchanged values. `DynamicBrokerConfigTest` pins that re-applying an unchanged value does not reconfigure. Assisted-by: Claude Sonnet 5.5
gqmelo
added a commit
that referenced
this pull request
Nov 5, 2026
On 4.2, `MetadataDelta` has no public constructor, only `MetadataDelta.Builder`. `HostedPartition.Online` and `HostedPartition.Offline` take no type parameter, `MemoryRecords` and `SimpleRecord` are in `org.apache.kafka.common.record`, and the Scala `BrokerReconfigurable` returns a Scala set. The tests added by #835, #840, #841, #850, and #852 used the main signatures. The Java `UnifiedLogTest` on 4.2 is a stub, and the log tests still live in the Scala `kafka.log.UnifiedLogTest`. The cherry-pick of #840 ports its four `forceDeleteLocalSegmentsBelow` tests to the Scala suite instead of pulling the upstream Java tests in through the conflict. Assisted-by: Claude Opus 5.5
gqmelo
pushed a commit
that referenced
this pull request
Nov 5, 2026
…ng record (#852) `incrementalAlterConfig` and `legacyAlterConfigs` emit a `ConfigRecord` for every `SET` on a `BROKER` resource, even when the value is unchanged. KAFKA-14136 added this so brokers reload an SSL keystore or truststore file that changed on disk. A client that re-applies `inkless.control.plane.connection.string` on a timer therefore appends one metadata record per tick. The reason for KAFKA-14136 does not apply to this key: - It is not one of the reloadable file configs (`ssl.keystore.location`, `ssl.truststore.location`). - It is valid only on the default broker resource, and the file reload runs only for per-broker resources. - On the default resource, a broker diffs the replayed properties against its current config and calls no reconfigurable when nothing changed (`DynamicBrokerConfig.processReconfiguration`). An unchanged record is already a no-op. Skip the record for this key only, in both alter paths. Other `BROKER` configs, including the read and write connection strings, keep emitting a record. Topic configs already skip unchanged values. `DynamicBrokerConfigTest` pins that re-applying an unchanged value does not reconfigure. Assisted-by: Claude Sonnet 5.5
gqmelo
added a commit
that referenced
this pull request
Nov 5, 2026
On 4.1, `RemoteStorageFetchInfo` carries a `TopicPartition` instead of a `TopicIdPartition`, so the remote fetch callback in `ReplicaManager` looks up the topic ID with `topicIdPartition`. `LogReadResult` takes no `Errors` argument, `RetriableRemoteStorageException` doesn't exist and the reclaim floor throws `RemoteStorageException`, and `remote.log.manager.thread.pool.size` sizes the follower pool. `HostedPartition.Online` and `HostedPartition.Offline` take no type parameter, `MemoryRecords` and `SimpleRecord` are in `org.apache.kafka.common.record`, and the Scala `BrokerReconfigurable` returns a Scala set. The changes added by #831, #834, #835, #840, #841, #847, #850, and #852 used the main signatures. The Java `UnifiedLogTest` on 4.1 is a stub, and the log tests still live in the Scala `kafka.log.UnifiedLogTest`. The cherry-pick of #840 ports its four `forceDeleteLocalSegmentsBelow` tests to the Scala suite instead of pulling the upstream Java tests in through the conflict. After #847, `kafka.server.ReplicaManager` on 4.1 is too big for SpotBugs to analyze, and `:core:spotbugsMain` fails with `SKIPPED_CLASS_TOO_BIG`. The class is about 400 lines longer on 4.1 than on 4.2 and 4.3, and SpotBugs 4.9.8 fails the same way. The exclude filter now skips that pattern for `ReplicaManager` only. Assisted-by: Claude Opus 5.5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
incrementalAlterConfigandlegacyAlterConfigsemit aConfigRecordfor everySETon aBROKERresource, even when the value is unchanged. KAFKA-14136 added this so brokers reload an SSL keystore or truststore file that changed on disk. A client that re-appliesinkless.control.plane.connection.stringon a timer therefore appends one metadata record per tick.The reason for KAFKA-14136 does not apply to this key:
ssl.keystore.location,ssl.truststore.location).DynamicBrokerConfig.processReconfiguration). An unchanged record is already a no-op.Skip the record for this key only, in both alter paths. Other
BROKERconfigs, including the read and write connection strings, keep emitting a record. Topic configs already skip unchanged values.DynamicBrokerConfigTestpins that re-applying an unchanged value does not reconfigure.