KAFKA-14919 sync topic configs test

[anton-liauchuk]

KAFKA-14919 sync topic configs test

[anton-liauchuk]

@gharris1727

Please take a look.

[gharris1727]

Hi @anton-liauchuk Thanks for the PR!

Is the KAFKA ticket linked the right one? I don't think this implementation matches the description of that ticket. I expected changes to the FakeLocalMetadataStore to be necessary.

[anton-liauchuk]

@gharris1727

Hi @anton-liauchuk Thanks for the PR!

Is the KAFKA ticket linked the right one? I don't think this implementation matches the description of that ticket. I expected changes to the FakeLocalMetadataStore to be necessary.

From the task description:

These tests and/or the metadata store should be changed so that the tests are isolated from one another, and actually perform the assertions that correspond to their titles.

I made changes to the tests to ensure they assert a specific operation. Before my changes, the test
org.apache.kafka.connect.mirror.integration.MirrorConnectorsWithCustomForwardingAdminIntegrationTest#testReplicationIsCreatingTopicsUsingProvidedForwardingAdmin might have passed due to refresh.topics.enabled flag and related configuration. After the new changes, this test asserts the behavior of org.apache.kafka.connect.mirror.MirrorSourceConnector#computeAndCreateTopicPartitions without depending on other functionality.

The test org.apache.kafka.connect.mirror.integration.MirrorConnectorsWithCustomForwardingAdminIntegrationTest#testSyncTopicConfigUseProvidedForwardingAdmin did not properly check the sync functionality because there were no configs for the topic and no related assertions. With the recent changes, I updated the test to check the retention.bytes config to ensure that it was properly synced. To accomplish this, I had to implement org.apache.kafka.connect.mirror.clients.admin.FakeForwardingAdminWithLocalMetadata#incrementalAlterConfigs to store new configs during the sync.

For now, it seems that these two tests were the main issue in this test class. With the new changes, each test will assert only one particular functionality. Do you think we should also change the metadata store?

[gharris1727]

Okay thank you for the clarification @anton-liauchuk . While avoiding confusing one operation for another by turning off all-but-one operation avoids the ambiguity, it still leaves the opportunity for ambiguity to appear in the future. If we are avoiding certain situations due to ambiguity, that reduces our total test coverage.

Also I think testCreatePartitionsUseProvidedForwardingAdmin remains ambiguous, at least from re-reading my original comment: #13575 (comment)

I had to implement incrementalAlterConfigs

Thanks, that definitely needed to be done. FakeLocalMetadataStore should have the granularity to assert that we used incremental vs the legacy API, and it doesn't right now. That would have caught this mistake earlier when MM2 support for the new API was added.

Please refactor the FakeLocalMetadata store to make the ambiguity impossible, thanks!

[anton-liauchuk]

@gharris1727

Please take a look.

[github-actions]

This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.

If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact).

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

[github-actions]

This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.