Thanks for updating the KIP. The code-diff is a little hard to read. It's better to so something similar as in this KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor
(Just as an example. Maybe take a look as other KIPs, too.) Some side remarks: - please update the link to the DISCUSS thread - there are some typos: Kstream -> KStream; Topology Builder exception -> TopologyBuilderException You propose to add `otherValueSerde(final String joinName)` -- I guess the method name is a c&p error and method name must be updated? Changes to internal classes like `KStreamImpl` are not required in the KIP as those as implementation details. The KIP should focus on public changes. -Matthias On 12/26/17 11:19 AM, Matthias Margush wrote: > Greetings. > > Thanks for the comments and suggestions. I updated the KIP with these > proposals for the questions posed by Matt & Matthias: > > *Can you please c&p the corresponding content instead of just > putting links? A KIP should be a self-contained Wiki page. Also, if we add > a optional config parameter, how would we specify it? **Please list all > changes to want to apply to `Joined` class.* > > I added more details around the proposed changes directly to the KIP. > > *I will point out that your KIP doesn't outline what would happen if > you picked a name that resulted in a non unique topic name? What would be > the error handling behavior there?* > > Looking at the current behavior of methods that allow the user to specify > names for internal resources (e.g. `reduce`, `aggregate`), I added a > proposal that the code generate a similar exception if a name conflict is > detected in the topology: > > org.apache.kafka.streams.errors.TopologyBuilderException: "Invalid topology > building: Topic reduction-same-name-repartition has already been registered > by another source." > > *What is the impact on KStream-KTable join?* > > Proposed that kstream-ktable joins similarly make use of the provided > joinName when generating internal repartition topics. > > On Mon, Dec 4, 2017 at 2:57 PM Matthias J. Sax <matth...@confluent.io> > wrote: > >> Matthias, >> >> thanks for the KIP. >> >> Can you please c&p the corresponding content instead of just putting >> links? A KIP should be a self-contained Wiki page. >> >> Also, if we add a optional config parameter, how would we specify it? >> Please list all changes to want to apply to `Joined` class. >> >> Furthermore, `Joined` is also used for KStream-KTable join but the KIP >> only talks about windowed joins (ie, KStream-KTream join). What the >> impact on KStream-KTable join? >> >> >> -Matthias >> >> On 11/29/17 6:09 AM, Matt Farmer wrote: >>> Hi Matthias, >>> >>> I certainly have found the auto-generated names unwieldy while doing >>> cluster administration. >>> >>> I will point out that your KIP doesn't outline what would happen if you >>> picked a name that resulted in a non unique topic name? What would be the >>> error handling behavior there? >>> >>> On Wed, Nov 29, 2017 at 9:03 AM Matthias Margush < >> matthias.marg...@gmail.com> >>> wrote: >>> >>>> Hi everyone, >>>> >>>> I created this KIP to allow windowing joins to be named. If named, then >> the >>>> associated internal topic names would be derived from that, instead of >>>> being randomly generated. >>>> >>>> >>>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP+230%3A+Name+Windowing+Joins >>>> >>>> Thanks, >>>> >>>> Matthias >>>> >>> >> >> >
signature.asc
Description: OpenPGP digital signature
