You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/05/07 20:07:22 UTC
[GitHub] [kafka] gharris1727 opened a new pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
gharris1727 opened a new pull request #8630:
URL: https://github.com/apache/kafka/pull/8630
This is a partner to #6796 that applied the same change but for `ConnectorClientConfigOverridePolicy`.
Signed-off-by: Greg Harris <gr...@confluent.io>
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] gharris1727 commented on a change in pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [kafka] liukrimhrim commented on pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
liukrimhrim commented on pull request #8630:
URL: https://github.com/apache/kafka/pull/8630#issuecomment-626269096
> I've refactored the `PluginUtilsTest` to include more cases, and discovered one discrepancy for the `basic-auth-extension`.
>
> Based on the pattern from the other modules under `connect/` like `transforms`, I would assume that all of the classes in that module would be isolated by default. This does not appear to be the case:
>
> * `org.apache.kafka.connect.rest.basic.auth.extension.BasicAuthSecurityRestExtension` is correctly loaded in isolation
> * `org.apache.kafka.connect.rest.basic.auth.extension.JaasBasicAuthFilter` is not loaded in isolation but should be
> * `org.apache.kafka.connect.rest.basic.auth.extension.PropertyFileLoginModule` is not loaded in isolation but should be
>
> I've left the two classes which do not meet my expectations commented out in the test. I'm not sure what the intended isolation behavior in this case should be.
Do we have a principle to deduce which class should be loaded in isolation, and which should not? Or is it completely a guess based on experience (if a class is a dep only for plugins, it will be fine to load in isolation)?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] gharris1727 commented on pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
gharris1727 commented on pull request #8630:
URL: https://github.com/apache/kafka/pull/8630#issuecomment-625470438
@kkonstantine Can you take a quick look at this and kick off some tests?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] gharris1727 commented on pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
gharris1727 commented on pull request #8630:
URL: https://github.com/apache/kafka/pull/8630#issuecomment-637772629
@kkonstantine I'd appreciate another pass when you're available, thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] gharris1727 commented on pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
gharris1727 commented on pull request #8630:
URL: https://github.com/apache/kafka/pull/8630#issuecomment-625536404
I've refactored the `PluginUtilsTest` to include more cases, and discovered one discrepancy for the `basic-auth-extension`.
Based on the pattern from the other modules under `connect/` like `transforms`, I would assume that all of the classes in that module would be isolated by default. This does not appear to be the case:
- org.apache.kafka.connect.rest.basic.auth.extension.BasicAuthSecurityRestExtension is correctly loaded in isolation
- org.apache.kafka.connect.rest.basic.auth.extension.JaasBasicAuthFilter is not loaded in isolation but should be
- org.apache.kafka.connect.rest.basic.auth.extension.PropertyFileLoginModule is not loaded in isolation but should be
I've left the two classes which do not meet my expectations commented out in the test. I'm not sure what the intended isolation behavior in this case should be.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] kkonstantine commented on a change in pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on a change in pull request #8630:
URL: https://github.com/apache/kafka/pull/8630#discussion_r437754411
##########
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:
Sorry, I wasn't clear. I meant that I think the bookkeeping of specific classes in this test might be hard and that we might miss adding or removing an entry once a class is added or removed in any of the packages. Totally agree that we add classes and interfaces to the API as needed.
The basic principles behind classloading isolation was that we exclude everything in the framework from isolation and the users should not expect to package connect classes with their connectors. Yet, we had the need to introduce exceptions because we package some of the Connect plugins (Connectors, SMTs and Converters and etc) with the framework itself. That explains the inclusion and exclusion lists here and why specific classes/interfaces are present. The intention is keep this "per java package" approach and add only the necessary exceptions. That's why I suggested that an per class explicit listing here might be a bit tedious and perhaps a bit surprising to developers that contribute a class that is properly isolated but they'd be ask to add it here (e.g. a new SMT).
Having said that the benefits of this PR outweigh my concern atm, so I'm happy to move forward and include the test refactoring as well.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] rhauch removed a comment on pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
rhauch removed a comment on pull request #8630:
URL: https://github.com/apache/kafka/pull/8630#issuecomment-632824360
retest this please
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] kkonstantine commented on a change in pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on a change in pull request #8630:
URL: https://github.com/apache/kafka/pull/8630#discussion_r433640144
##########
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[] {
Review comment:
nit: do you mind using `List<String>` and `Arrays.asList(...)`? I don't think array declaration is better if the result is not going to be used as an array. Also, won't work if you try to reinitialize a declared variable.
##########
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",
+ "org.apache.kafka.connect.data.Decimal",
+ "org.apache.kafka.connect.data.Field",
+ "org.apache.kafka.connect.data.Schema",
+ "org.apache.kafka.connect.data.SchemaAndValue",
+ "org.apache.kafka.connect.data.SchemaBuilder",
+ "org.apache.kafka.connect.data.SchemaProjector",
+ "org.apache.kafka.connect.data.Struct",
+ "org.apache.kafka.connect.data.Time",
+ "org.apache.kafka.connect.data.Timestamp",
+ "org.apache.kafka.connect.data.Values",
+ "org.apache.kafka.connect.errors.",
+ "org.apache.kafka.connect.errors.AlreadyExistsException",
+ "org.apache.kafka.connect.errors.ConnectException",
+ "org.apache.kafka.connect.errors.DataException",
+ "org.apache.kafka.connect.errors.IllegalWorkerStateException",
+ "org.apache.kafka.connect.errors.NotFoundException",
+ "org.apache.kafka.connect.errors.RetriableException",
+ "org.apache.kafka.connect.errors.SchemaBuilderException",
+ "org.apache.kafka.connect.errors.SchemaProjectorException",
+ "org.apache.kafka.connect.header.",
+ "org.apache.kafka.connect.header.ConnectHeader",
+ "org.apache.kafka.connect.header.ConnectHeaders",
+ "org.apache.kafka.connect.header.Header",
+ "org.apache.kafka.connect.header.Headers",
+ "org.apache.kafka.connect.health.",
+ "org.apache.kafka.connect.health.AbstractState",
+ "org.apache.kafka.connect.health.ConnectClusterDetails",
+ "org.apache.kafka.connect.health.ConnectClusterState",
+ "org.apache.kafka.connect.health.ConnectorHealth",
+ "org.apache.kafka.connect.health.ConnectorState",
+ "org.apache.kafka.connect.health.ConnectorType",
+ "org.apache.kafka.connect.health.TaskState",
+ "org.apache.kafka.connect.rest.",
+ "org.apache.kafka.connect.rest.ConnectRestExtension",
+ "org.apache.kafka.connect.rest.ConnectRestExtensionContext",
+ "org.apache.kafka.connect.sink.",
+ "org.apache.kafka.connect.sink.SinkConnector",
+ "org.apache.kafka.connect.sink.SinkRecord",
+ "org.apache.kafka.connect.sink.SinkTask",
+ "org.apache.kafka.connect.sink.SinkTaskContext",
+ "org.apache.kafka.connect.source.",
+ "org.apache.kafka.connect.source.SourceConnector",
+ "org.apache.kafka.connect.source.SourceRecord",
+ "org.apache.kafka.connect.source.SourceTask",
+ "org.apache.kafka.connect.source.SourceTaskContext",
+ "org.apache.kafka.connect.storage.",
+ "org.apache.kafka.connect.storage.Converter",
+ "org.apache.kafka.connect.storage.ConverterConfig",
+ "org.apache.kafka.connect.storage.ConverterType",
+ "org.apache.kafka.connect.storage.HeaderConverter",
+ "org.apache.kafka.connect.storage.OffsetStorageReader",
+ //"org.apache.kafka.connect.storage.SimpleHeaderConverter", explicitly isolated
+ //"org.apache.kafka.connect.storage.StringConverter", explicitly isolated
+ "org.apache.kafka.connect.storage.StringConverterConfig",
+ //"org.apache.kafka.connect.transforms.", isolated by default
+ "org.apache.kafka.connect.transforms.Transformation",
+ "org.apache.kafka.connect.util.",
+ "org.apache.kafka.connect.util.ConnectorUtils"
+ };
+ // Classes in the API should never be loaded in isolation.
+ for (String clazz : apiClasses) {
+ assertFalse(
+ clazz + " from 'api' is loaded in isolation but should not be",
+ PluginUtils.shouldLoadInIsolation(clazz)
+ );
+ }
+ }
+
+ @Test
+ public void testConnectRuntimeClasses() {
+ // Only list packages, because there are too many classes.
+ String[] runtimeClasses = new String[]{
Review comment:
nit: same as above.
##########
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 and it's highly improbable that new classes will be removed or added.
Can we instead keep the existing testing based on packages, keep testing the exceptions and then test a few indicative classes that most probably won't be removed any time soon as a way to test the inclusion and exclusion from isolation?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] rhauch commented on pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
rhauch commented on pull request #8630:
URL: https://github.com/apache/kafka/pull/8630#issuecomment-632824360
retest this please
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] gharris1727 edited a comment on pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
gharris1727 edited a comment on pull request #8630:
URL: https://github.com/apache/kafka/pull/8630#issuecomment-625536404
I've refactored the `PluginUtilsTest` to include more cases, and discovered one discrepancy for the `basic-auth-extension`.
Based on the pattern from the other modules under `connect/` like `transforms`, I would assume that all of the classes in that module would be isolated by default. This does not appear to be the case:
- `org.apache.kafka.connect.rest.basic.auth.extension.BasicAuthSecurityRestExtension` is correctly loaded in isolation
- `org.apache.kafka.connect.rest.basic.auth.extension.JaasBasicAuthFilter` is not loaded in isolation but should be
- `org.apache.kafka.connect.rest.basic.auth.extension.PropertyFileLoginModule` is not loaded in isolation but should be
I've left the two classes which do not meet my expectations commented out in the test. I'm not sure what the intended isolation behavior in this case should be.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] rhauch commented on pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
rhauch commented on pull request #8630:
URL: https://github.com/apache/kafka/pull/8630#issuecomment-632824461
ok to test
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] kkonstantine commented on pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on pull request #8630:
URL: https://github.com/apache/kafka/pull/8630#issuecomment-625488195
ok to test
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] kkonstantine merged pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
kkonstantine merged pull request #8630:
URL: https://github.com/apache/kafka/pull/8630
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] kkonstantine commented on pull request #8630: KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on pull request #8630:
URL: https://github.com/apache/kafka/pull/8630#issuecomment-641612088
retest this please
----------------------------------------------------------------
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:
users@infra.apache.org