KAFKA-18863: Connect Multiversion Support (Versioned Connector Creation and related changes)#17743
KAFKA-18863: Connect Multiversion Support (Versioned Connector Creation and related changes)#17743gharris1727 merged 183 commits intoapache:trunkfrom
Conversation
|
Hi @gharris1727. Please take a look at the remaining multiversioning PRs soon. |
gharris1727
left a comment
There was a problem hiding this comment.
Thanks for the PR @snehashisp. I think this will conflict with #17804, I see some overlap in the changed lines.
| connectorConfig.getString(ConnectorConfig.CONNECTOR_CLASS_CONFIG)); | ||
|
|
||
| final Connector connector = instantiateConnector(connectorConfig.originalsStrings()); |
There was a problem hiding this comment.
This method also does not need the object, just the class.
There was a problem hiding this comment.
I use the connector instance in the metrics PR to get the version of the connector in TaskPluginsMetadata. I think instantiating a connector here to get the version is okay here but we can come back to this once we review the metrics PR.
There was a problem hiding this comment.
We added a method for that which also avoid instantiating the plugin, Plugins#pluginVersion. Minimizing instantiations is relatively important because we don't know what sort of resources these plugins allocate.
There was a problem hiding this comment.
Sounds good. I think start API for connectors is where most instantiations of resources should happen but I agree this is an unknown and should be avoided if possible. Have reverted this and will make the necessary adjustments in the following PR.
|
Thanks for the review @gharris1727, will pick it up soon and resolve the conflicts. |
|
Hey @snehashisp I think this looks pretty close. I just had one remaining comment about connector instantiation. Could you also create a new ticket for this PR and link it to the parent? Thanks! |
|
Thanks @gharris1727. Have made the requested changes. |
…on and related changes) (apache#17743) Reviewers: Greg Harris <greg.harris@aiven.io>
|
Hi guys, I’m running into an issue where a static field in a class isn’t being shared between a connector and its SMT. More details here For my understanding, Kafka Connect (before 4.1) should load both under the same class loader when they come from the same plugin path, meaning static fields should be shared. However, what I’m seeing is that the connector and the SMT appear to be loaded by different class loaders, resulting in separate copies of the same class and its static state. Seems to me that with the KIP-891 this assumption is now broken. Am I get it correctly? In particular the broken behavior is related to the SMT that now are loaded in a dedicated class loader. Is this documented somewhere? In addition I'm seeing a weird and inconsistent behavior since in some cases it works as in the past (sharing between SMT and connector works) and in other case it is broken. I'm still trying to understand the root cause for this inconsistency. |
|
An update: seems that the behavior brakes wen in the |
|
Another update: I did other tests and things are more and more weird. All tests done with My original tests was with this directory structure in this case each connector should be isolated each other (having a dedicated class loader). In that case the sharing between connector and SMT not works as for KC 4.0 then I tried with So all connector are not isolated and share the same class loader. In this case no issue. And I'll say that this is expected. Then I tried with /kafka/connect
|___ debezium-connectors
| |___ debezium-connector-postgres
| |___ debezium-connector-mongodb
|___ debezium-connector-sqlserverwhere |
|
Thanks @mfvitale for the report. Can you open a Jira with the details you shared here and in your email to the dev list? |
The is one of a set of PRs for KIP-891. The list of total PRs given below all build one the previous one in the list. They can be reviewed individually, or if the complete set of changes is preferrable, please refer to the last PR.
This is PR#4 and contains to the actual code that creates connectors, converters transformations and other places that need to be adjusted to support multiple connector versions.
Additional PRs for changes to status endpoint showing currently running version and JMX metrics are underway and need not block any feedback for the existing PRs. Some manual testing has been done and extensive tests will soon be added.
Committer Checklist (excluded from commit message)