Brian's suggestion makes sense to me. I don't know of a specific reason regarding why we choose the Class type in the registrar instead of instance types. +Maximilian Michels <[email protected]> +Robert Bradshaw <[email protected]> may have more context.
Thanks, Cham On Mon, Jul 27, 2020 at 10:48 AM Kenneth Knowles <[email protected]> wrote: > > > On Mon, Jul 27, 2020 at 10:47 AM Kenneth Knowles <[email protected]> wrote: > >> 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. >> > > Additionally notable that Spotbugs operates on post-compile bytecode, not > source. > > Kenn > > > > >> 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 >>>> >>>
