beardt commented on code in PR #13168:
URL: https://github.com/apache/kafka/pull/13168#discussion_r1093403718
##########
clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java:
##########
@@ -54,6 +58,12 @@ public void testConfiguredInstances() {
testInvalidInputs("org.apache.kafka.clients.producer.unknown-metrics-reporter");
testInvalidInputs("test1,test2");
testInvalidInputs("org.apache.kafka.common.metrics.FakeMetricsReporter,");
+ testInvalidInputs(TestInterceptorConfig.INTERCEPTOR_CLASSES_CONFIG,
TestInterceptorConfig.ORG_APACHE_KAFKA_TEST_MOCK_CONSUMER_INTERCEPTOR + ", "
+ +
TestInterceptorConfig.ORG_APACHE_KAFKA_TEST_MOCK_CONSUMER_INTERCEPTOR + ", "
+ +
TestInterceptorConfig.ORG_APACHE_KAFKA_TEST_MOCK_CONSUMER_INTERCEPTOR,
org.apache.kafka.test.MockConsumerInterceptor.class);
+ testInvalidInputs(TestInterceptorConfig.INTERCEPTOR_CLASSES_CONFIG,
TestInterceptorConfig.ORG_APACHE_KAFKA_TEST_MOCK_PRODUCER_INTERCEPTOR + ", "
+ +
TestInterceptorConfig.ORG_APACHE_KAFKA_TEST_MOCK_PRODUCER_INTERCEPTOR + ", "
+ +
TestInterceptorConfig.ORG_APACHE_KAFKA_TEST_MOCK_PRODUCER_INTERCEPTOR,
org.apache.kafka.test.MockProducerInterceptor.class);
Review Comment:
As a follow up on your earlier comments, another value of this test is in
understanding the intent of the coded feature. In this case (pun intended), a
developer looking at this test case would rightfully wonder what does
`TestConfig.METRIC_REPORT_CLASSES_CONFIG` have to do with interceptors?
Furthermore, using `TestConfig` in your suggested fashion and goal adds
confusion to the test case as `METRIC_REPORT_CLASSES_CONFIG` would never be
used with interceptors in the real world.
**NOTE:** Although not in scope, `TestConfig` really should be renamed to
`MetricReporterTestConfig` as this more accurately aligns with the
implementation.
Therefore, I recommend retaining the slightly renamed
`InterceptorTestConfig` class as this is consistent with the collaborating test
subject e.g. `MockConsumerInterceptor` and reduces noise when reasoning out
the test case. Additionally, I've modified your suggested test case below
which includes a slightly updated version of the `InterceptorTestConfig` class
your review:
```
@Test
public void testConfiguredInstancesClosedOnFailure() {
try {
Map<String, String> props = new HashMap<>();
String threeConsumerInterceptors =
MockConsumerInterceptor.class.getName() + ", "
+ MockConsumerInterceptor.class.getName() + ", "
+ MockConsumerInterceptor.class.getName();
props.put(TestInterceptorConfig.INTERCEPTOR_CLASSES_CONFIG,
threeConsumerInterceptors);
props.put(TestInterceptorConfig.CLIENT_ID_CONFIG, "test");
TestConfig testConfig = new TestConfig(props);
MockConsumerInterceptor.setThrowOnConfigExceptionThreshold(TestInterceptorConfig.getTargetInterceptor());
assertThrows(
Exception.class,
() ->
testConfig.getConfiguredInstances(TestInterceptorConfig.INTERCEPTOR_CLASSES_CONFIG,
Object.class)
);
assertEquals(3, MockConsumerInterceptor.CONFIG_COUNT.get());
assertEquals(2, MockConsumerInterceptor.CLOSE_COUNT.get());
} finally {
MockConsumerInterceptor.resetCounters();
}
}
```
```
private static class InterceptorTestConfig extends AbstractConfig {
private final int targetInterceptor = 3;
private static final ConfigDef CONFIG;
private static final String INTERCEPTOR_CLASSES_CONFIG_DOC = "A list
of classes to use as interceptors.";
public static final String INTERCEPTOR_CLASSES_CONFIG =
"interceptor.classes";
public static final String CLIENT_ID_CONFIG = "client.id";
public static final String BOOTSTRAP_SERVERS_CONFIG =
"bootstrap.servers";
static {
CONFIG = new ConfigDef().define(INTERCEPTOR_CLASSES_CONFIG,
Type.LIST,
"",
Importance.LOW,
INTERCEPTOR_CLASSES_CONFIG_DOC);
}
public InterceptorTestConfig(Map<?, ?> props) {
super(CONFIG, props);
}
public int getTargetInterceptor() {
return targetInterceptor;
}
}
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
