Hi Matthias,

I've also made a pass over the KIP, aside from the-other-Matthias's
comment, I'm wondering if you have scenarios that want to distinguish the
two internal topics of the join?

Currently we use "-this" and "-other" suffix for the topics. So for example:

stream1.join(stream2, ...)  // stream1 will be materialized with "-this",
and stream2 with "-other"

While:

stream2.join(stream1, ...)  // stream2 will be materialized with "-this",
and stream1 with "-other"


If we think it is reasonable to require users be aware that the above join
situations are not exactly the same, then the current naming is fine; if we
want them to be mutually reused (I'm not sure if this is a common case?)
then we probably need to consider something new?



Guozhang





On Tue, Feb 13, 2018 at 7:02 PM, Matthias Margush <
matthias.marg...@gmail.com> wrote:

> Thanks for the reminder! I need to do some wordsmithing based on the
> feedback I’ve gotten. I’ll do that soon (hopefully).
> On Tue, Feb 13, 2018 at 1:45 PM Matthias J. Sax <matth...@confluent.io>
> wrote:
>
> > Is there any updates for this KIP?
> >
> > -Matthias
> >
> > On 12/28/17 12:27 PM, Matthias J. Sax wrote:
> > > 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
> > >>>>>
> > >>>>
> > >>>
> > >>>
> > >>
> > >
> >
> >
>



-- 
-- Guozhang

Reply via email to