On Sun, Jul 26, 2020 at 8:50 PM Kenneth Knowles <[email protected]> wrote:
> Rawtypes are a legacy compatibility feature that breaks type checking (and > further analyses) > Noting for the benefit of the thread that this is not hypothetical. Fixing the rawtypes in this API surfaced nullability issues according to spotbugs. Kenn 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 >> >
