gharris1727 commented on a change in pull request #8630:
URL: https://github.com/apache/kafka/pull/8630#discussion_r434139998
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginUtilsTest.java
##########
@@ -114,68 +90,264 @@ public void testConnectFrameworkClasses() {
assertFalse(PluginUtils.shouldLoadInIsolation(
"org.apache.kafka.clients.admin.KafkaAdminClient")
);
- assertFalse(PluginUtils.shouldLoadInIsolation(
- "org.apache.kafka.connect.rest.ConnectRestExtension")
- );
}
@Test
- public void testAllowedConnectFrameworkClasses() {
-
assertTrue(PluginUtils.shouldLoadInIsolation("org.apache.kafka.connect.transforms."));
- assertTrue(PluginUtils.shouldLoadInIsolation(
- "org.apache.kafka.connect.transforms.ExtractField")
- );
- assertTrue(PluginUtils.shouldLoadInIsolation(
- "org.apache.kafka.connect.transforms.ExtractField$Key")
- );
-
assertTrue(PluginUtils.shouldLoadInIsolation("org.apache.kafka.connect.json."));
- assertTrue(PluginUtils.shouldLoadInIsolation(
- "org.apache.kafka.connect.json.JsonConverter")
- );
- assertTrue(PluginUtils.shouldLoadInIsolation(
- "org.apache.kafka.connect.json.JsonConverter$21")
- );
-
assertTrue(PluginUtils.shouldLoadInIsolation("org.apache.kafka.connect.file."));
- assertTrue(PluginUtils.shouldLoadInIsolation(
- "org.apache.kafka.connect.file.FileStreamSourceTask")
- );
- assertTrue(PluginUtils.shouldLoadInIsolation(
- "org.apache.kafka.connect.file.FileStreamSinkConnector")
- );
+ public void testConnectApiClasses() {
+ String[] apiClasses = new String[] {
+ // Enumerate all packages and classes
+ "org.apache.kafka.connect.",
+ "org.apache.kafka.connect.components.",
+ "org.apache.kafka.connect.components.Versioned",
+ //"org.apache.kafka.connect.connector.policy.", isolated by default
+
"org.apache.kafka.connect.connector.policy.ConnectorClientConfigOverridePolicy",
+
"org.apache.kafka.connect.connector.policy.ConnectorClientConfigRequest",
+ "org.apache.kafka.connect.connector.",
+ "org.apache.kafka.connect.connector.Connector",
+ "org.apache.kafka.connect.connector.ConnectorContext",
+ "org.apache.kafka.connect.connector.ConnectRecord",
+ "org.apache.kafka.connect.connector.Task",
+ "org.apache.kafka.connect.data.",
+ "org.apache.kafka.connect.data.ConnectSchema",
+ "org.apache.kafka.connect.data.Date",
Review comment:
> The exhaustive list of classes is unmaintainable
Yes, it will be difficult to keep this test perfectly up-to-date without
due-diligence from reviewers and committers. I think this due-diligence is
valuable, and will avoid a bug-fix PR like this being necessary in the future.
> it's highly improbable that new classes will be removed or added.
I would say infrequent, but not improbable. This next release includes two
KIPs that added classes to the api, and each had to change the whitelist and/or
tests (that's why this had merge conflicts earlier).
I don't think it's significantly more effort to maintain the exhaustive list
than it is to maintain a reduced list of classes either. Consider this decision
tree:
1. Did you add an API class?
If not -> It's not necessary to change this test, since it's only
concerned with API classes.
2. Is your new class inside an existing isolated-by-default package?
If so -> You'd need to exclude this class, and not doing so introduces a
bug.
Once you change PluginUtils, you'd naturally update PluginUtilsTest with
your class name to verify the fix.
3. Is your new class in a new package
If so -> You'd need to update this test even if it was a non-exhaustive
packages-only test.
Only if you've fallen through all of those conditions, and added an API
class in an existing package that is already whitelisted, would you save the
time necessary to update this test. And in this case, the code is already
correct, so the test silently diverges from the real class list, without any
negative effects.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
