Kafka-14420 Use incrementalAlterConfigs API for syncing topic configurations (KIP-894)

[tinaselenge]

Implements https://cwiki.apache.org/confluence/display/KAFKA/KIP-894%3A+Use+incrementalAlterConfigs+API+for+syncing+topic+configurations

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[mimaison]

Thanks for the PR! I left a few questions and suggestions.

[C0urante]

Thanks for the PR!

Haven't made a complete pass yet (probably not worth it until Mickael's comments are addressed), but wanted to leave a few thoughts that don't appear to conflict with any of the review comments left so far.

[tinaselenge]

@mimaison @C0urante Thank you so much for reviewing the PR. I think I have addressed all the review comments, please let me know if I have missed anything or you have any further comments.

The jenkins build has failed due to mostly unrelated tests, except one integration test for mirrormaker, MirrorConnectorsIntegrationSSLTest > testReplication() FAILED. However, this test passes on my local machine.

[C0urante]

Thanks Tina! Left a few more comments.

I think for some of the unhandled cases here, it would also be nice to add testing coverage to ensure that things are working as expected now and that we don't regress in the future. FWIW, I still haven't gotten to look too much at the testing changes; I'm assuming that any logical errors here aren't covered or are covered incorrectly, since otherwise the tests would be failing.

[C0urante]

Thanks @tinaselenge, this is looking better. I think there are a few comments from the last round that went by unaddressed; I've resolved any that I think still need to be touched on.

[tinaselenge]

@C0urante Sorry, it was left in incomplete state. I have been meaning to get back to it and address the remaining comments. I think I have now addressed everything. Thank you so much for reviewing the PR again. Please let me know if I still missed something or have further comments.

[C0urante]

@tinaselenge I owe you a big apology!

I think the API you had for the ConfigPropertyFilter interface was correct the first time around.

The double-check logic is necessary because we need to distinguish between three possible cases when incrementally altering topic configs:

0. A property with a default value on the source topic should be synced to the target topic (using a SET operation in the config alter request)
1. A property with a default value on the source topic should use the default value on the target topic (which is accomplished via a DELETE operation in the config alter request)
2. A property with a default value on the source topic should not be altered on the target topic at all

If we only rely on ConfigPropertyFilter::shouldReplicateSourceDefault, that doesn't give us enough information to choose between those three options (since the return value of that method can only have two different values).

Instead, we can and should use both methods to decide how to act (for some property that has a default value on the source cluster):

0. If both ConfigPropertyFilter::shouldReplicateConfigProperty and ConfigPropertyFilter::shouldReplicateSourceDefault return true, then we sync the property to the target cluster using SET
1. If ConfigPropertyFilter::shouldReplicateConfigProperty returns true but ConfigPropertyFilter::shouldReplicateSourceDefault returns false, then we sync the property to the target cluster using DELETE
2. If ConfigPropertyFilter::shouldReplicateConfigProperty returns false, we do not sync the property at all

So I've come to realize that the way you'd originally implemented this is actually the only correct way to satisfy the design outlined in the KIP.

I'm very sorry for wasting your time on this wild goose chase, please accept my apologies!

[tinaselenge]

@C0urante No worries at all! I should have clarified it better. I think both logics work but perhaps the double logic in the targetConfig() is better since we can't rely solely on shouldReplicateSourceDefault as you said. I reverted that part but also added some comments explaining the conditions that we are filtering against, hopefully that makes sense.

[C0urante]

Thanks @tinaselenge! This is looking great, almost ready to merge.

[C0urante]

LGTM, thanks @tinaselenge!

(Sorry for the delay; took last week off 🌴)

[ijuma]

It looks like we started getting a lot of test failures after this PR:

https://ci-builds.apache.org/job/Kafka/job/kafka/job/trunk/1746/#showFailuresLink

I see a lot of the same failures in this PR build:

https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13373/16/#showFailuresLink

@C0urante did you triage these failures before merging?

[gharris1727]

Ticket for the failures introduced here: https://issues.apache.org/jira/browse/KAFKA-14905

Some cursory investigation reveals that these tests hook into alterConfigs via FakeForwardingAdminWithLocalMetadata but don't hook the corresponding incrementalAlterConfigs. However, I cannot confirm this as the tests are consistently passing locally with or without that fix. I'll open a tentative fix PR and see if the test then passes on Jenkins.

[ijuma]

Thanks for helping with this @gharris1727.