DBZ-4518 Make kafka query timeout configurable by snigdhasjg · Pull Request #3091 · debezium/debezium

snigdhasjg · 2022-01-07T18:41:32Z

When creating history topic, debezium core module's KafkaDatabaseHistory class make an API call using KafkaAdminClient to get broker default replication factor.

The call can get timed-out if there are SSL encryption and SASL validation happens on server side while creating connection.

The aim here is to make timeout configurable.

github-actions · 2022-01-07T18:41:51Z

Welcome as a new contributor to Debezium, @snigdhasjg. Reviewers, please add missing author name(s) and alias name(s) to the COPYRIGHT.txt and Aliases.txt respectively.

snigdhasjg · 2022-01-07T18:52:27Z

+    public static final Field KAFKA_QUERY_TIMEOUT_MS = Field.create(CONFIGURATION_FIELD_PREFIX_STRING + "kafka.query.timeout.ms")
+            .withDisplayName("Kafka admin client query timeout (ms)")
+            .withType(Type.LONG)
+            .withGroup(Field.createGroupEntry(Field.Group.CONNECTION_ADVANCED, 1)) // TODO Validate the group & position


Please confirm Group and Position. Not sure what are those.

@snigdhasjg Could you please increase the value by one so the values are unique?

@rk3rn3r Do you know why the collison was not reported?

Done. But may I know where is CONNECTION_ADVANCED position 1? Because I couldn’t find it.

@snigdhasjg oh, my bad, I misread ADVANCED vs CONNECTION_ADVANCED. Please use ADVANCED as the other one is usually used form setting-up connection to the database. Could also please squash the commits too?

@jpechane the duplicate check uses *ConnectorConfig.ALL_FIELDS to check for duplicates. The new field wasn't added to that lists yet.

#DBZ-4537 this card is closed.

@rk3rn3r Sorry for asking, where should I add the comment about new property?

@ani-sha did you take care of this when working on DBZ-4537 or else could you file a follow up task for that?

@rk3rn3r, so DBZ-4537 is closed already. But could you remind me, what is the follow-up work you have in mind?

@gunnarmorling I was referring to

so HistorizedRelationalDatabaseConnectorConfig.CONFIG_DEFINITION and {{Db2ConnectorConfig.configDef}}.

especially the Db2 connector related work. And I don't know exactly what Jiri intended, but I referred to the work he had in mind on his previous comment.

@jpechane, ok, could you log a follow-up issue for this then? I've really lost track of what this is about. Thx!

jpechane · 2022-01-10T08:27:31Z

@snigdhasjg Could you please also update documentation?

snigdhasjg · 2022-01-10T09:54:36Z

@jpechane how do I update the documentation? Could you please guide me there.

jpechane · 2022-01-10T10:45:19Z

@snigdhasjg The doc file in question is https://github.com/debezium/debezium/blob/main/documentation/modules/ROOT/partials/modules/all-connectors/ref-connector-configuration-database-history-properties.adoc all relevant connectrs will automatically use it. I'd say that the best variant is to place the new config option just after the kafka.recovery.poll.interval.ms.

snigdhasjg · 2022-01-10T18:21:57Z

I'd say that the best variant is to place the new config option just after the kafka.recovery.poll.interval.ms.

I have added descriptions. Please validate

gunnarmorling · 2022-01-11T13:52:35Z

+    public static final Field KAFKA_QUERY_TIMEOUT_MS = Field.create(CONFIGURATION_FIELD_PREFIX_STRING + "kafka.query.timeout.ms")
+            .withDisplayName("Kafka admin client query timeout (ms)")
+            .withType(Type.LONG)
+            .withGroup(Field.createGroupEntry(Field.Group.ADVANCED, 2))


@rk3rn3r, about having a new group DATABASE_HISTORY for this and all related properties?

Sure, if you think a new group is needed you can introduce a new group for it any time.
pinging @mdrillin @indraraj @uidoyen

As these are connection properties (for Kafka) and mandatory, they could also to the CONNECTION group. We do like that for the MySQL connector at the moment. I would personally not use the ADVANCED group as it is meant for very advanced, mostly optional properties.

@rk3rn3r this property is optional. Have default value of 3sec. Do you still think it belongs to CONNECTION?

We do like that for the MySQL connector at the moment.

@rk3rn3r, wasn't CONNECTION meant for the actual DB-connection related properties? And do you have a reference to where we put history-related properties into that group?

Sure, if you think a new group is needed you can introduce a new group for it any time.

Ok, but what are the implications of this? Will this require a synced effort to get corresponding changes done to the UI? I.e. how exactly are these group definitions interpreted in the UI?

@snigdhasjg @gunnarmorling Yes, this property is optional, but it belongs to the database history settings which are mandatory for connectors with database history. And all properties around database history are on the connection tab in the UI because they hold database history Kafka (broker) connection settings. So we decided this is a good place without introducing too many groups, especially as long as not all connectors support database history (yet).

@gunnarmorling

And do you have a reference to where we put history-related properties into that group?

For example we have it configured already here in the same file KafkaDatabaseHistory.java#L100

what are the implications of this?

We should move all related properties to that new group and then we need to change the UI to show these properties accordingly on the connection tab (or wherever you want to place it). But this change could be done safely now as it will only be in effect when the UI migrates to the JSON schemas.

Does this help?

But this change could be done safely now as it will only be in effect when the UI migrates to the JSON schemas.

@rk3rn3r, are you sure about this? Isn't that grouping/ordering already obtained today also with the current approach of having the Debezium connectors embedded into the UI backend and dynamically building the JSON schema from that? Or are you saying it's safe, as we don't update that embedded version to 1.9-next (which would bring this change) just yet?

@gunnarmorling 100% sure. We are using 1.9-SNAPSHOT as dependency and nightly images. But until we migrate to the JOSN files the grouping comes from the big maps in the ConnectorIntegrator implementations. For example see PostgresConnectorIntegrator

@rk3rn3r Used groupCONNECTION index 33

Not sure what else I'm missing here.

That's good @snigdhasjg. Really appreciate your effort!
Last the final question to @gunnarmorling if he prefers a new group DATABASE_HISTORY now or if we leave it like this and create a new JIRA to cover new group creation?

gunnarmorling

LGTM overall; one minor remark inline.

rk3rn3r · 2022-02-02T12:40:24Z

Last call to @jpechane @gunnarmorling to quickly give a final review of the last comments and code and merge eventually. 😉

jpechane · 2022-02-04T13:09:21Z

@snigdhasjg Applied, thanks a lott. Sorry for having the PR to drag for that long.
@gunnarmorling No follow-up is needed as Db2 connector already uses shared configuration.

snigdhasjg · 2022-02-04T18:13:52Z

Thank you folks for your valuable feedback :)

snigdhasjg commented Jan 7, 2022

View reviewed changes

gunnarmorling reviewed Jan 11, 2022

View reviewed changes

Comment thread ...m-connector-mysql/src/test/java/io/debezium/relational/history/KafkaDatabaseHistoryTest.java

DBZ-4518 Make kafka query timeout configurable

22d56eb

jpechane merged commit 0ab45ec into debezium:main Feb 4, 2022

Uh oh!

Conversation

snigdhasjg commented Jan 7, 2022

Uh oh!

github-actions Bot commented Jan 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rk3rn3r Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rk3rn3r Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpechane commented Jan 10, 2022

Uh oh!

snigdhasjg commented Jan 10, 2022

Uh oh!

jpechane commented Jan 10, 2022

Uh oh!

snigdhasjg commented Jan 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rk3rn3r Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rk3rn3r Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gunnarmorling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rk3rn3r commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpechane commented Feb 4, 2022

Uh oh!

snigdhasjg commented Feb 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

rk3rn3r Jan 10, 2022 •

edited

Loading

rk3rn3r Feb 4, 2022 •

edited

Loading

rk3rn3r Jan 18, 2022 •

edited

Loading

rk3rn3r Jan 25, 2022 •

edited

Loading

rk3rn3r commented Feb 2, 2022 •

edited

Loading