Skip to content

KAFKA-10000: Add all public-facing config properties related to exactly-once source support (KIP-618)#11775

Merged
showuon merged 1 commit intoapache:trunkfrom
C0urante:kafka-10000-config-changes
May 12, 2022
Merged

KAFKA-10000: Add all public-facing config properties related to exactly-once source support (KIP-618)#11775
showuon merged 1 commit intoapache:trunkfrom
C0urante:kafka-10000-config-changes

Conversation

@C0urante
Copy link
Copy Markdown
Contributor

Adds the new connector- and worker-level properties described in KIP-618.

Relies on changes from:

Note that these properties have no effect on the behavior of the Connect framework (beyond some rudimentary config validation checks) in this PR. The behavioral changes for these properties will be implemented in downstream PRs.

@C0urante C0urante changed the title KAFKA-10000: Add all public-facing config properties related to exactly-once source support KAFKA-10000: Add all public-facing config properties related to exactly-once source support (KIP-618) Feb 17, 2022
@C0urante C0urante marked this pull request as draft February 18, 2022 06:34
@C0urante
Copy link
Copy Markdown
Contributor Author

Converting to draft until upstream PRs are reviewed.

@C0urante C0urante force-pushed the kafka-10000-config-changes branch from c014300 to 69d63ed Compare February 25, 2022 15:54
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not for this PR, but looking at this made me realise that ConfigDef could benefit from a Validator specifically for enums. There's currently inconsistency around case sensitivity, for instance. And using enumOptions more widely would simplify other call sites which typically list all the enum members. Wdyt?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Definitely agree! Some of the rough edges around, e.g., the consumer isolation.level property and its case-sensitive parsing have been a little troublesome. It'd be great to abstract+simplify some of the logic there and also add some user-facing consistency with how these types of properties are parsed.

KIP-713 may be the place to discuss this kind of effort. It's unclear how much we really care about protecting the various out-of-the-box Validator instances that we ship as "public interface", but I chose to err on the side of caution here since the second we add one for enums, people are going to start using it, so we'd better get it right the first time. This seems like exactly the sort of thing that the KIP process helps facilitate: making careful, measure-twice-cut-once changes to public-facing parts of the project.

@C0urante C0urante force-pushed the kafka-10000-config-changes branch from c0254da to 48aad48 Compare March 3, 2022 16:51
@C0urante C0urante marked this pull request as ready for review March 3, 2022 16:51
@C0urante
Copy link
Copy Markdown
Contributor Author

C0urante commented Mar 3, 2022

Marking ready for review since #11773, though not yet merged, has been approved.

Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

@C0urante , thanks for the PR. Left some comments. Thanks.

Comment thread connect/api/src/main/java/org/apache/kafka/connect/source/SourceConnector.java Outdated
Comment thread connect/api/src/main/java/org/apache/kafka/connect/source/SourceConnector.java Outdated
Comment thread connect/api/src/main/java/org/apache/kafka/connect/source/SourceTask.java Outdated
Comment thread connect/api/src/main/java/org/apache/kafka/connect/source/SourceTaskContext.java Outdated
Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM!

@showuon
Copy link
Copy Markdown
Member

showuon commented Apr 13, 2022

@C0urante , there's checkstyle error. Please help fix. Thanks.

@C0urante
Copy link
Copy Markdown
Contributor Author

Sorry @showuon, should be good to go now.

@C0urante
Copy link
Copy Markdown
Contributor Author

@showuon is there anything left you'd like me to address?

@showuon
Copy link
Copy Markdown
Member

showuon commented Apr 19, 2022

@C0urante , thanks for the reminder!
@tombentley , do you have any other comments for this PR?
Thanks.

@C0urante C0urante force-pushed the kafka-10000-config-changes branch from f28d8f8 to 2cf98db Compare May 10, 2022 11:52
@C0urante
Copy link
Copy Markdown
Contributor Author

@showuon how long would you like to wait before merging this if we don't hear back?

@showuon showuon merged commit 7268284 into apache:trunk May 12, 2022
@showuon
Copy link
Copy Markdown
Member

showuon commented May 12, 2022

Thanks for your reminder. Merged into trunk.

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.

4 participants