> For Piotr's comment 4. I. I agree with Klou that this sounds rather like a > problem of the builder's usage than a builder problem. I think such a > scenario can easily be solved by introducing a transfer object.
It could be solved, but that would mean we have some kind of builder/factory/descriptor that creates a factory/builder that creates a final object. I think in this specific example I would prefer to add missing runtime parameters in the build/create() method instead of asking people implementing an operator to wrap it into two layers of indirection. However let’s not clutter this discussion, I might be missing something. Feel free to open a Jira tickets/start a new design discussion about this pattern (it’s currently partially implemented in the form of `StreamOperatorFactory` [1]). Piotrek [1] https://issues.apache.org/jira/browse/FLINK-11974 <https://issues.apache.org/jira/browse/FLINK-11974> > On 27 Aug 2019, at 14:43, Till Rohrmann <trohrm...@apache.org> wrote: > > Hi all, > > I would be in favour of the following convention > > 1. a) static method for builder creation > 2. No strict rule because especially for boolean flags it might make sense > to have something like `enableX()` or `withY()` where one doesn't specify a > concrete value. > 3. Mutable builders but if there is a good reason to follow the immutable > approach then I wouldn't forbid it. > 4. Private constructor if there is a builder and no backwards compatibility > constraint > 5. No setX methods on the instance to create > > For Piotr's comment 4. I. I agree with Klou that this sounds rather like a > problem of the builder's usage than a builder problem. I think such a > scenario can easily be solved by introducing a transfer object. > > Cheers, > Till > > On Tue, Aug 27, 2019 at 1:46 PM Timo Walther <twal...@apache.org> wrote: > >> 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 >> >> >>
