Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sets ConfigEntry.Default flag in addition to the ConfigEntry.Source for Kafka versions > V1_1_0_0 #1594

Merged
merged 4 commits into from
Feb 21, 2020

Conversation

sladkoff
Copy link
Contributor

We were relying on the ConfigEntry.Default field to filter out any default config settings when using admin.DescribeConfig().

After updating to the most recent version of sarama, this flag is always set to false with Kafka versions > V1_1_0_0.

We found out that we can now use the ConfigEntry.Source to check for defaults. I don't know it was intended change to "break" the ConfigEntry.Default in higher versions - let me know.

This change sets the ConfigEntry.Default flag in addition to the ConfigEntry.Source for Kafka versions > V1_1_0_0.

I haven't written any tests yet as I'm not familiar with the code base. I can add some later.

@sladkoff sladkoff requested a review from bai as a code owner January 31, 2020 15:06
@ghost ghost added the cla-needed label Jan 31, 2020
… DescribeConfig

This breaks the output of ListTopics for newer request versions, it now includes default configuration settings.
Clients can now rely on the `Default` flag again and don't have to check the `Source` for higher Kafka versions.
@sladkoff sladkoff force-pushed the feature/fix-topic-config-default-flag branch from 51ba5d7 to 1b5e305 Compare January 31, 2020 15:44
@bai bai requested review from varun06 and d1egoaz February 4, 2020 10:22
@varun06
Copy link
Contributor

varun06 commented Feb 5, 2020

@sladkoff you might have to sign CLA.

@sladkoff
Copy link
Contributor Author

sladkoff commented Feb 5, 2020

@varun06 yeah, I did that already... the check is still in a failed state and I can not re-submit the CLA form because it says that I already signed it... Can you help with that? Maybe reset something?

@ghost ghost removed the cla-needed label Feb 5, 2020
@d1egoaz
Copy link
Contributor

d1egoaz commented Feb 5, 2020

I've re-run the CLA check, it's OK now

Copy link
Contributor

@d1egoaz d1egoaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sladkoff LGTM, but please add some tests before we can merge this

Copy link
Contributor

@varun06 varun06 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will like to see some tests around this change please.

@sladkoff
Copy link
Contributor Author

Just letting you know that I've added some tests. Are we good here?

@dnwe dnwe changed the title Fix returned topic config default flag Sets ConfigEntry.Default flag in addition to the ConfigEntry.Source for Kafka versions > V1_1_0_0 Feb 21, 2020
@dnwe dnwe merged commit 6d92277 into IBM:master Feb 21, 2020
ronanh pushed a commit to ronanh/sarama that referenced this pull request Mar 4, 2020
…or Kafka versions > V1_1_0_0 (IBM#1594)

* Set describeConfigsRequest.Version in ListTopics for consistency with DescribeConfig

This breaks the output of ListTopics for newer request versions, it now includes default configuration settings.

* Set ConfigEntry.Default for KafkaVersions > 0

Clients can now rely on the `Default` flag again and don't have to check the `Source` for higher Kafka versions.

* Set ConfigEntry.Source to default for KafkaVersions <= 0 when applicable

* Add tests for default flag/source
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants