Hi John, Thanks for the review! Replied inline.
On Fri, Apr 24, 2020 at 8:09 PM John Roesler <vvcep...@apache.org> wrote: > Hi Guozhang, > > Thanks for the KIP! I took a quick look, and I'm really happy to see this > underway. > > Some quick questions: > > 1. Can you elaborate on the reason that stores just have a list of > serdes, whereas > other components have an explicit key/value serde? > This is because of the existing API "List<Serde> StoreBuilder#serdes()". Although both of its implementations would return two serdes (one for key and one for value), the API is more general to return a list. And hence the TopologyDescription#Store which gets them directly from StoreBuilder is exposing the same API. 1.5. A side-effect of this seems to be that the string-formatted serde > description is > different, depending on whether the serdes are listed on a store or a > topic. Just an > observation. > Yes I agree. I think we can probably change the "List<Serde> StoreBuilder#serdes()" signature as well (which would be a breaking change though, so we should do that via deprecation), but I'm a bit concerned since it was designed for future store types which may not be of K-V format any more. > 2. You mentioned the key compatibility concern in my mind. We do know that > such > use cases exist. Namely, our own tests and > https://zz85.github.io/kafka-streams-viz/ > I'm wondering if we can add a forward-compatible machine-readable format > to the > KIP, so that even though we must break the parsers right now, maybe we'll > never > have to break them again. For example, I'm thinking of a "toJson" method > on the > TopologyDescription that formats the entire topology description as a json > string. > > Yes, I also have concerns about that (as described in the compatibility section). One proposal I have is that we ONLY augment the toString result if the TopologyDescription is from a Topology built from `StreamsBuilder#build(Properties)`, which is only recently added and hence most old usage would not get the benefits of it. But after thinking about this a bit more, I'm now more inclined to just always augment the string, and also add a `toJson` method in addition to `toString`. > Thanks again! > -John > > On Fri, Apr 24, 2020, at 00:26, Guozhang Wang wrote: > > Hello folks, > > > > I'd like to start the discussion for KIP-598: > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=148648762 > > > > It proposes to augment the topology description's sub-classes with store > > and source / sink serde information. Let me know what you think, thanks! > > > > -- > > -- Guozhang > > > -- -- Guozhang
