[yugabyte/yugabyte-db#26069] Custom metrics implementation for multi task model #165
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vaibhav-yb
commented
Feb 17, 2025
| @@ -1086,13 +1086,6 @@ public static AutoCreateMode parse(String value, String defaultValue) { | |||
| public static final Field SOURCE_INFO_STRUCT_MAKER = CommonConnectorConfig.SOURCE_INFO_STRUCT_MAKER | |||
| .withDefault(PostgresSourceInfoStructMaker.class.getName()); | |||
|
|
|||
| public static final Field TASK_ID = Field.create("task.id") | |||
There was a problem hiding this comment.
This was removed since we already have a task.id field in the CommonConnectorConfig of type STRING - we are now reusing the same config.
vaibhav-yb
commented
Feb 17, 2025
| @@ -123,9 +124,10 @@ public ChangeEventSourceCoordinator<PostgresPartition, PostgresOffsetContext> st | |||
| final PostgresValueConverter valueConverter = valueConverterBuilder.build(typeRegistry); | |||
|
|
|||
| schema = new PostgresSchema(connectorConfig, defaultValueConverter, topicNamingStrategy, valueConverter); | |||
| this.taskContext = new PostgresTaskContext(connectorConfig, schema, topicNamingStrategy, connectorConfig.taskId()); | |||
| this.taskContext = new PostgresTaskContext(connectorConfig, schema, topicNamingStrategy, connectorConfig.getTaskId()); | |||
There was a problem hiding this comment.
This change is simply to use the existing method to get the task ID.
vaibhav-yb
commented
Feb 17, 2025
| @@ -21,9 +21,9 @@ public class PostgresPartition extends AbstractPartition implements Partition { | |||
| private static final String SERVER_PARTITION_KEY = "server"; | |||
|
|
|||
| private final String serverName; | |||
| private final int taskId; | |||
| private final String taskId; | |||
There was a problem hiding this comment.
Task ID was added in #163 as an integer but since we are reusing the existing one in CommonConnectorConfig, keeping it as String would simplify things as we will not need unnecessary type casts.
vaibhav-yb
changed the title
vaibhav-yb
changed the title
suranjan
approved these changes
Feb 20, 2025
Sumukh-Phalgaonkar
approved these changes
Feb 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Currently, the connector follows a single task single partition model and thus uses the
DefaultChangeEventSourceMetricsFactorywhich doesn't let us override the metric object names. Since we cannot override names, this causes an issue when we go for the multi task model as there's a conflict in the metric object names if a task registers its metric objects and subsequent tasks try to register their metric objects too.Solution
This is resolved by adding a custom metrics implementation which lets us customise things accordingly for a multi task model. This is done by following the example of metrics implemented by SqlServerConnector at https://github.com/yugabyte/debezium/tree/2.5.2.Final/debezium-connector-sqlserver/src/main/java/io/debezium/connector/sqlserver/metrics
This closes yugabyte/yugabyte-db#26069