[
https://issues.apache.org/jira/browse/KAFKA-8299?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16827367#comment-16827367
]
ASF GitHub Bot commented on KAFKA-8299:
---------------------------------------
C0urante commented on pull request #6644: KAFKA-8299: Add generic-safe
getConfiguredInstance() methods to AbstractConfig
URL: https://github.com/apache/kafka/pull/6644
[Jira](https://issues.apache.org/jira/browse/KAFKA-8299)
The changes here add four new methods to the `AbstractConfig` class that
mirror the existing `getConfiguredInstance(...)` and
`getConfiguredInstances(...)` methods; the only difference is that any
parameter of type `Class<T>` is changed to `TypeLiteral<T>` and that type
checking is done with `TypeUtils.isInstance(...)` instead of
`Class.isInstance(...)`
A new unit test (`AbstractConfigTest.testGenericClassInstantiation()`)
confirms this behavior, including the lack of unchecked cast warnings and the
throwing of a runtime exception when an attempt is made to instantiate a class
that extends from/implements the proper raw type but with improper type
parameters.
### 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:
us...@infra.apache.org
> Add type-safe instantiation of generic classes to AbstractConfig
> ----------------------------------------------------------------
>
> Key: KAFKA-8299
> URL: https://issues.apache.org/jira/browse/KAFKA-8299
> Project: Kafka
> Issue Type: Improvement
> Components: config
> Reporter: Chris Egerton
> Assignee: Chris Egerton
> Priority: Minor
>
> {{AbstractConfig.getConfiguredInstance(String key, Class<T> klass)}} and
> other similar methods isn't type-safe for generic types. For example, the
> following code compiles but generates a runtime exception when the created
> {{Consumer}} is invoked:
>
> {code:java}
> public class KafkaIssueSnippet {
> public static class PrintInt implements Consumer<Integer> {
> @Override
> public void accept(Integer i) {
> System.out.println(i);
> }
> }
> public static void main(String[] args) {
> final String stringConsumerProp = "string.consumer.class";
> AbstractConfig config = new AbstractConfig(
> new ConfigDef().define(
> stringConsumerProp,
> ConfigDef.Type.CLASS,
> ConfigDef.Importance.HIGH,
> "A class that implements Consumer<String>"
> ),
> Collections.singletonMap(
> stringConsumerProp,
> PrintInt.class.getName()
> )
> );
> Consumer<String> stringConsumer = config.getConfiguredInstance(
> stringConsumerProp,
> Consumer.class
> );
> stringConsumer.accept("Oops! ClassCastException");
> }
> }{code}
> The compiler (rightfully so) generates a warning about the unchecked cast
> from {{Consumer}} to {{Consumer<String>}} to indicate that exactly this sort
> of thing may happen, but it would be nice if we didn't have to worry about
> this in the first place and instead had the same guarantees for generic types
> that we do for non-generic types: that either the
> {{getConfiguredInstance(...)}} method returns an object to us that we know
> for sure is an instance of the requested type, or an exception is thrown.
> Apache Commons contains a useful reflection library that could possibly be
> used to bridge this gap; specifically, its
> [TypeUtils|https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/reflect/TypeUtils.html]
> and
> [TypeLiteral|https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/reflect/TypeLiteral.html]
> classes could be used to add new {{getConfiguredInstance}} and
> {{getConfiguredInstances}} methods to the {{AbstractConfig}} class that
> accept instances of {{TypeLiteral}} instead of {{Class}} and then perform
> type checking to ensure that the requested class actually implements/extends
> from the requested type.
> Since this affects public API it's possible a KIP will be required, but the
> changes are pretty lightweight (four new methods that heavily resemble
> existing ones). If a contributor or committer, especially one familiar with
> this section of the codebase, has an opinion on the necessity of a KIP their
> input would be appreciated.
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
