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

[WIP] KIP-891: Connect Multiversion Support (Transformation and Predicate Changes) #17742

Open
wants to merge 142 commits into
base: trunk
Choose a base branch
from

Conversation

snehashisp
Copy link
Contributor

@snehashisp snehashisp commented Nov 10, 2024

The is one of a set of PRs for KIP-891. The list of total PRs given below all build one the previous one in the list. They can be reviewed individually, or if the complete set of changes is preferrable, please refer to the last PR.

This is PR#3 and contains changes to support multiple versions of transformation and predicates with appropriate defaults and validations.

  1. KAFKA-18182: KIP-891 Connect Multiversion Support (Base PR with Plugin Loading Isolation Changes) #16984 - Plugins Loading Isolation Changes
  2. KAFKA-18215: KIP-891 Connect Multiversioning Support (Configs and Validation changes for Connectors and Converters) #17741 - Versioned Configs and Validation for Connector and Converters
  3. [WIP] KIP-891: Connect Multiversion Support (Transformation and Predicate Changes) #17742 - Transformation and Predicate Support
  4. [WIP] KIP-891: Connect Multiversion Support (Versioned Connector Creation and related changes) #17743 - Versioned Connector Creation
  5. [WIP] KIP-891: Connect Multiversion Support (Updates to status and metrics) #17988 - Updates to status and metrics

Committer Checklist (excluded from commit message)

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

Copy link
Contributor

@gharris1727 gharris1727 left a comment

Choose a reason for hiding this comment

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

I don't see anything that stands out to me right now. Once the build is fixed i think we can merge.

@snehashisp
Copy link
Contributor Author

snehashisp commented Dec 15, 2024

Hi @gharris1727. I have updated the code a bit. PTAL and LMK if you have any queries.

final ConfigDef.Validator typeValidator = ConfigDef.LambdaValidator.with(
(String name, Object value) -> {
validateProps(prefix);
// The value will be null if the class couldn't be found; no point in performing follow-up validation
if (value != null) {
getConfigDefFromConfigProvidingClass(typeConfig, (Class<?>) value);
getConfigDefFromPlugin(typeConfig, ((Class<?>) value).getName(), null, plugins);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this validator needs to include the error when the version is incorrect, similar to the converters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Gradle build or GitHub Actions ci-approved clients connect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants