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 >>> >>
