KAFKA-19248: Multiversioning in Kafka Connect - Plugin Loading Isolation Tests#18325
KAFKA-19248: Multiversioning in Kafka Connect - Plugin Loading Isolation Tests#18325gharris1727 merged 243 commits intoapache:trunkfrom
Conversation
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
| for (Map.Entry<String, String> entry : replacements.entrySet()) { | ||
| content = content.replace(entry.getKey(), entry.getValue()); | ||
| } | ||
| File tmpFile = new File(System.getProperty("java.io.tmpdir") + File.separator + source.getName()); |
There was a problem hiding this comment.
nit: use Files.createTempFile?
There was a problem hiding this comment.
Tried this but it unfortunately always adds a random numerical id to the file generated, even with both prefix and suffix (with one of them as ""). It will add the id in between the prefix and suffix. This fails compilation as the file name of the class is required to have the same name as the class with the .java suffix.
| try { | ||
| String content = Files.readString(source.toPath()); | ||
| for (Map.Entry<String, String> entry : replacements.entrySet()) { | ||
| content = content.replace(entry.getKey(), entry.getValue()); |
There was a problem hiding this comment.
nit: I don't really like this "modifying arbitrary source code before compilation" technique, it's very powerful and open-ended, and could be misused and be more difficult to debug like self-modifying code. But because the current application is within reason, maybe this is good enough to merge.
Prior art here is the ReadVersionFromResourcePlugin, whose code is duplicated in read-version-from-resource-v1 and read-version-from-resource-v2, with version files are specified explicitly. I think that we shouldn't follow this same strategy for these new plugins and versions, but there's a middle ground where we generate version files from Strings specified in createPluginJar/writeJar.
There was a problem hiding this comment.
Yeah its powerful. It would be interesting to see in what ways people start misusing this capability. If it turns out to be problematic, I will followup and see if the version file approach can be introduced instead.
| } | ||
|
|
||
| public synchronized Path build(String pluginDir) throws IOException { | ||
| Path pluginDirPath = Files.createTempDirectory(pluginDir); |
There was a problem hiding this comment.
This method is leaking files on each run. I found a bunch of files left over on my machine:
cd /var/folders/k1/6b93_bm16596yf4d3dlp6vbh0000gn/T
ls
rm -rf SINK_CONNECTOR-* SOURCE_CONNECTOR-* PREDICATE-* TRANSFORMATION-* HEADER_CONVERTER-* CONVERTER-* all_versioned_artifact*
There was a problem hiding this comment.
Thanks for catching this. Added deleteOnExit on the created temp plugins so that it gets cleaned up.
snehashisp
changed the title
|
Hi @gharris1727. Have addressed the comments in this PR, PTAL soon. |
gharris1727
left a comment
gharris1727
left a comment
There was a problem hiding this comment.
LGTM.
I was having some difficulty running this locally due to https://issues.apache.org/jira/browse/KAFKA-18834 . As it's unrelated and seems to be well understood, I'll just keep an eye out to see if this causes more problems on trunk.
…ion Tests (apache#18325) This adds tests for [KIP-891](https://cwiki.apache.org/confluence/display/KAFKA/KIP-891%3A+Running+multiple+versions+of+Connector+plugins). It primarily focuses on tests for the new additions in plugin loading isolation. It has dependency on the actual KIP implementation PRs and should be merged post apache#17742 Reviewers: Greg Harris <greg.harris@aiven.io>
2 participants
This adds tests for
KIP-891.
It primarily focuses on tests for the new additions in plugin loading
isolation. It has dependency on the actual KIP implementation PRs and
should be merged post #17742