Hi all,
I agree with Arvid, although for point 2 I would be less strict.
@Piotr, for the side note you mentioned, and from the description you
mention in the mail for example I,
it seems that the need to pass parameters in the build() is not an
inherent need of the build pattern but it
can be mitigated by just creating sth like a StreamOperatorConfig (and
not the operator builder itself) on the
client, serialize it, and then at the TM, use the actual
StreamOperator builder with that config to create the
operator. There you can have all the needed parameters and also
perform the validation that
Dawid mention.
Again, this is just from the summary you provided, not from looking at
the code, so I may be missing something.
On the validation, I think that it should happen at the build(), as
this is the only place where
we know all the parameters.
Cheers,
Kostas
On Tue, Aug 27, 2019 at 9:47 AM Arvid Heise <ar...@data-artisans.com> wrote:
>
> Hi all,
>
> I'd like to differentiate between API level builder usage and "internal"
> builder usage (for example, test harness).
>
> For API level builder, in general everything goes, as long as it aligns
> with user expectations. API level usages are also much more discussed, such
> that I'd expect them to be consistent within one API. This freedom is
> especially required when considering APIs for non-java languages.
>
> Now for "internal" usages (which may or may not align with Java Datastream
> etc. usage). I'd like to get a style that is well supported by the
> primarily used IDEs. I don't want to write a new builder from scratch and
> by the looks of it, we will get many more builders.
>
> Furthermore, I'd like to emphasize that the primary use case for using
> builders for me is to mitigate the lack of named arguments in Java, which
> is especially painful for immutable types. In an ideal world, I'd like to
> have all classes immutable and use builders to create new instances if we
> have
> a) too many parameters to be passed (which should become many more once we
> commit even more to DI),
> b) we have meaningful default values, such that we can omit a significant
> amount parameters by using a buidler, or
> c) we have a good amount of optional (=nullable) parameters.
> Obviously, we deviate from that whenever performance considerations demand
> it.
>
> With that my votes for the questions:
> 1. static method only, hide builder ctor
> 2. Intellij and avro use setX() for property X, so I'd go with that. Lombok
> just uses X(), so I wouldn't mind it.
> 3. Mutable approach. Immutable doesn't make much sense to me. Then I can
> directly go with a Wither pattern on the immutable class without builder.
> 4. Private ctor. If it has a builder, it should be used. Exception:
> migration support for some intermediate versions (if we added a builder to
> a class, keep the ctor deprecated public for 1,2 versions).
> 5. no setX in general, we want to have an immutable class. Exceptions where
> due (should be mostly for performance reasons).
>
> Best,
>
> Arvid
>
> On Mon, Aug 26, 2019 at 4:40 PM Piotr Nowojski <pi...@ververica.com> wrote:
>
> > Hi,
> >
> > I agree with Dawid, modulo that I don’t have any preference about point 2
> > - I’m ok even with not enforcing this.
> >
> > One side note about point 4. There are use cases where passing obligatory
> > parameters in the build method itself might make sense:
> >
> > I. - when those parameters can not be or can not be easily passed via the
> > constructor. Good example of that is “builder” pattern for the
> > StreamOperators (StreamOperatorFactory), where factory is constructed on
> > the API level in the client, then it’s being serialised and sent over the
> > network and reconstructed on the TaskManager, where StreamOperator is
> > finally constructed. The issue is that some of the obligatory parameters
> > are only available on the TaskManager, so they can not be passed on a
> > DataStream level in the client.
> > II. - when builder might be used to create multiple instances of the
> > object with different values.
> >
> > Piotrek
> >
> > > On 26 Aug 2019, at 15:12, Jark Wu <imj...@gmail.com> wrote:
> > >
> > > Hi Gyula,
> > >
> > > Thanks for bringing this. I think it would be nice if we have a common
> > > approach to create builder pattern.
> > > Currently, we have a lot of builders but with different tastes.
> > >
> > >> 1. Creating the builder objects:
> > > I prefer option a) too. It would be easier for users to get the builder
> > > instance.
> > >
> > >> 2. Setting properties on the builder:
> > > I don't have a preference for it. But I think there is another option
> > might
> > > be more concise, i.e. "something()" without `with` or `set` prefix.
> > > For example:
> > >
> > > CsvTableSource source = new CsvTableSource.builder()
> > > .path("/path/to/your/file.csv")
> > > .field("myfield", Types.STRING)
> > > .field("myfield2", Types.INT)
> > > .build();
> > >
> > > This pattern is heavily used in flink-table, e.g. `TableSchema`,
> > > `TypeInference`, `BuiltInFunctionDefinition`.
> > >
> > >> 3. Implementing the builder object:
> > > I prefer b) Mutable approach which is simpler from the implementation
> > part.
> > >
> > >
> > > Besides that, I think maybe we can add some other aspects:
> > >
> > > 4. Constructor of the main class.
> > > a) private constructor
> > > b) public constructor
> > >
> > > 5. setXXX methods of the main class
> > > a) setXXX methods are not allowed
> > > b) setXXX methods are allowed.
> > >
> > > I prefer both option a). Because I think one of the reason to have the
> > > builder is that we don't want the constructor public.
> > > A public constructor makes it hard to maintain and evolve compatibly when
> > > adding new parameters, FlinkKafkaProducer is a good example.
> > > For set methods, I think in most cases, we want users to set the fields
> > > eagerly (through the builder) and `setXXX` methods on the main class
> > > is duplicate with the methods on the builder. We should avoid that.
> > >
> > >
> > > Regards,
> > > Jark
> > >
> > >
> > > On Mon, 26 Aug 2019 at 20:18, Gyula Fóra <gyula.f...@gmail.com> wrote:
> > >
> > >> Hi All!
> > >>
> > >> I would like to start a code-style related discussion regarding how we
> > >> implement the builder pattern in the Flink project.
> > >>
> > >> It would be the best to have a common approach, there are some aspects
> > of
> > >> the pattern that come to my mind please feel free to add anything I
> > missed:
> > >>
> > >> 1. Creating the builder objects:
> > >>
> > >> a) Always create using static method in "built" class:
> > >> Here we should have naming guidelines: .builder(..) or
> > >> .xyzBuilder(...)
> > >> b) Always use builder class constructor
> > >> c) Mix: Again we should have some guidelines when to use which
> > >>
> > >> I personally prefer option a) to always have a static method to create
> > the
> > >> builder with static method names that end in builder.
> > >>
> > >> 2. Setting properties on the builder:
> > >>
> > >> a) withSomething(...)
> > >> b) setSomething(...)
> > >> c) other
> > >>
> > >> I don't really have a preference but either a or b for consistency.
> > >>
> > >>
> > >> 3. Implementing the builder object:
> > >>
> > >> a) Immutable -> Creates a new builder object after setting a property
> > >> b) Mutable -> Returns (this) after setting the property
> > >>
> > >> I personally prefer the mutable version as it keeps the builder
> > >> implementation much simpler and it seems to be a very common way of
> > doing
> > >> it.
> > >>
> > >> What do you all think?
> > >>
> > >> Regards,
> > >> Gyula
> > >>
> >
> >
>
> --
>
> Arvid Heise | Senior Software Engineer
>
> <https://www.ververica.com/>
>
> Follow us @VervericaData
>
> --
>
> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
> Conference
>
> Stream Processing | Event Driven | Real Time
>
> --
>
> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
>
> --
> Ververica GmbH
> Registered at Amtsgericht Charlottenburg: HRB 158244 B
> Managing Directors: Dr. Kostas Tzoumas, Dr. Stephan Ewen
