kevin-wu24 commented on code in PR #21053:
URL: https://github.com/apache/kafka/pull/21053#discussion_r2678036560
##########
metadata/src/main/java/org/apache/kafka/image/ConfigurationDelta.java:
##########
@@ -56,19 +70,28 @@ public void deleteAll() {
}
public ConfigurationImage apply() {
+ Set<String> whitelist = VALID_CONFIGS_REF.get();
Review Comment:
You can look at `MetadataLoader#handleLoadSnapshot` for how the metadata
publishing pipeline (which generates the deltas we are working with here) deals
with a snapshot received from the KRaft layer. Since the `SnapshotGenerator` is
a metadata publisher, the next snapshot that is written to disk after the
"cleanup" code executes should no longer have dynamic configs unknown to kafka.
However, in order for the active controller to successfully handle an alter
config request after updating its software, but before it loads a "cleaned"
snapshot, I think we still need to clean up the `ConfigurationControlManager`'s
state, because its `configData` is the "dynamic config state of truth" for
handling RPCs, not whatever exists in the `MetadataLoader`. Controller state
for RPC handling uses `QuoumController#QuorumMetaLogListener` to load snapshots
from KRaft, which is separate code from `MetadataLoader`.
I think I made an observation before that it seemed incorrect to do this
kind of metadata state modification without explicitly committing records to
KRaft. The `MetadataLoader` and `QuoumController#QuorumMetaLogListener` are
both raft listeners, with each being responsible for different things (the
former is responsible for generating the next snapshot on-disk, amongst other
things, and the latter is indirectly responsible, via the
`ConfigurationControlManager`, for handling alter config requests). Should we
instead commit an explicit "delete these X unknown dynamic config records" to
KRaft alongside the user's requested alter config upon receiving an alter
config request that would otherwise be invalid because of unknown configs? This
would avoid code duplication in the listeners.
@ahuang98 @0xffff-zhiyan @jsancio Let me know what you think of this
approach, or if I am misunderstanding something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
