Timely thread. I just ran into this same API while doing some code gardening in https://github.com/apache/beam/pull/12376
It is almost always good to build your own more-limited interfaces than to have Java's reflection types (Class, Method, Field, etc) on your API surface. The only time you should use those classes is when your APIs are specifically about doing reflection things with them, which is not the case here. An interface dedicated to what you are trying to accomplish is simpler and more informative than Class<...> For my PR, the issue is that class literal Foo.class has the rawtype Class<Foo>. Rawtypes are a legacy compatibility feature that breaks type checking (and further analyses) and harms readability. They should be forbidden in new code. Class literals for generic types are quite inconvenient for this, especially when placed in a heterogeneous map using wildcard parameters [1]. So making either the change Brian proposes or something similar is desirable, to avoid forcing inconvenience on users of the API, and to just simplify and clarify it. Kenn [1] https://github.com/apache/beam/pull/12376/files#diff-2fa38a7f8d24217f1f7bde0f5c7dbb40R495 Kenn On Fri, Jul 24, 2020 at 11:04 AM Brian Hulette <[email protected]> wrote: > Hi all, > I've been working with +Scott Lukas <[email protected]> on using the new > schema io interfaces [1] in cross-language. This means adding a > general-purpose ExternalTransformRegistrar [2,3] that will register all > SchemaIOProvider implementations via ServiceLoader. > > We've run into an issue though - ExternalTransformRegistrar is supposed to > return a `Map<String, Class<? extends ExternalTransformBuilder>>`. This > makes it very challenging (impossible?) for us to create a general-purpose > ExternalTransformBuilder that defers to SchemaIOProvider. Ideally we would > instead return a Map<String, ExternalTransformBuilder> (i.e. a concrete > instance rather than a class object), so that we could just register > different instances of a class like: > > class SchemaIOBuilder extends ExternalTransformBuilder { > private SchemaIOProvider provider; > > PTransform<InputT, OutputT> buildExternal(ConfigT configuration) { > // Use provider to create PTransform > } > } > > I think it would be possible to change the ExternalTransformRegistrar > interface so it has a single method, Map<String, ExternalTransformBuilder> > knownBuilders(). It could even be done in a backwards-compatible way if we > keep the old method and provide a default implementation of the new method > that builds instances. > > However, I'm curious if there's some strong reason for using Class<? > extends ExternalTransformBuilder> as the return type for knownBuilders that > I'm missing. Does anyone know why we chose that? > > Thanks, > Brian > > [1] https://s.apache.org/beam-schema-io > [2] > https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/ExternalTransformRegistrar.java > [3] > https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/ExternalTransformBuilder.java >
