[pulsar-io] pass the pulsar service url to debezium source for history database #11251

nlu90 · 2021-07-07T23:35:37Z

Motivation

The Debezium requires pulsar a service URL for history database usage.

In #11056 , the service.url field from PulsarKafkaWorkerConfig is no longer available. And the value is also deleted from multiple yaml config files in this commit. This causes the integration test for Debezium connector to fail.

Based on the Debezium paradigm, all configurations should be passed as strings. There's no easy way to inject a PulsarClient via configuration.

We need to ask user to provide the pulsar url explicitly and probably auth info also.

Modifications

Make the database.history.pulsar.service.url field required
Add the config value back to example yaml files
Update the integration test config

Verifying this change

Make sure that the change passes the CI checks.

dlg99

This LGTM and it will fix the tests/allow connector to run.
This returns us to original problem of having to add extra parameters to configure the pulsar client, this can be address later IMO.

eolivelli

LGTM is this unblocks Debezium tests

IIUC we wanted to use the client in order to access auth and TLS.

With this change it is not possible to do it.

What is your plan?

dlg99 · 2021-07-08T17:08:39Z

...ava/org/apache/pulsar/tests/integration/io/sources/debezium/DebeziumMongoDbSourceTester.java

@@ -50,7 +50,7 @@ public DebeziumMongoDbSourceTester(PulsarCluster cluster) {
        sourceConfig.put("mongodb.password", "dbz");
        sourceConfig.put("mongodb.task.id","1");
        sourceConfig.put("database.whitelist", "inventory");
-        sourceConfig.put("database.history.pulsar.service.url", pulsarServiceUrl);
+        sourceConfig.put("history.database.pulsar.service.url", pulsarServiceUrl);


I thought it should start with DatabaseHistory.CONFIGURATION_FIELD_PREFIX_STRING which is CONFIGURATION_FIELD_PREFIX_STRING = "database.history."
debezium does

Configuration dbHistoryConfig = config.subset(DatabaseHistory.CONFIGURATION_FIELD_PREFIX_STRING, false)

Yeah, I just updated them.

nlu90 · 2021-07-08T21:16:06Z

LGTM is this unblocks Debezium tests

IIUC we wanted to use the client in order to access auth and TLS.

With this change it is not possible to do it.

What is your plan?

Based on Debezium's current paradigm, user can only pass string parameters in the configuration. For our case, it'll be better if they allow passing serialized objects. This will be the most secure way but needs help from Debezium community.

A second way would be we can access the PulsarClient via some static method in PulsarDatabaseHistory. This is a little bit hacky but works quickly if it's possible.

One last way is we add history database specific pulsar auth parameters in PulsarDatabaseHistory. This makes it follow the Debezium paradigm but not secure with pulsar.

dlg99 · 2021-07-09T00:41:12Z

@nlu90

One last way is we add history database specific pulsar auth parameters

adding all parameters is tedious (the way config definition works there needs definition of each parameter with bunch of metadata around).
What if we could define one parameter, String, for serialized config json and then do something like

String clientConfJson = pulsarClient.getConfig();

to pass that to the PulsarDatabaseHistory that can use ClientBuilder.loadConf(..)

nlu90 · 2021-07-09T01:56:36Z

@nlu90

One last way is we add history database specific pulsar auth parameters

adding all parameters is tedious (the way config definition works there needs definition of each parameter with bunch of metadata around).
What if we could define one parameter, String, for serialized config json and then do something like
String clientConfJson = pulsarClient.getConfig();
to pass that to the PulsarDatabaseHistory that can use ClientBuilder.loadConf(..)

@dlg99 This sounds an interesting idea!
I just checked the PulsarClient API, it doesn't support getConfig currently. We may want to discuss with the community about adding this API first.

sijie

@nlu90 what is the plan for supporting authentication for the Pulsar cluster?

sijie · 2021-07-09T04:20:32Z

A second way would be we can access the PulsarClient via some static method in PulsarDatabaseHistory. This is a little bit hacky but works quickly if it's possible.

@nlu90 I felt this is the most secure solution because the connector users don't need to provide the auth information through function config. The current PR still requires users to submit auth information via function config. Although I admit the code will look a bit hacky, it is still manageable.

eolivelli · 2021-07-09T13:48:07Z

if this patch is unblocking @dlg99 's work to add back support for running Integration Tests of Pulsar IO
I would prefer to commit the patch in the current form and then follow up.

CI is in a very bad state currently.

cc @sijie @rdhabalia @merlimat @codelipenghui

sijie · 2021-07-13T02:59:54Z

@eolivelli How is that related to CI in a very bad state since the integration tests for debezium is not enabled?

I would have a concern about this patch as it doesn't support accessing an authenticated Pulsar cluster yet. Even we merge this pull request for now, the connector can't be used in an authenticated cluster. I think we need to spend more time figuring out how to fix this properly. All the changes related are done in the master which is the development branch for 2.9 (which is targeted to release in September based on a time-based release schedule). I don't see why we need to rush on merging this. We need to fix this properly in order to make the debezium connector work in an authenticated cluster.

sijie · 2021-07-13T04:32:14Z

@nlu90 an alternative approach is to expose the client builder via the context. So we can just serialize the client builder to a base64 encoded string and pass it to debezium config. Hence the pulsar database history can get the client builder and create a Pulsar client from that. I created a PR (#11293) to demonstrate the idea. Let me know what you think.

eolivelli · 2021-07-13T05:43:51Z

@sijie

How is that related to CI in a very bad state since the integration tests for debezium is not enabled?
Sorry, I wasn't clear.

The point is that currently Debezium related tests are not running on CI, and I meant that if CI is not running the tests this is kind of a "bad state" :-)

We cannot enable them (#11154) because currently they will fail due to the problem that this PR is trying to address.

I would feel more confident with this change and any other proposal if we could revert #11056, commit #11154 and then be back in resolving the Client Auth problem.

So my proposal is:

revert PIP-85: [pulsar-io] pass pulsar client via context to connector #11056 -> this prepares the way for Fixing Debezium integration tests #11154
commit Fixing Debezium integration tests #11154 -> now we have CI that validates new changes
work on this fix or on [IO] Pass client builder to debezium database history #11293

Does it sound like a good plan ?
because without #11154 we cannot validate on CI any patch related to Debezium

dlg99 · 2021-07-13T15:29:31Z

@sijie I second @eolivelli here.
We should either merge this change + fix for the CI or revert the breaking change, merge the fix for the CI, and do a follow up for the authentication after that.
At least the follow up change will get tested this time.
I don't find attractive idea of piling up more changes on top of this commit because it:

has unpredictable ETA. Whatever way you decide to go with authentication will require separate discussion/CR, maybe even PIP.
is blocking other changes (i.e. debezium upgrade)
is leaving the debezium connectors in broken state (affects people testing/experimenting with pulsar on 2.9-snapshot)
assumes that more changes added with the integration tests not enabled on the CI

We are dealing with a case of broken functionality in the build. I think the first priority should be fixing the regression quickly and new features should be second.

an alternative approach for #11251

…y database (apache#11251) ### Motivation The Debezium requires pulsar a service URL for history database usage. In apache#11056 , the `service.url` field from `PulsarKafkaWorkerConfig` is no longer available. And the value is also deleted from multiple yaml config files in this [commit](apache@3ce24c9). This causes the integration test for Debezium connector to fail. Based on the Debezium [paradigm](https://debezium.io/documentation/reference/1.5/connectors/mysql.html#debezium-mysql-connector-database-history-configuration-properties), all configurations should be passed as strings. There's no easy way to inject a PulsarClient via configuration. We need to ask user to provide the pulsar url explicitly and probably auth info also. ### Modifications 1. Make the `database.history.pulsar.service.url` field required 2. Add the config value back to example yaml files 3. Update the integration test config ### Verifying this change - [ ] Make sure that the change passes the CI checks.

an alternative approach for apache#11251

nlu90 added 2 commits July 7, 2021 16:24

pass the pulsar service url to debezium source for history database

7ac0177

update integration test config

6bea348

dlg99 approved these changes Jul 8, 2021

View reviewed changes

eolivelli reviewed Jul 8, 2021

View reviewed changes

eolivelli approved these changes Jul 8, 2021

View reviewed changes

update configs value

0598b75

dlg99 reviewed Jul 8, 2021

View reviewed changes

fix

3408361

nlu90 marked this pull request as ready for review July 9, 2021 00:09

sijie assigned nlu90 Jul 9, 2021

sijie added the area/connector label Jul 9, 2021

sijie added this to the 2.9.0 milestone Jul 9, 2021

sijie reviewed Jul 9, 2021

View reviewed changes

freeznet approved these changes Jul 9, 2021

View reviewed changes

sijie mentioned this pull request Jul 13, 2021

[IO] Pass client builder to debezium database history #11293

Merged

codelipenghui approved these changes Jul 15, 2021

View reviewed changes

codelipenghui merged commit d2d192b into apache:master Jul 15, 2021

dlg99 mentioned this pull request Jul 15, 2021

Fixing Debezium integration tests #11154

Merged

1 task

nlu90 mentioned this pull request Jul 23, 2021

[doc] update doc for debezium connector and log-topic message #11451

Merged

codelipenghui pushed a commit that referenced this pull request Aug 20, 2021

[IO] Pass client builder to debezium database history (#11293)

12f6566

an alternative approach for #11251

gaoran10 mentioned this pull request Mar 8, 2022

fix(docs): add missing config in debezium mysql source #14590

Closed

4 tasks

bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022

[IO] Pass client builder to debezium database history (apache#11293)

04cc940

an alternative approach for apache#11251

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pulsar-io] pass the pulsar service url to debezium source for history database #11251

[pulsar-io] pass the pulsar service url to debezium source for history database #11251

nlu90 commented Jul 7, 2021 •

edited

dlg99 left a comment

eolivelli left a comment

dlg99 Jul 8, 2021 •

edited

nlu90 Jul 8, 2021

nlu90 commented Jul 8, 2021

dlg99 commented Jul 9, 2021

nlu90 commented Jul 9, 2021

sijie left a comment

sijie commented Jul 9, 2021

eolivelli commented Jul 9, 2021

sijie commented Jul 13, 2021

sijie commented Jul 13, 2021

eolivelli commented Jul 13, 2021

dlg99 commented Jul 13, 2021

[pulsar-io] pass the pulsar service url to debezium source for history database #11251

[pulsar-io] pass the pulsar service url to debezium source for history database #11251

Conversation

nlu90 commented Jul 7, 2021 • edited

Motivation

Modifications

Verifying this change

dlg99 left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

dlg99 Jul 8, 2021 • edited

Choose a reason for hiding this comment

nlu90 Jul 8, 2021

Choose a reason for hiding this comment

nlu90 commented Jul 8, 2021

dlg99 commented Jul 9, 2021

nlu90 commented Jul 9, 2021

sijie left a comment

Choose a reason for hiding this comment

sijie commented Jul 9, 2021

eolivelli commented Jul 9, 2021

sijie commented Jul 13, 2021

sijie commented Jul 13, 2021

eolivelli commented Jul 13, 2021

dlg99 commented Jul 13, 2021

nlu90 commented Jul 7, 2021 •

edited

dlg99 Jul 8, 2021 •

edited