Hi, thank you very much for all you suggestions. I've started to update the KIP ( https://cwiki.apache.org/confluence/display/KAFKA/KIP-307%3A+Allow+to+define+custom+processor+names+with+KStreams+DSL ). Also, I propose to rename the Processed class into Described - this will be more meaningful (but this is just a detail).
I'm OK to not enforcing uppercase for specific names but should we allow arbitrary names with whitespaces for example ? Currently, I can't tell if this can lead to some side effects ? Le lun. 11 juin 2018 à 01:31, Matthias J. Sax <matth...@confluent.io> a écrit : > Just catching up on this thread. > > I like the general idea. Couple of comments: > > - I think that adding `Processed` (or maybe a different name?) is a > valid proposal for stateless operators that only have a single overload > atm. It would align with the overall API design. > > - for all methods with multiple existing overloads, we can consider to > extend `Consumed`, `Produced`, `Materialized` etc to take an additional > processor name (not sure atm how elegant this is; we would need to > "play" with the API a little bit; the advantage would be, that we do not > add more overloads what seems to be key for this KIP) > > - operators return void: while I agree that the "name first" chaining > idea is not very intuitive, it might still work, if we name the method > correctly (again, we would need to "play" with the API a little bit to see) > > - for DSL operators that are translated to multiple nodes: it might > make sense to use the specified operator name as prefix and add > reasonable suffixes. For example, a join translates into 5 operators > that could be name "name-left-store-processor", > "name-left-join-processor", "name-right-store-processor", > "name-right-join-processor", and "name-join-merge-processor" (or > similar). Maybe just using numbers might also work. > > - I think, we should strip the number suffixes if a user provides names > > - enforcing upper case seems to be tricky: for example, we do not > enforce upper case for store names and we cannot easily change it as it > would break compatibility -- thus, for consistency reasons we might not > want to do this > > - for better understand of the impact of the KIP, it would be quite > helpful if you would list all method names that are affected in the KIP > (ie, list all newly added overloads) > > > -Matthias > > > On 5/31/18 6:40 PM, Guozhang Wang wrote: > > Hi Florian, > > > > Re 1: I think changing the KStreamImpl / KTableImpl to allow modifying > the > > processor name after the operator is fine as long as we do the check > again > > when modifying that. In fact, we are having some topology optimization > > going on which may modify processor names in the final topology anyways ( > > https://github.com/apache/kafka/pull/4983). Semantically I think it is > > easier to understand to developers than "deciding the processor name for > > the next operator". > > > > Re 2: Yeah I'm thinking that for operators that translates to multiple > > processor names, we can still use the provided "hint" to name the > processor > > names, e.g. for Joins we can name them as `join-foo-this` and > > `join-foo-that` etc if user calls `as("foo")`. > > > > Re 3: The motivation I had about removing the suffix is that it has huge > > restrictions on topology compatibilities: consider if user code added a > new > > operator, or library does some optimization to remove some operators, the > > suffix indexing may be changed for a large amount of the processor names: > > this will in turn change the internal state store names, as well as > > internal topic names as well, making the new application topology to be > > incompatible with the ones. One rationale I had about this KIP is that > > aligned this effort, moving forward we can allow users to customize > > internal names so that they can still be reused even with topology > changes > > (e.g. KIP-230), so I think removing the suffix index would be more > > applicable in the long run. > > > > > > > > Guozhang > > > > > > > > > > > > On Thu, May 31, 2018 at 3:08 PM, Florian Hussonnois < > fhussonn...@gmail.com> > > wrote: > > > >> Hi , > >> Thank you very much for your feedback. > >> > >> 1/ > >> I agree that overloading most of the methods with a Processed is not > ideal. > >> I've started modifying the KStream API and I got to the same conclusion. > >> Also ading a new method directly to KStreamImpl and KTableImpl classes > >> seems to be a better option. > >> > >> However a processor name cannot be redefined after calling an operator > (or > >> maybe I miss something in the code). > >> From my understanding, this will only set the KStream name property not > the > >> processor name previsouly added to the topology builder - leading to > >> InvalidTopology exception. > >> > >> So the new method should actually defines the name of the next > processor : > >> Below is an example : > >> > >> *stream.as <http://stream.as>(Processed.name("MAPPE_TO_UPPERCASE")* > >> * .map( (k, v) -> KeyValue.pair(k, v.toUpperCase()))* > >> > >> I think this approach could solve the cases for methods returning void ? > >> > >> Regarding this new method we have two possible implementations : > >> > >> 1. Adding a method like : withName(String processorName) > >> 2. or adding a method accepting an Processed object : as(Processed). > >> > >> I think solution 2. is preferable as the Processed class could be > enriched > >> further (in futur). > >> > >> 2/ > >> As Guozhang said some operators add internal processors. > >> For example the branch() method create one KStreamBranch processor to > route > >> records and one KStreamPassThrough processor for each branch. > >> In that situation only the parent processor can be named. For children > >> processors we could keep the current behaviour that add a suffix (i.e > >> KSTREAM-BRANCHCHILD-) > >> > >> This also the case for the join() method that result to adding multiple > >> processors to the topology (windowing, left/right joins and a merge > >> processor). > >> I think, like for the branch method users could only define a processor > >> name prefix. > >> > >> 3/ > >> I think we should still added a suffix like "-0000000000" to processor > >> name and enforce uppercases as this will keep some consistency with the > >> ones generated by the API. > >> > >> 4/ > >> Yes, the KTable interface should be modified like KStream to allow > custom > >> processor names definition. > >> > >> Thanks, > >> > >> > >> Le jeu. 31 mai 2018 à 19:18, Damian Guy <damian....@gmail.com> a écrit > : > >> > >>> Hi Florian, > >>> > >>> Thanks for the KIP. What about KTable and other DSL interfaces? Will > they > >>> not want to be able to do the same thing? > >>> It would be good to see a complete set of the public API changes. > >>> > >>> Cheers, > >>> Damian > >>> > >>> On Wed, 30 May 2018 at 19:45 Guozhang Wang <wangg...@gmail.com> wrote: > >>> > >>>> Hello Florian, > >>>> > >>>> Thanks for the KIP. I have some meta feedbacks on the proposal: > >>>> > >>>> 1. You mentioned that this `Processed` object will be added to a new > >>>> overloaded variant of all the stateless operators, what about the > >>> stateful > >>>> operators? Would like to hear your opinions if you have thought about > >>> that: > >>>> note for stateful operators they will usually be mapped to multiple > >>>> processor node names, so we probably need to come up with some ways to > >>>> define all their names. > >>>> > >>>> 2. I share the same concern with Bill as for adding lots of new > >> overload > >>>> functions into the stateless operators, as we have just spent quite > >> some > >>>> effort in trimming them since 1.0.0 release. If the goal is to just > >>> provide > >>>> some "hints" on the generated processor node names, not strictly > >>> enforcing > >>>> the exact names that to be generated, then how about we just add a new > >>>> function to `KStream` and `KTable` classes like: "as(Processed)", with > >>> the > >>>> semantics as "the latest operators that generate this KStream / KTable > >>> will > >>>> be named accordingly to this hint". > >>>> > >>>> The only caveat, is that for all operators like `KStream#to` and > >>>> `KStream#print` that returns void, this alternative would not work. > But > >>> for > >>>> the current operators: > >>>> > >>>> a. KStream#print, > >>>> b. KStream#foreach, > >>>> c. KStream#to, > >>>> d. KStream#process > >>>> > >>>> I personally felt that except `KStream#process` users would not > usually > >>>> bother to override their names, and for `KStream#process` we could add > >> an > >>>> overload variant with the additional Processed object. > >>>> > >>>> > >>>> 3. In your example, the processor names are still added with a suffix > >>> like > >>>> " > >>>> -0000000000", is this intentional? If yes, why (I thought with user > >>>> specified processor name hints we will not add suffix to distinguish > >>>> different nodes of the same type any more)? > >>>> > >>>> > >>>> Guozhang > >>>> > >>>> > >>>> On Tue, May 29, 2018 at 6:47 AM, Bill Bejeck <bbej...@gmail.com> > >> wrote: > >>>> > >>>>> Hi Florian, > >>>>> > >>>>> Thanks for the KIP. I think being able to add more context to the > >>>>> processor names would be useful. > >>>>> > >>>>> I like the idea of adding a "withProcessorName" to Produced, Consumed > >>> and > >>>>> Joined. > >>>>> > >>>>> But instead of adding the "Processed" parameter to a large percentage > >>> of > >>>>> the methods, which would result in overloaded methods (which we > >> removed > >>>>> quite a bit with KIP-182) what do you think of adding a method > >>>>> to the AbstractStream class "withName(String processorName)"? BTW I"m > >>> not > >>>>> married to the method name, it's the best I can do off the top of my > >>>> head. > >>>>> > >>>>> For the methods that return void, we'd have to add a parameter, but > >>> that > >>>>> would at least cut down on the number of overloaded methods in the > >> API. > >>>>> > >>>>> Just my 2 cents. > >>>>> > >>>>> Thanks, > >>>>> Bill > >>>>> > >>>>> On Sun, May 27, 2018 at 4:13 PM, Florian Hussonnois < > >>>> fhussonn...@gmail.com > >>>>>> > >>>>> wrote: > >>>>> > >>>>>> Hi, > >>>>>> > >>>>>> I would like to start a new discussion on following KIP : > >>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- > >>>>>> 307%3A+Allow+to+define+custom+processor+names+with+KStreams+DSL > >>>>>> > >>>>>> This is still a draft. > >>>>>> > >>>>>> Looking forward for your feedback. > >>>>>> -- > >>>>>> Florian HUSSONNOIS > >>>>>> > >>>>> > >>>> > >>>> > >>>> > >>>> -- > >>>> -- Guozhang > >>>> > >>> > >> > >> > >> -- > >> Florian HUSSONNOIS > >> > > > > > > > > -- Florian HUSSONNOIS
