Replacing
Class<? extend ExternalTransformBuilder>
with
ExternalTransformBuilder
sounds reasonable to me. Looks like an oversight that we introduced the
unnecessary class indirection.
-Max
On 27.07.20 20:45, Chamikara Jayalath wrote:
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 <mailto:[email protected]> +Robert
Bradshaw <mailto:[email protected]> may have more context.
Thanks,
Cham
On Mon, Jul 27, 2020 at 10:48 AM Kenneth Knowles <[email protected]
<mailto:[email protected]>> wrote:
On Mon, Jul 27, 2020 at 10:47 AM Kenneth Knowles <[email protected]
<mailto:[email protected]>> wrote:
On Sun, Jul 26, 2020 at 8:50 PM Kenneth Knowles <[email protected]
<mailto:[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] <mailto:[email protected]>> wrote:
Hi all,
I've been working with +Scott Lukas
<mailto:[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
