Hi all,
great to put this code style discussion on the mailing list because I
also have found this style inconsistent in the past.
Regarding Gyula's suggestions:
1. a static method `builder()` I think IDEs are also hightlight methods
with this name
2. I would vote for a more declarative `propertyA(...).propertyB(...)`
approach instead of setters because most methods should be setters.
However, if implementers want to add methods such as `addField(..)`,
`useProcessingTime()` this sounds also fine to me.
3. mutable
Regarding Dawid's suggestions:
4. regarding required and optional parameters, I would allow both options
5. always end with `build()`
Thanks,
Timo
On 27.08.19 10:04, Kostas Kloudas wrote:
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
