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

[fix][io] KCA: Option to use kafka connector's SourceConnector class to create task and task config #19772

Merged
merged 2 commits into from
Mar 10, 2023

Conversation

dlg99
Copy link
Contributor

@dlg99 dlg99 commented Mar 10, 2023

Motivation

KCA's SourceConnector uses Kafka's task name as a parameter and creates the task directly.
this works well enough for simpler or old sources (e.g. debezium's postgre/mysql/mongo etc).
Actual expected life cycle is to create a connector, then get the task class and the task config from it.
Connectors that rely on such cycle do not work with KCA, e.g. yugabyte's debezium source. See
https://apache-pulsar.slack.com/archives/C5Z4T36F7/p1677506713032549 for details.

Modifications

Added option to pass source connector's class name, made use of it.
Kept the KCA compatible with old configuration, falling back to the legacy behavior if the config is not set.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added unit test.

Does this pull request potentially affect one of the following parts:

NO

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: dlg99#11

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 10, 2023
@dlg99 dlg99 changed the title KCA: Option to use kafka connector's SourceConnector class to create task and task config [fix][io] KCA: Option to use kafka connector's SourceConnector class to create task and task config Mar 10, 2023
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Great work

There is an import to be removed before committing the patch

@@ -37,6 +36,7 @@
import org.apache.pulsar.common.schema.KeyValue;
import org.apache.pulsar.functions.api.Record;
import org.apache.pulsar.io.core.SourceContext;
import org.jetbrains.annotations.NotNull;
Copy link
Contributor

Choose a reason for hiding this comment

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

This was added by the IDE.
We should remove it before committing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

@eolivelli eolivelli merged commit 90b0f0a into apache:master Mar 10, 2023
@eolivelli
Copy link
Contributor

@dlg99 I would like to cherry-pick this patch to branch-2.11 and to branch-2.10, but the patch doesn't apply.

Could you please send a new PR ? or maybe there are dependencies that we are missing ?

dlg99 added a commit to dlg99/pulsar that referenced this pull request Mar 10, 2023
…to create task and task config (apache#19772)

(cherry picked from commit 90b0f0a)
dlg99 added a commit to dlg99/pulsar that referenced this pull request Mar 10, 2023
…to create task and task config (apache#19772)

(cherry picked from commit 90b0f0a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connector doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants