Thanks Matthias. 1) Given that TopologyDescription is for debugging purposes before `KafkaStreams.start()`. I think the simplest option b) may be sufficient. Just needs to emphasize its possible value semantics in Java docs.
3) You can tell that I was actually thinking about this together with KIP-130. To me if we can expose the runtime information, which is dynamic, via metrics in KIP-130 then we could remove this function. The main reason is that, again, the task migration make this function's behavior a bit difficult to explain. For example: streams.start(); sleep(/* some time */) streams.toString(); --------------------------- Even with the same configuration, depending on for how long did you wait after started, the function could return very different string results due to rebalances. That being said, I was not trying to make the decision in this KIP, as I saw it more related to KIP-130. So we could probably still keep it as is in KIP-120, and consider removing it in KIP-130. That's why I was just "asking your thoughts on this", but not necessary wanting to make an action in this KIP. Guozhang On Sat, Mar 11, 2017 at 11:10 AM, Matthias J. Sax <matth...@confluent.io> wrote: > Thanks for your feedback Guozhang. > > > 1) There are multiple ways to do this. Let me know what you think about > all options: > > (a) I updated the KIP to this: > > > public final class Source implements Node { > > public final String name; > > // topicNames and topicPattern are mutually exclusive, i.e., > only one will be not-null > > public final List<String> topicNames; // null if #addSource(..., > Pattern) was used > > public final Pattern topicPattern; // null if #addSource(..., > String...) was used > > } > > (b) We could also go with a single variable (as originally proposed). > This would have the advantage (compared to (a)), that null checks are > not required accessing TopologyDescription#Source class. > > > String topics; // can be comma separated list of topic names or pattern > (as String) > > However, with an encoded list or an encoded pattern it's required to > parse the string again, what we want to avoid in the first place. > > (c) Use a single variable as in (b) > > > String topics; // always a pattern (as String) > > We translate a list of topic names into a pattern > "topic1|topic2|topic3". We loose the information if the source was added > via list or via pattern. > > > > 2) Your understanding is correct. Added a comment to the KIP. > > > > 3) I would keep KafkaStreams#toString() -- it's conceptually two > different things and runtime information is useful, too. But as its > return value is ambiguous to parse (and must be parsed in the first > place what is cumbersome), we could add KafkaStreams#describe() as > > > public synchronized KafkaStreamsDescription describe(); > > KafkaStreamsDescription class would be similar to TopologyDescription to > allow programmatic access to runtime information. I guess we could even > reuse (parts of) TopologyDescription within KafkaStreamsDescription to > avoid code duplication. > > If you think this would be useful, I can extend the KIP accordingly. > > > > -Matthias > > > > > On 3/10/17 1:38 PM, Guozhang Wang wrote: > > One more question here: > > > > 3. with TopologyDescription, do we still want to keep the > > `KafkaStream.toString()` function? I think it may still have some > advantage > > such that it contains tasks information after `KafkaStream#start()` has > > been called, but much of it is duplicate with the TopologyDescription, > and > > it is only in the form of the string hence hard to programmatically > > leverage. So would like to hear your thoughts. > > > > Guozhang > > > > On Thu, Mar 9, 2017 at 11:20 PM, Guozhang Wang <wangg...@gmail.com> > wrote: > > > >> Thanks Matthias, the updated KIP lgtm overall. A couple of minor > comments: > >> > >> 1. With regard to this class: > >> > >> public final class Source implements Node { > >> public final String name; > >> public final String topic; // can be topic name or pattern (as > >> String) > >> } > >> > >> Note that the source node could contain more than a single topic, i.e. a > >> list of topics besides a pattern. > >> > >> 2. With regard to > >> > >> public synchronized TopologyDescription describe(); > >> > >> My understand is that whenever the topology is modified, one needs to > call > >> this function again to get a new description object, as the old one > won't > >> be updated automatically. Hence the usage pattern would be: > >> > >> TopologyDescription description = topology.describe(); > >> > >> topology.addProcessor(...) > >> > >> description = topology.describe(); // have to call again > >> > >> ----------- > >> > >> Is that right? If yes could you clarify this in the wiki? > >> > >> > >> > >> Guozhang > >> > >> On Thu, Mar 9, 2017 at 2:30 AM, Michael Noll <mich...@confluent.io> > wrote: > >> > >>> Thanks for the update, Matthias. > >>> > >>> +1 to the points 1,2,3,4 you mentioned. > >>> > >>> Naming is always a tricky subject, but renaming KStreamBuilder > >>> to StreamsTopologyBuilder looks ok to me (I would have had a slight > >>> preference towards DslTopologyBuilder, but hey.) The most important > >>> aspect > >>> is, IMHO, what you also pointed out: to make it clear that the current > >>> KStreamBuilder actually builds a topology (though currently the latter > is > >>> actually called `TopologyBuilder` currently), and not a `KStream`. > >>> > >>> > >>> > >>> > >>> > >>> On Wed, Mar 8, 2017 at 11:43 PM, Matthias J. Sax < > matth...@confluent.io> > >>> wrote: > >>> > >>>> Hi, > >>>> > >>>> sorry for not replying earlier and thanks for all your feedback. After > >>>> some more discussions I updated the KIP. The new proposal puts some > >>>> other design considerations into account, that I want to highlight > >>>> shortly. Those considerations, automatically resolve the concerns > >>> raised. > >>>> > >>>> First some answers: > >>>> > >>>>> The PAPI processors I use in my KStreams app are all functioning on > >>>> KTable > >>>>> internals. I wouldn't be able to convert them to > >>> process()/transform(). > >>>>> > >>>>> What's the harm in permitting both APIs to be used in the same > >>>> application? > >>>> > >>>> It's not about "harm" but about design. We want to switch from a > >>>> "inheritance" to a "composition" pattern. > >>>> > >>>> About the interface idea: using a shared interface would not help to > get > >>>> a composition pattern > >>>> > >>>> > >>>> Next I want to give the design considerations leading to the updated > >>> KIP: > >>>> > >>>> 1) Using KStreamBuilder in the constructor of KafkaStreams is > unnatural. > >>>> KafkaStreams client executes a `Topology` and this execution should be > >>>> independent of the way the topology is "put together", ie, low-level > API > >>>> or DSL. > >>>> > >>>> 2) Thus, we don't want to have any changes to KafkaStreams class. > >>>> > >>>> 3) Thus, KStreamBuilder needs to have a method `build()` that returns > a > >>>> `Topology` that can be passed into KafakStreams. > >>>> > >>>> 4) Because `KStreamBuilder` should build a `Topology` I suggest to > >>>> rename the new class to `StreamsTopologyBuilder` (the name > >>>> TopologyBuilder would actually be more natural, but would be easily > >>>> confused with old low-level API TopologyBuilder). > >>>> > >>>> Thus, PAPI and DSL can be mixed-and-matched with full power, as > >>>> StreamsTopologyBuilder return the created Topology via #build(). > >>>> > >>>> I also removed `final` for both builder classes. > >>>> > >>>> > >>>> > >>>> With regard to the larger scope of the overal API redesign, I also > want > >>>> to point to a summary of API issues: > >>>> https://cwiki.apache.org/confluence/display/KAFKA/ > >>>> Kafka+Streams+Discussions > >>>> > >>>> Thus, this KIP is only one building block of a larger improvement > >>>> effort, and we hope to get as much as possible done for 0.11. If you > >>>> have any API improvement ideas, please share them so we can come up > with > >>>> an holistic sound design (instead of uncoordinated local improvements > >>>> that might diverge) > >>>> > >>>> > >>>> > >>>> Looking forward to your feedback on this KIP and the other API issues. > >>>> > >>>> > >>>> > >>>> -Matthias > >>>> > >>>> > >>>> > >>>> > >>>> On 2/15/17 7:36 PM, Mathieu Fenniak wrote: > >>>>> On Wed, Feb 15, 2017 at 5:04 PM, Matthias J. Sax < > >>> matth...@confluent.io> > >>>>> wrote: > >>>>> > >>>>>> - We also removed method #topologyBuilder() from KStreamBuilder > >>> because > >>>>>> we think #transform() should provide all functionality you need to > >>>>>> mix-an-match Processor API and DSL. If there is any further concern > >>>>>> about this, please let us know. > >>>>>> > >>>>> > >>>>> Hi Matthias, > >>>>> > >>>>> Yes, I'm sorry I didn't respond sooner, but I still have a lot of > >>>> concerns > >>>>> about this. You're correct to point out that transform() can be used > >>> for > >>>>> some of the output situations I pointed out; albeit it seems somewhat > >>>>> awkward to do so in a "transform" method; what do you do with the > >>> retval? > >>>>> > >>>>> The PAPI processors I use in my KStreams app are all functioning on > >>>> KTable > >>>>> internals. I wouldn't be able to convert them to > >>> process()/transform(). > >>>>> > >>>>> What's the harm in permitting both APIs to be used in the same > >>>> application? > >>>>> > >>>>> Mathieu > >>>>> > >>>> > >>>> > >>> > >> > >> > >> > >> -- > >> -- Guozhang > >> > > > > > > > > -- -- Guozhang