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.

Sign up for GitHub

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

Jump to bottom

[pulsar-io] pass client builder if no service url provided to debezium connector #12145

Merged
merged 7 commits into from
Jan 18, 2022
Merged

[pulsar-io] pass client builder if no service url provided to debezium connector #12145

merged 7 commits into from
Jan 18, 2022

Conversation

freeznet
Copy link
Contributor

Motivation

#11293 allows to passing client builder to debezium database history, but it still requires passing database.history.pulsar.service.url as well. With client builder, the database.history.pulsar.service.url is not been used anymore.
This PR fixes the logic and only pass client builder with no database.history.pulsar.service.url provided.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

internal bug fix

  • doc

    (If this PR contains doc changes)

@freeznet freeznet force-pushed the freeznet/fix-debezium-client-auth branch from 5ae04fb to 60bbf95 Compare September 23, 2021 03:42
@freeznet freeznet requested a review from sijie September 23, 2021 03:42
@freeznet freeznet self-assigned this Sep 23, 2021
@freeznet freeznet added area/connector release/2.8.2 type/bug The PR fixed a bug or issue reported a bug labels Sep 23, 2021
@freeznet freeznet added this to the 2.9.0 milestone Sep 23, 2021
@freeznet freeznet changed the title [pulsar-io] pass client builder if no service url provided with debezium connector [pulsar-io] pass client builder if no service url provided to debezium connector Sep 23, 2021
@eolivelli
Copy link
Contributor

@dlg99 please take a look

eolivelli
eolivelli reviewed Sep 23, 2021
View reviewed changes
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.

should we update the integration tests ?

@Anonymitaet Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Sep 24, 2021
@eolivelli eolivelli modified the milestones: 2.9.0, 2.10.0 Oct 6, 2021
@freeznet freeznet force-pushed the freeznet/fix-debezium-client-auth branch from 60bbf95 to 24cd641 Compare November 17, 2021 05:38
@freeznet
Copy link
Contributor Author

@eolivelli sorry for the late reply, I have modified the integration tests, PTAL when you have time, thanks.

@freeznet freeznet requested a review from eolivelli November 18, 2021 05:05
@freeznet
Copy link
Contributor Author

@nlu90 please help to review this when you have time, thanks.

eolivelli
eolivelli requested changes Nov 18, 2021
View reviewed changes
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.

We are doubling the number of Debezium tests. They are very heavyweight, we should not go this way.
I believe that we can simply add only one additional test case to test this change and leave the other tests unchanged

@eolivelli
Copy link
Contributor

@nicoloboschi PTAL

nlu90
nlu90 reviewed Nov 18, 2021
View reviewed changes
Copy link
Member

@nlu90 nlu90 left a comment

Choose a reason for hiding this comment

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

+1 for avoid doubling the test workload.

@freeznet freeznet force-pushed the freeznet/fix-debezium-client-auth branch from dde73a7 to 47a87f6 Compare November 19, 2021 04:59
eolivelli
eolivelli approved these changes Nov 19, 2021
View reviewed changes
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

nlu90
nlu90 approved these changes Nov 19, 2021
View reviewed changes
Copy link
Member

@nlu90 nlu90 left a comment

Choose a reason for hiding this comment

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

LGTM

@codelipenghui codelipenghui merged commit aaa22f9 into apache:master Jan 18, 2022
@codelipenghui
Copy link
Contributor

@freeznet Could you please create a PR for 2.8 and 2.9 directly? There is some conflict when cherry-picking into 2.8 and 2.9 branches.

freeznet added a commit to freeznet/pulsar that referenced this pull request Jan 29, 2022
@freeznet
[pulsar-io] pass client builder if no service url provided to debeziu…
d58f48f 
…m connector (apache#12145)

# Conflicts:
#	pulsar-io/debezium/core/src/main/java/org/apache/pulsar/io/debezium/DebeziumSource.java
#	pulsar-io/debezium/core/src/main/java/org/apache/pulsar/io/debezium/PulsarDatabaseHistory.java
#	tests/integration/src/test/java/org/apache/pulsar/tests/integration/io/sources/debezium/DebeziumMySqlSourceTester.java
#	tests/integration/src/test/java/org/apache/pulsar/tests/integration/io/sources/debezium/PulsarDebeziumSourcesTest.java
@freeznet freeznet mentioned this pull request Jan 29, 2022
Merged
4 tasks
freeznet added a commit to freeznet/pulsar that referenced this pull request Jan 29, 2022
@freeznet
[pulsar-io] pass client builder if no service url provided to debeziu…
f0c1b78 
…m connector (apache#12145)

# Conflicts:
#	pulsar-io/debezium/core/src/main/java/org/apache/pulsar/io/debezium/DebeziumSource.java
@freeznet freeznet mentioned this pull request Jan 29, 2022
Merged
4 tasks
@freeznet
Copy link
Contributor Author

@codelipenghui added #14040 and #14041 to add this into 2.8 and 2.9.

@zymap zymap added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Jan 30, 2022
michaeljmarshall pushed a commit that referenced this pull request Feb 11, 2022
@freeznet
[pulsar-io] pass client builder if no service url provided to debeziu…
5f9fd68 
…m connector (#12145) (#14040)

# Conflicts:
#	pulsar-io/debezium/core/src/main/java/org/apache/pulsar/io/debezium/DebeziumSource.java
#	pulsar-io/debezium/core/src/main/java/org/apache/pulsar/io/debezium/PulsarDatabaseHistory.java
#	tests/integration/src/test/java/org/apache/pulsar/tests/integration/io/sources/debezium/DebeziumMySqlSourceTester.java
#	tests/integration/src/test/java/org/apache/pulsar/tests/integration/io/sources/debezium/PulsarDebeziumSourcesTest.java

### Motivation

#11293 allows to passing client builder to debezium database history, but it still requires passing `database.history.pulsar.service.url` as well. With client builder, the `database.history.pulsar.service.url` is not been used anymore. 
This PR fixes the logic and only pass client builder with no `database.history.pulsar.service.url` provided. 

Cherry-pick #12145 into branch-2.8
@gaozhangmin gaozhangmin mentioned this pull request Feb 21, 2022
Closed
codelipenghui pushed a commit that referenced this pull request Feb 24, 2022
@freeznet
[pulsar-io] pass client builder if no service url provided to debeziu…
7792fac 
…m connector (#12145) (#14041)
@nlu90 nlu90 mentioned this pull request Jun 8, 2023
Closed
2 tasks
@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-not-needed Your PR changes do not impact docs labels Jun 9, 2023
@Anonymitaet
Copy link
Member

Anonymitaet commented Jun 9, 2023 •
edited
Loading

This PR needs to update docs. And I've updated docs in apache/pulsar-site#605

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlu90 nlu90 nlu90 approved these changes

@eolivelli eolivelli eolivelli approved these changes

@dlg99 dlg99 dlg99 approved these changes

@sijie sijie Awaiting requested review from sijie

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@lhotari lhotari Awaiting requested review from lhotari

Assignees

@freeznet freeznet

Labels
area/connector cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-complete Your PR changes impact docs and the related docs have been already added. release/2.8.3 release/2.9.3 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.
7 participants
@freeznet @eolivelli @codelipenghui @Anonymitaet @dlg99 @nlu90 @zymap