Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Leah Thomas]

Ahh, that makes sense. Thanks for clarifying!

On Thu, Dec 3, 2020 at 3:01 PM Matthias J. Sax  wrote:

> Thanks for looking into it. I also needed to refresh my memory.
>
> In Scala, it's sufficient to expose the `static` methods for a native
> Scala integration. If you look into the code, those static methods
> return the actually Java objects, and thus all non-static Java methods
> are available automatically.
>
> As we only add new non-static methods, we don't need to do anything for
> the Scala API, and the new methods will be available for Scala users
> automatically.
>
>
> -Matthias
>
> On 12/3/20 7:16 AM, Leah Thomas wrote:
> > Thanks for thinking of that Matthias. After looking at what we currently
> > have for `StreamJoined` in scala, we just have bare bones functionality,
> > allowing users to use `StreamJoined` through the two `with()` functions
> and
> > one `as()` function. This is the same for `Materialized` in scala, which
> > does not include implementation for `withLoggingEnabled()` etc. Neither
> of
> > these scala versions include `withKeySerde()` or `withValueSerde()`.
> >
> > I'm not sure if we consciously made the decision to not implement
> > additional configs for `Materialized` and `StreamJoined` in Scala, but I
> > don't think it makes a lot of sense to add functionality for
> > `withLoggingEnabled()` and `withLoggingDisabled()` if the same cannot be
> > said for Materialized or the other configs we allow in the Java API. My
> > thought is to leave it out of this KIP and, if we so choose, create a
> > follow-up ticket to implement all the additional functionality at once
> for
> > the Scala API. WDYT?
> >
> > On Wed, Dec 2, 2020 at 5:40 PM Matthias J. Sax  wrote:
> >
> >> One more follow up: do we need to update the Scala API, too?
> >>
> >>
> >> -Matthias
> >>
> >> On 12/2/20 7:51 AM, Leah Thomas wrote:
> >>> Thanks for the feedback! I'll go ahead and move it to vote now.
> >>>
> >>> Best,
> >>> Leah
> >>>
> >>> On Wed, Dec 2, 2020 at 6:58 AM Bruno Cadonna 
> wrote:
> >>>
>  Thanks for the KIP!
> 
>  LGTM, as well!
> 
>  Best,
>  Bruno
> 
>  On 02.12.20 00:41, Walker Carlson wrote:
> > Thanks for making these changes. It makes more sense now to me.
> Overall
>  LGTM
> >
> > walker
> >
> > On Tue, Dec 1, 2020 at 3:39 PM Sophie Blee-Goldman <
> >> sop...@confluent.io>
> > wrote:
> >
> >> Thanks for the KIP! I'm happy with the state of things after your
> >> latest
> >> update,
> >> LGTM
> >>
> >> Sophie
> >>
> >> On Tue, Dec 1, 2020 at 2:26 PM Leah Thomas 
>  wrote:
> >>
> >>> Hi Matthias,
> >>>
> >>> Yeah I think it should, good catch. That should also answer
> Walker's
> >>> question about why we have an option for `withLoggingEnabled()`
> even
> >> though
> >>> that's the default. Passing in a new map of configs could allow the
>  user
> >> to
> >>> configure the log differently than the default. I've updated the
> KIP
> >> to
> >>> reflected the added parameter and an added variable, `topicConfig`
> to
> >> store
> >>> the map of configs.
> >>>
> >>> Best,
> >>> Leah
> >>>
> >>> On Mon, Nov 30, 2020 at 5:35 PM Matthias J. Sax 
> >> wrote:
> >>>
>  Thanks for the KIP Leah.
> 
>  Should `withLoggingEnabled()` take a `Map config`
>  similar to the one from `Materialized`?
> 
> 
>  -Matthias
> 
>  On 11/30/20 12:22 PM, Walker Carlson wrote:
> > Ah. That makes sense. Thank you for fixing that.
> >
> > One minor question. If the default is enabled is there any case
> >> where a
> > user would turn logging off then back on again? I can see having
> >> the
> > enableLoging for completeness so it's not that important
> probably.
> >
> > Anyways other than that it looks good!
> >
> > Walker
> >
> > On Mon, Nov 30, 2020 at 12:06 PM Leah Thomas <
> ltho...@confluent.io
> >>>
>  wrote:
> >
> >> Hey Walker,
> >>
> >> Thanks for your response.
> >>
> >> 1. Ah yeah thanks for the catch, that was held over from
> >> copy/paste.
>  Both
> >> functions should take no parameters, as they just
> `loggingEnabled`
> >> to
>  true
> >> or false. I've removed the `WindowBytesStoreSupplier
> >>> otherStoreSupplier`
> >> from the methods in the KIP
> >> 2. I think the fix to 1 answers this question, otherwise, I'm
> not
> >>> quite
> >> sure what you're asking. With the updated method calls, there
> >>> shouldn't
>  be
> >> any duplication.
> >>
> >> Cheers,
> >> Leah
> >>
> >> On Mon, Nov 30, 2020 at 12:14 PM Walker Carlson <
> >>> wcarl...@confluent.io>
> >> wrote:
> 

Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Matthias J. Sax]

Thanks for looking into it. I also needed to refresh my memory.

In Scala, it's sufficient to expose the `static` methods for a native
Scala integration. If you look into the code, those static methods
return the actually Java objects, and thus all non-static Java methods
are available automatically.

As we only add new non-static methods, we don't need to do anything for
the Scala API, and the new methods will be available for Scala users
automatically.


-Matthias

On 12/3/20 7:16 AM, Leah Thomas wrote:
> Thanks for thinking of that Matthias. After looking at what we currently
> have for `StreamJoined` in scala, we just have bare bones functionality,
> allowing users to use `StreamJoined` through the two `with()` functions and
> one `as()` function. This is the same for `Materialized` in scala, which
> does not include implementation for `withLoggingEnabled()` etc. Neither of
> these scala versions include `withKeySerde()` or `withValueSerde()`.
> 
> I'm not sure if we consciously made the decision to not implement
> additional configs for `Materialized` and `StreamJoined` in Scala, but I
> don't think it makes a lot of sense to add functionality for
> `withLoggingEnabled()` and `withLoggingDisabled()` if the same cannot be
> said for Materialized or the other configs we allow in the Java API. My
> thought is to leave it out of this KIP and, if we so choose, create a
> follow-up ticket to implement all the additional functionality at once for
> the Scala API. WDYT?
> 
> On Wed, Dec 2, 2020 at 5:40 PM Matthias J. Sax  wrote:
> 
>> One more follow up: do we need to update the Scala API, too?
>>
>>
>> -Matthias
>>
>> On 12/2/20 7:51 AM, Leah Thomas wrote:
>>> Thanks for the feedback! I'll go ahead and move it to vote now.
>>>
>>> Best,
>>> Leah
>>>
>>> On Wed, Dec 2, 2020 at 6:58 AM Bruno Cadonna  wrote:
>>>
 Thanks for the KIP!

 LGTM, as well!

 Best,
 Bruno

 On 02.12.20 00:41, Walker Carlson wrote:
> Thanks for making these changes. It makes more sense now to me. Overall
 LGTM
>
> walker
>
> On Tue, Dec 1, 2020 at 3:39 PM Sophie Blee-Goldman <
>> sop...@confluent.io>
> wrote:
>
>> Thanks for the KIP! I'm happy with the state of things after your
>> latest
>> update,
>> LGTM
>>
>> Sophie
>>
>> On Tue, Dec 1, 2020 at 2:26 PM Leah Thomas 
 wrote:
>>
>>> Hi Matthias,
>>>
>>> Yeah I think it should, good catch. That should also answer Walker's
>>> question about why we have an option for `withLoggingEnabled()` even
>> though
>>> that's the default. Passing in a new map of configs could allow the
 user
>> to
>>> configure the log differently than the default. I've updated the KIP
>> to
>>> reflected the added parameter and an added variable, `topicConfig` to
>> store
>>> the map of configs.
>>>
>>> Best,
>>> Leah
>>>
>>> On Mon, Nov 30, 2020 at 5:35 PM Matthias J. Sax 
>> wrote:
>>>
 Thanks for the KIP Leah.

 Should `withLoggingEnabled()` take a `Map config`
 similar to the one from `Materialized`?


 -Matthias

 On 11/30/20 12:22 PM, Walker Carlson wrote:
> Ah. That makes sense. Thank you for fixing that.
>
> One minor question. If the default is enabled is there any case
>> where a
> user would turn logging off then back on again? I can see having
>> the
> enableLoging for completeness so it's not that important probably.
>
> Anyways other than that it looks good!
>
> Walker
>
> On Mon, Nov 30, 2020 at 12:06 PM Leah Thomas >>
 wrote:
>
>> Hey Walker,
>>
>> Thanks for your response.
>>
>> 1. Ah yeah thanks for the catch, that was held over from
>> copy/paste.
 Both
>> functions should take no parameters, as they just `loggingEnabled`
>> to
 true
>> or false. I've removed the `WindowBytesStoreSupplier
>>> otherStoreSupplier`
>> from the methods in the KIP
>> 2. I think the fix to 1 answers this question, otherwise, I'm not
>>> quite
>> sure what you're asking. With the updated method calls, there
>>> shouldn't
 be
>> any duplication.
>>
>> Cheers,
>> Leah
>>
>> On Mon, Nov 30, 2020 at 12:14 PM Walker Carlson <
>>> wcarl...@confluent.io>
>> wrote:
>>
>>> Hello Leah,
>>>
>>> Thank you for the KIP.
>>>
>>> I had a couple questions that maybe you can expand on from what
>> is
>> on
 the
>>> KIP.
>>>
>>> 1) Why are we enabling/disabling the logging by passing in a
>>> `WindowBytesStoreSupplier`?
>>> It seems to me that these two things should be separate.
>>>
>>> 2) There 

Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Leah Thomas]

Thanks for thinking of that Matthias. After looking at what we currently
have for `StreamJoined` in scala, we just have bare bones functionality,
allowing users to use `StreamJoined` through the two `with()` functions and
one `as()` function. This is the same for `Materialized` in scala, which
does not include implementation for `withLoggingEnabled()` etc. Neither of
these scala versions include `withKeySerde()` or `withValueSerde()`.

I'm not sure if we consciously made the decision to not implement
additional configs for `Materialized` and `StreamJoined` in Scala, but I
don't think it makes a lot of sense to add functionality for
`withLoggingEnabled()` and `withLoggingDisabled()` if the same cannot be
said for Materialized or the other configs we allow in the Java API. My
thought is to leave it out of this KIP and, if we so choose, create a
follow-up ticket to implement all the additional functionality at once for
the Scala API. WDYT?

On Wed, Dec 2, 2020 at 5:40 PM Matthias J. Sax  wrote:

> One more follow up: do we need to update the Scala API, too?
>
>
> -Matthias
>
> On 12/2/20 7:51 AM, Leah Thomas wrote:
> > Thanks for the feedback! I'll go ahead and move it to vote now.
> >
> > Best,
> > Leah
> >
> > On Wed, Dec 2, 2020 at 6:58 AM Bruno Cadonna  wrote:
> >
> >> Thanks for the KIP!
> >>
> >> LGTM, as well!
> >>
> >> Best,
> >> Bruno
> >>
> >> On 02.12.20 00:41, Walker Carlson wrote:
> >>> Thanks for making these changes. It makes more sense now to me. Overall
> >> LGTM
> >>>
> >>> walker
> >>>
> >>> On Tue, Dec 1, 2020 at 3:39 PM Sophie Blee-Goldman <
> sop...@confluent.io>
> >>> wrote:
> >>>
>  Thanks for the KIP! I'm happy with the state of things after your
> latest
>  update,
>  LGTM
> 
>  Sophie
> 
>  On Tue, Dec 1, 2020 at 2:26 PM Leah Thomas 
> >> wrote:
> 
> > Hi Matthias,
> >
> > Yeah I think it should, good catch. That should also answer Walker's
> > question about why we have an option for `withLoggingEnabled()` even
>  though
> > that's the default. Passing in a new map of configs could allow the
> >> user
>  to
> > configure the log differently than the default. I've updated the KIP
> to
> > reflected the added parameter and an added variable, `topicConfig` to
>  store
> > the map of configs.
> >
> > Best,
> > Leah
> >
> > On Mon, Nov 30, 2020 at 5:35 PM Matthias J. Sax 
>  wrote:
> >
> >> Thanks for the KIP Leah.
> >>
> >> Should `withLoggingEnabled()` take a `Map config`
> >> similar to the one from `Materialized`?
> >>
> >>
> >> -Matthias
> >>
> >> On 11/30/20 12:22 PM, Walker Carlson wrote:
> >>> Ah. That makes sense. Thank you for fixing that.
> >>>
> >>> One minor question. If the default is enabled is there any case
>  where a
> >>> user would turn logging off then back on again? I can see having
> the
> >>> enableLoging for completeness so it's not that important probably.
> >>>
> >>> Anyways other than that it looks good!
> >>>
> >>> Walker
> >>>
> >>> On Mon, Nov 30, 2020 at 12:06 PM Leah Thomas  >
> >> wrote:
> >>>
>  Hey Walker,
> 
>  Thanks for your response.
> 
>  1. Ah yeah thanks for the catch, that was held over from
> copy/paste.
> >> Both
>  functions should take no parameters, as they just `loggingEnabled`
>  to
> >> true
>  or false. I've removed the `WindowBytesStoreSupplier
> > otherStoreSupplier`
>  from the methods in the KIP
>  2. I think the fix to 1 answers this question, otherwise, I'm not
> > quite
>  sure what you're asking. With the updated method calls, there
> > shouldn't
> >> be
>  any duplication.
> 
>  Cheers,
>  Leah
> 
>  On Mon, Nov 30, 2020 at 12:14 PM Walker Carlson <
> > wcarl...@confluent.io>
>  wrote:
> 
> > Hello Leah,
> >
> > Thank you for the KIP.
> >
> > I had a couple questions that maybe you can expand on from what
> is
>  on
> >> the
> > KIP.
> >
> > 1) Why are we enabling/disabling the logging by passing in a
> > `WindowBytesStoreSupplier`?
> > It seems to me that these two things should be separate.
> >
> > 2) There is already `withThisStoreSupplier(final
> >> WindowBytesStoreSupplier
> > otherStoreSupplier)` and `withOtherStoreSupplier(final
> > WindowBytesStoreSupplier otherStoreSupplier)`. Why do we need to
>  duplicate
> > them when the `retentionPeriod` can be set through them?
> >
> > Thanks,
> > Walker
> >
> > On Mon, Nov 30, 2020 at 8:53 AM Leah Thomas <
> ltho...@confluent.io>
>  wrote:
> >
> >> After reading through
> >> 

Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Matthias J. Sax]

One more follow up: do we need to update the Scala API, too?


-Matthias

On 12/2/20 7:51 AM, Leah Thomas wrote:
> Thanks for the feedback! I'll go ahead and move it to vote now.
> 
> Best,
> Leah
> 
> On Wed, Dec 2, 2020 at 6:58 AM Bruno Cadonna  wrote:
> 
>> Thanks for the KIP!
>>
>> LGTM, as well!
>>
>> Best,
>> Bruno
>>
>> On 02.12.20 00:41, Walker Carlson wrote:
>>> Thanks for making these changes. It makes more sense now to me. Overall
>> LGTM
>>>
>>> walker
>>>
>>> On Tue, Dec 1, 2020 at 3:39 PM Sophie Blee-Goldman 
>>> wrote:
>>>
 Thanks for the KIP! I'm happy with the state of things after your latest
 update,
 LGTM

 Sophie

 On Tue, Dec 1, 2020 at 2:26 PM Leah Thomas 
>> wrote:

> Hi Matthias,
>
> Yeah I think it should, good catch. That should also answer Walker's
> question about why we have an option for `withLoggingEnabled()` even
 though
> that's the default. Passing in a new map of configs could allow the
>> user
 to
> configure the log differently than the default. I've updated the KIP to
> reflected the added parameter and an added variable, `topicConfig` to
 store
> the map of configs.
>
> Best,
> Leah
>
> On Mon, Nov 30, 2020 at 5:35 PM Matthias J. Sax 
 wrote:
>
>> Thanks for the KIP Leah.
>>
>> Should `withLoggingEnabled()` take a `Map config`
>> similar to the one from `Materialized`?
>>
>>
>> -Matthias
>>
>> On 11/30/20 12:22 PM, Walker Carlson wrote:
>>> Ah. That makes sense. Thank you for fixing that.
>>>
>>> One minor question. If the default is enabled is there any case
 where a
>>> user would turn logging off then back on again? I can see having the
>>> enableLoging for completeness so it's not that important probably.
>>>
>>> Anyways other than that it looks good!
>>>
>>> Walker
>>>
>>> On Mon, Nov 30, 2020 at 12:06 PM Leah Thomas 
>> wrote:
>>>
 Hey Walker,

 Thanks for your response.

 1. Ah yeah thanks for the catch, that was held over from copy/paste.
>> Both
 functions should take no parameters, as they just `loggingEnabled`
 to
>> true
 or false. I've removed the `WindowBytesStoreSupplier
> otherStoreSupplier`
 from the methods in the KIP
 2. I think the fix to 1 answers this question, otherwise, I'm not
> quite
 sure what you're asking. With the updated method calls, there
> shouldn't
>> be
 any duplication.

 Cheers,
 Leah

 On Mon, Nov 30, 2020 at 12:14 PM Walker Carlson <
> wcarl...@confluent.io>
 wrote:

> Hello Leah,
>
> Thank you for the KIP.
>
> I had a couple questions that maybe you can expand on from what is
 on
>> the
> KIP.
>
> 1) Why are we enabling/disabling the logging by passing in a
> `WindowBytesStoreSupplier`?
> It seems to me that these two things should be separate.
>
> 2) There is already `withThisStoreSupplier(final
>> WindowBytesStoreSupplier
> otherStoreSupplier)` and `withOtherStoreSupplier(final
> WindowBytesStoreSupplier otherStoreSupplier)`. Why do we need to
 duplicate
> them when the `retentionPeriod` can be set through them?
>
> Thanks,
> Walker
>
> On Mon, Nov 30, 2020 at 8:53 AM Leah Thomas 
 wrote:
>
>> After reading through
>> https://issues.apache.org/jira/browse/KAFKA-9921
 I
>> removed the option to enable/disable caching for `StreamJoined`,
 as
> caching
>> will always be disabled because we retain duplicates.
>>
>> I updated the KIP accordingly, it now adds only `enableLogging`
 as a
>> config.
>>
>> On Mon, Nov 30, 2020 at 9:54 AM Leah Thomas 
> wrote:
>>
>>> Hi all,
>>>
>>> I'd like to kick-off the discussion for KIP-689: Extend
 `StreamJoined`
> to
>>> allow more store configs. This builds off the work of KIP-479
>>> <
>>
>

>>
>

>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join
>>
>> to
>>> add options to enable/disable both logging and caching for stream
 join
>>> stores.
>>>
>>> KIP is here:
>>>
>>
>

>>
>

>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-689%3A+Extend+%60StreamJoined%60+to+allow+more+store+configs
>>>
>>>
>>> Looking forward to hearing your thoughts,
>>> Leah
>>>
>>
>

>>>
>>
>

>>>
>>
> 

Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Leah Thomas]

Thanks for the feedback! I'll go ahead and move it to vote now.

Best,
Leah

On Wed, Dec 2, 2020 at 6:58 AM Bruno Cadonna  wrote:

> Thanks for the KIP!
>
> LGTM, as well!
>
> Best,
> Bruno
>
> On 02.12.20 00:41, Walker Carlson wrote:
> > Thanks for making these changes. It makes more sense now to me. Overall
> LGTM
> >
> > walker
> >
> > On Tue, Dec 1, 2020 at 3:39 PM Sophie Blee-Goldman 
> > wrote:
> >
> >> Thanks for the KIP! I'm happy with the state of things after your latest
> >> update,
> >> LGTM
> >>
> >> Sophie
> >>
> >> On Tue, Dec 1, 2020 at 2:26 PM Leah Thomas 
> wrote:
> >>
> >>> Hi Matthias,
> >>>
> >>> Yeah I think it should, good catch. That should also answer Walker's
> >>> question about why we have an option for `withLoggingEnabled()` even
> >> though
> >>> that's the default. Passing in a new map of configs could allow the
> user
> >> to
> >>> configure the log differently than the default. I've updated the KIP to
> >>> reflected the added parameter and an added variable, `topicConfig` to
> >> store
> >>> the map of configs.
> >>>
> >>> Best,
> >>> Leah
> >>>
> >>> On Mon, Nov 30, 2020 at 5:35 PM Matthias J. Sax 
> >> wrote:
> >>>
>  Thanks for the KIP Leah.
> 
>  Should `withLoggingEnabled()` take a `Map config`
>  similar to the one from `Materialized`?
> 
> 
>  -Matthias
> 
>  On 11/30/20 12:22 PM, Walker Carlson wrote:
> > Ah. That makes sense. Thank you for fixing that.
> >
> > One minor question. If the default is enabled is there any case
> >> where a
> > user would turn logging off then back on again? I can see having the
> > enableLoging for completeness so it's not that important probably.
> >
> > Anyways other than that it looks good!
> >
> > Walker
> >
> > On Mon, Nov 30, 2020 at 12:06 PM Leah Thomas 
>  wrote:
> >
> >> Hey Walker,
> >>
> >> Thanks for your response.
> >>
> >> 1. Ah yeah thanks for the catch, that was held over from copy/paste.
>  Both
> >> functions should take no parameters, as they just `loggingEnabled`
> >> to
>  true
> >> or false. I've removed the `WindowBytesStoreSupplier
> >>> otherStoreSupplier`
> >> from the methods in the KIP
> >> 2. I think the fix to 1 answers this question, otherwise, I'm not
> >>> quite
> >> sure what you're asking. With the updated method calls, there
> >>> shouldn't
>  be
> >> any duplication.
> >>
> >> Cheers,
> >> Leah
> >>
> >> On Mon, Nov 30, 2020 at 12:14 PM Walker Carlson <
> >>> wcarl...@confluent.io>
> >> wrote:
> >>
> >>> Hello Leah,
> >>>
> >>> Thank you for the KIP.
> >>>
> >>> I had a couple questions that maybe you can expand on from what is
> >> on
>  the
> >>> KIP.
> >>>
> >>> 1) Why are we enabling/disabling the logging by passing in a
> >>> `WindowBytesStoreSupplier`?
> >>> It seems to me that these two things should be separate.
> >>>
> >>> 2) There is already `withThisStoreSupplier(final
>  WindowBytesStoreSupplier
> >>> otherStoreSupplier)` and `withOtherStoreSupplier(final
> >>> WindowBytesStoreSupplier otherStoreSupplier)`. Why do we need to
> >> duplicate
> >>> them when the `retentionPeriod` can be set through them?
> >>>
> >>> Thanks,
> >>> Walker
> >>>
> >>> On Mon, Nov 30, 2020 at 8:53 AM Leah Thomas 
> >> wrote:
> >>>
>  After reading through
>  https://issues.apache.org/jira/browse/KAFKA-9921
> >> I
>  removed the option to enable/disable caching for `StreamJoined`,
> >> as
> >>> caching
>  will always be disabled because we retain duplicates.
> 
>  I updated the KIP accordingly, it now adds only `enableLogging`
> >> as a
>  config.
> 
>  On Mon, Nov 30, 2020 at 9:54 AM Leah Thomas  >>>
> >>> wrote:
> 
> > Hi all,
> >
> > I'd like to kick-off the discussion for KIP-689: Extend
> >> `StreamJoined`
> >>> to
> > allow more store configs. This builds off the work of KIP-479
> > <
> 
> >>>
> >>
> 
> >>>
> >>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join
> 
>  to
> > add options to enable/disable both logging and caching for stream
> >> join
> > stores.
> >
> > KIP is here:
> >
> 
> >>>
> >>
> 
> >>>
> >>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-689%3A+Extend+%60StreamJoined%60+to+allow+more+store+configs
> >
> >
> > Looking forward to hearing your thoughts,
> > Leah
> >
> 
> >>>
> >>
> >
> 
> >>>
> >>
> >
>

Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Bruno Cadonna]

Thanks for the KIP!

LGTM, as well!

Best,
Bruno

On 02.12.20 00:41, Walker Carlson wrote:

Thanks for making these changes. It makes more sense now to me. Overall LGTM

walker

On Tue, Dec 1, 2020 at 3:39 PM Sophie Blee-Goldman 
wrote:


Thanks for the KIP! I'm happy with the state of things after your latest
update,
LGTM

Sophie

On Tue, Dec 1, 2020 at 2:26 PM Leah Thomas  wrote:


Hi Matthias,

Yeah I think it should, good catch. That should also answer Walker's
question about why we have an option for `withLoggingEnabled()` even

though

that's the default. Passing in a new map of configs could allow the user

to

configure the log differently than the default. I've updated the KIP to
reflected the added parameter and an added variable, `topicConfig` to

store

the map of configs.

Best,
Leah

On Mon, Nov 30, 2020 at 5:35 PM Matthias J. Sax 

wrote:



Thanks for the KIP Leah.

Should `withLoggingEnabled()` take a `Map config`
similar to the one from `Materialized`?


-Matthias

On 11/30/20 12:22 PM, Walker Carlson wrote:

Ah. That makes sense. Thank you for fixing that.

One minor question. If the default is enabled is there any case

where a

user would turn logging off then back on again? I can see having the
enableLoging for completeness so it's not that important probably.

Anyways other than that it looks good!

Walker

On Mon, Nov 30, 2020 at 12:06 PM Leah Thomas 

wrote:



Hey Walker,

Thanks for your response.

1. Ah yeah thanks for the catch, that was held over from copy/paste.

Both

functions should take no parameters, as they just `loggingEnabled`

to

true

or false. I've removed the `WindowBytesStoreSupplier

otherStoreSupplier`

from the methods in the KIP
2. I think the fix to 1 answers this question, otherwise, I'm not

quite

sure what you're asking. With the updated method calls, there

shouldn't

be

any duplication.

Cheers,
Leah

On Mon, Nov 30, 2020 at 12:14 PM Walker Carlson <

wcarl...@confluent.io>

wrote:


Hello Leah,

Thank you for the KIP.

I had a couple questions that maybe you can expand on from what is

on

the

KIP.

1) Why are we enabling/disabling the logging by passing in a
`WindowBytesStoreSupplier`?
It seems to me that these two things should be separate.

2) There is already `withThisStoreSupplier(final

WindowBytesStoreSupplier

otherStoreSupplier)` and `withOtherStoreSupplier(final
WindowBytesStoreSupplier otherStoreSupplier)`. Why do we need to

duplicate

them when the `retentionPeriod` can be set through them?

Thanks,
Walker

On Mon, Nov 30, 2020 at 8:53 AM Leah Thomas 

wrote:



After reading through

https://issues.apache.org/jira/browse/KAFKA-9921

I

removed the option to enable/disable caching for `StreamJoined`,

as

caching

will always be disabled because we retain duplicates.

I updated the KIP accordingly, it now adds only `enableLogging`

as a

config.

On Mon, Nov 30, 2020 at 9:54 AM Leah Thomas 


wrote:



Hi all,

I'd like to kick-off the discussion for KIP-689: Extend

`StreamJoined`

to

allow more store configs. This builds off the work of KIP-479
<











https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join


to

add options to enable/disable both logging and caching for stream

join

stores.

KIP is here:












https://cwiki.apache.org/confluence/display/KAFKA/KIP-689%3A+Extend+%60StreamJoined%60+to+allow+more+store+configs



Looking forward to hearing your thoughts,
Leah

Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Walker Carlson]

Thanks for making these changes. It makes more sense now to me. Overall LGTM

walker

On Tue, Dec 1, 2020 at 3:39 PM Sophie Blee-Goldman 
wrote:

> Thanks for the KIP! I'm happy with the state of things after your latest
> update,
> LGTM
>
> Sophie
>
> On Tue, Dec 1, 2020 at 2:26 PM Leah Thomas  wrote:
>
> > Hi Matthias,
> >
> > Yeah I think it should, good catch. That should also answer Walker's
> > question about why we have an option for `withLoggingEnabled()` even
> though
> > that's the default. Passing in a new map of configs could allow the user
> to
> > configure the log differently than the default. I've updated the KIP to
> > reflected the added parameter and an added variable, `topicConfig` to
> store
> > the map of configs.
> >
> > Best,
> > Leah
> >
> > On Mon, Nov 30, 2020 at 5:35 PM Matthias J. Sax 
> wrote:
> >
> > > Thanks for the KIP Leah.
> > >
> > > Should `withLoggingEnabled()` take a `Map config`
> > > similar to the one from `Materialized`?
> > >
> > >
> > > -Matthias
> > >
> > > On 11/30/20 12:22 PM, Walker Carlson wrote:
> > > > Ah. That makes sense. Thank you for fixing that.
> > > >
> > > > One minor question. If the default is enabled is there any case
> where a
> > > > user would turn logging off then back on again? I can see having the
> > > > enableLoging for completeness so it's not that important probably.
> > > >
> > > > Anyways other than that it looks good!
> > > >
> > > > Walker
> > > >
> > > > On Mon, Nov 30, 2020 at 12:06 PM Leah Thomas 
> > > wrote:
> > > >
> > > >> Hey Walker,
> > > >>
> > > >> Thanks for your response.
> > > >>
> > > >> 1. Ah yeah thanks for the catch, that was held over from copy/paste.
> > > Both
> > > >> functions should take no parameters, as they just `loggingEnabled`
> to
> > > true
> > > >> or false. I've removed the `WindowBytesStoreSupplier
> > otherStoreSupplier`
> > > >> from the methods in the KIP
> > > >> 2. I think the fix to 1 answers this question, otherwise, I'm not
> > quite
> > > >> sure what you're asking. With the updated method calls, there
> > shouldn't
> > > be
> > > >> any duplication.
> > > >>
> > > >> Cheers,
> > > >> Leah
> > > >>
> > > >> On Mon, Nov 30, 2020 at 12:14 PM Walker Carlson <
> > wcarl...@confluent.io>
> > > >> wrote:
> > > >>
> > > >>> Hello Leah,
> > > >>>
> > > >>> Thank you for the KIP.
> > > >>>
> > > >>> I had a couple questions that maybe you can expand on from what is
> on
> > > the
> > > >>> KIP.
> > > >>>
> > > >>> 1) Why are we enabling/disabling the logging by passing in a
> > > >>> `WindowBytesStoreSupplier`?
> > > >>> It seems to me that these two things should be separate.
> > > >>>
> > > >>> 2) There is already `withThisStoreSupplier(final
> > > WindowBytesStoreSupplier
> > > >>> otherStoreSupplier)` and `withOtherStoreSupplier(final
> > > >>> WindowBytesStoreSupplier otherStoreSupplier)`. Why do we need to
> > > >> duplicate
> > > >>> them when the `retentionPeriod` can be set through them?
> > > >>>
> > > >>> Thanks,
> > > >>> Walker
> > > >>>
> > > >>> On Mon, Nov 30, 2020 at 8:53 AM Leah Thomas 
> > > >> wrote:
> > > >>>
> > >  After reading through
> > > https://issues.apache.org/jira/browse/KAFKA-9921
> > > >> I
> > >  removed the option to enable/disable caching for `StreamJoined`,
> as
> > > >>> caching
> > >  will always be disabled because we retain duplicates.
> > > 
> > >  I updated the KIP accordingly, it now adds only `enableLogging`
> as a
> > >  config.
> > > 
> > >  On Mon, Nov 30, 2020 at 9:54 AM Leah Thomas  >
> > > >>> wrote:
> > > 
> > > > Hi all,
> > > >
> > > > I'd like to kick-off the discussion for KIP-689: Extend
> > > >> `StreamJoined`
> > > >>> to
> > > > allow more store configs. This builds off the work of KIP-479
> > > > <
> > > 
> > > >>>
> > > >>
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join
> > > 
> > >  to
> > > > add options to enable/disable both logging and caching for stream
> > > >> join
> > > > stores.
> > > >
> > > > KIP is here:
> > > >
> > > 
> > > >>>
> > > >>
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-689%3A+Extend+%60StreamJoined%60+to+allow+more+store+configs
> > > >
> > > >
> > > > Looking forward to hearing your thoughts,
> > > > Leah
> > > >
> > > 
> > > >>>
> > > >>
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Sophie Blee-Goldman]

Thanks for the KIP! I'm happy with the state of things after your latest
update,
LGTM

Sophie

On Tue, Dec 1, 2020 at 2:26 PM Leah Thomas  wrote:

> Hi Matthias,
>
> Yeah I think it should, good catch. That should also answer Walker's
> question about why we have an option for `withLoggingEnabled()` even though
> that's the default. Passing in a new map of configs could allow the user to
> configure the log differently than the default. I've updated the KIP to
> reflected the added parameter and an added variable, `topicConfig` to store
> the map of configs.
>
> Best,
> Leah
>
> On Mon, Nov 30, 2020 at 5:35 PM Matthias J. Sax  wrote:
>
> > Thanks for the KIP Leah.
> >
> > Should `withLoggingEnabled()` take a `Map config`
> > similar to the one from `Materialized`?
> >
> >
> > -Matthias
> >
> > On 11/30/20 12:22 PM, Walker Carlson wrote:
> > > Ah. That makes sense. Thank you for fixing that.
> > >
> > > One minor question. If the default is enabled is there any case where a
> > > user would turn logging off then back on again? I can see having the
> > > enableLoging for completeness so it's not that important probably.
> > >
> > > Anyways other than that it looks good!
> > >
> > > Walker
> > >
> > > On Mon, Nov 30, 2020 at 12:06 PM Leah Thomas 
> > wrote:
> > >
> > >> Hey Walker,
> > >>
> > >> Thanks for your response.
> > >>
> > >> 1. Ah yeah thanks for the catch, that was held over from copy/paste.
> > Both
> > >> functions should take no parameters, as they just `loggingEnabled` to
> > true
> > >> or false. I've removed the `WindowBytesStoreSupplier
> otherStoreSupplier`
> > >> from the methods in the KIP
> > >> 2. I think the fix to 1 answers this question, otherwise, I'm not
> quite
> > >> sure what you're asking. With the updated method calls, there
> shouldn't
> > be
> > >> any duplication.
> > >>
> > >> Cheers,
> > >> Leah
> > >>
> > >> On Mon, Nov 30, 2020 at 12:14 PM Walker Carlson <
> wcarl...@confluent.io>
> > >> wrote:
> > >>
> > >>> Hello Leah,
> > >>>
> > >>> Thank you for the KIP.
> > >>>
> > >>> I had a couple questions that maybe you can expand on from what is on
> > the
> > >>> KIP.
> > >>>
> > >>> 1) Why are we enabling/disabling the logging by passing in a
> > >>> `WindowBytesStoreSupplier`?
> > >>> It seems to me that these two things should be separate.
> > >>>
> > >>> 2) There is already `withThisStoreSupplier(final
> > WindowBytesStoreSupplier
> > >>> otherStoreSupplier)` and `withOtherStoreSupplier(final
> > >>> WindowBytesStoreSupplier otherStoreSupplier)`. Why do we need to
> > >> duplicate
> > >>> them when the `retentionPeriod` can be set through them?
> > >>>
> > >>> Thanks,
> > >>> Walker
> > >>>
> > >>> On Mon, Nov 30, 2020 at 8:53 AM Leah Thomas 
> > >> wrote:
> > >>>
> >  After reading through
> > https://issues.apache.org/jira/browse/KAFKA-9921
> > >> I
> >  removed the option to enable/disable caching for `StreamJoined`, as
> > >>> caching
> >  will always be disabled because we retain duplicates.
> > 
> >  I updated the KIP accordingly, it now adds only `enableLogging` as a
> >  config.
> > 
> >  On Mon, Nov 30, 2020 at 9:54 AM Leah Thomas 
> > >>> wrote:
> > 
> > > Hi all,
> > >
> > > I'd like to kick-off the discussion for KIP-689: Extend
> > >> `StreamJoined`
> > >>> to
> > > allow more store configs. This builds off the work of KIP-479
> > > <
> > 
> > >>>
> > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join
> > 
> >  to
> > > add options to enable/disable both logging and caching for stream
> > >> join
> > > stores.
> > >
> > > KIP is here:
> > >
> > 
> > >>>
> > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-689%3A+Extend+%60StreamJoined%60+to+allow+more+store+configs
> > >
> > >
> > > Looking forward to hearing your thoughts,
> > > Leah
> > >
> > 
> > >>>
> > >>
> > >
> >
>

Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Leah Thomas]

Hi Matthias,

Yeah I think it should, good catch. That should also answer Walker's
question about why we have an option for `withLoggingEnabled()` even though
that's the default. Passing in a new map of configs could allow the user to
configure the log differently than the default. I've updated the KIP to
reflected the added parameter and an added variable, `topicConfig` to store
the map of configs.

Best,
Leah

On Mon, Nov 30, 2020 at 5:35 PM Matthias J. Sax  wrote:

> Thanks for the KIP Leah.
>
> Should `withLoggingEnabled()` take a `Map config`
> similar to the one from `Materialized`?
>
>
> -Matthias
>
> On 11/30/20 12:22 PM, Walker Carlson wrote:
> > Ah. That makes sense. Thank you for fixing that.
> >
> > One minor question. If the default is enabled is there any case where a
> > user would turn logging off then back on again? I can see having the
> > enableLoging for completeness so it's not that important probably.
> >
> > Anyways other than that it looks good!
> >
> > Walker
> >
> > On Mon, Nov 30, 2020 at 12:06 PM Leah Thomas 
> wrote:
> >
> >> Hey Walker,
> >>
> >> Thanks for your response.
> >>
> >> 1. Ah yeah thanks for the catch, that was held over from copy/paste.
> Both
> >> functions should take no parameters, as they just `loggingEnabled` to
> true
> >> or false. I've removed the `WindowBytesStoreSupplier otherStoreSupplier`
> >> from the methods in the KIP
> >> 2. I think the fix to 1 answers this question, otherwise, I'm not quite
> >> sure what you're asking. With the updated method calls, there shouldn't
> be
> >> any duplication.
> >>
> >> Cheers,
> >> Leah
> >>
> >> On Mon, Nov 30, 2020 at 12:14 PM Walker Carlson 
> >> wrote:
> >>
> >>> Hello Leah,
> >>>
> >>> Thank you for the KIP.
> >>>
> >>> I had a couple questions that maybe you can expand on from what is on
> the
> >>> KIP.
> >>>
> >>> 1) Why are we enabling/disabling the logging by passing in a
> >>> `WindowBytesStoreSupplier`?
> >>> It seems to me that these two things should be separate.
> >>>
> >>> 2) There is already `withThisStoreSupplier(final
> WindowBytesStoreSupplier
> >>> otherStoreSupplier)` and `withOtherStoreSupplier(final
> >>> WindowBytesStoreSupplier otherStoreSupplier)`. Why do we need to
> >> duplicate
> >>> them when the `retentionPeriod` can be set through them?
> >>>
> >>> Thanks,
> >>> Walker
> >>>
> >>> On Mon, Nov 30, 2020 at 8:53 AM Leah Thomas 
> >> wrote:
> >>>
>  After reading through
> https://issues.apache.org/jira/browse/KAFKA-9921
> >> I
>  removed the option to enable/disable caching for `StreamJoined`, as
> >>> caching
>  will always be disabled because we retain duplicates.
> 
>  I updated the KIP accordingly, it now adds only `enableLogging` as a
>  config.
> 
>  On Mon, Nov 30, 2020 at 9:54 AM Leah Thomas 
> >>> wrote:
> 
> > Hi all,
> >
> > I'd like to kick-off the discussion for KIP-689: Extend
> >> `StreamJoined`
> >>> to
> > allow more store configs. This builds off the work of KIP-479
> > <
> 
> >>>
> >>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join
> 
>  to
> > add options to enable/disable both logging and caching for stream
> >> join
> > stores.
> >
> > KIP is here:
> >
> 
> >>>
> >>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-689%3A+Extend+%60StreamJoined%60+to+allow+more+store+configs
> >
> >
> > Looking forward to hearing your thoughts,
> > Leah
> >
> 
> >>>
> >>
> >
>

Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Matthias J. Sax]

Thanks for the KIP Leah.

Should `withLoggingEnabled()` take a `Map config`
similar to the one from `Materialized`?


-Matthias

On 11/30/20 12:22 PM, Walker Carlson wrote:
> Ah. That makes sense. Thank you for fixing that.
> 
> One minor question. If the default is enabled is there any case where a
> user would turn logging off then back on again? I can see having the
> enableLoging for completeness so it's not that important probably.
> 
> Anyways other than that it looks good!
> 
> Walker
> 
> On Mon, Nov 30, 2020 at 12:06 PM Leah Thomas  wrote:
> 
>> Hey Walker,
>>
>> Thanks for your response.
>>
>> 1. Ah yeah thanks for the catch, that was held over from copy/paste. Both
>> functions should take no parameters, as they just `loggingEnabled` to true
>> or false. I've removed the `WindowBytesStoreSupplier otherStoreSupplier`
>> from the methods in the KIP
>> 2. I think the fix to 1 answers this question, otherwise, I'm not quite
>> sure what you're asking. With the updated method calls, there shouldn't be
>> any duplication.
>>
>> Cheers,
>> Leah
>>
>> On Mon, Nov 30, 2020 at 12:14 PM Walker Carlson 
>> wrote:
>>
>>> Hello Leah,
>>>
>>> Thank you for the KIP.
>>>
>>> I had a couple questions that maybe you can expand on from what is on the
>>> KIP.
>>>
>>> 1) Why are we enabling/disabling the logging by passing in a
>>> `WindowBytesStoreSupplier`?
>>> It seems to me that these two things should be separate.
>>>
>>> 2) There is already `withThisStoreSupplier(final WindowBytesStoreSupplier
>>> otherStoreSupplier)` and `withOtherStoreSupplier(final
>>> WindowBytesStoreSupplier otherStoreSupplier)`. Why do we need to
>> duplicate
>>> them when the `retentionPeriod` can be set through them?
>>>
>>> Thanks,
>>> Walker
>>>
>>> On Mon, Nov 30, 2020 at 8:53 AM Leah Thomas 
>> wrote:
>>>
 After reading through https://issues.apache.org/jira/browse/KAFKA-9921
>> I
 removed the option to enable/disable caching for `StreamJoined`, as
>>> caching
 will always be disabled because we retain duplicates.

 I updated the KIP accordingly, it now adds only `enableLogging` as a
 config.

 On Mon, Nov 30, 2020 at 9:54 AM Leah Thomas 
>>> wrote:

> Hi all,
>
> I'd like to kick-off the discussion for KIP-689: Extend
>> `StreamJoined`
>>> to
> allow more store configs. This builds off the work of KIP-479
> <

>>>
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join

 to
> add options to enable/disable both logging and caching for stream
>> join
> stores.
>
> KIP is here:
>

>>>
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-689%3A+Extend+%60StreamJoined%60+to+allow+more+store+configs
>
>
> Looking forward to hearing your thoughts,
> Leah
>

>>>
>>
> 

Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Walker Carlson]

Ah. That makes sense. Thank you for fixing that.

One minor question. If the default is enabled is there any case where a
user would turn logging off then back on again? I can see having the
enableLoging for completeness so it's not that important probably.

Anyways other than that it looks good!

Walker

On Mon, Nov 30, 2020 at 12:06 PM Leah Thomas  wrote:

> Hey Walker,
>
> Thanks for your response.
>
> 1. Ah yeah thanks for the catch, that was held over from copy/paste. Both
> functions should take no parameters, as they just `loggingEnabled` to true
> or false. I've removed the `WindowBytesStoreSupplier otherStoreSupplier`
> from the methods in the KIP
> 2. I think the fix to 1 answers this question, otherwise, I'm not quite
> sure what you're asking. With the updated method calls, there shouldn't be
> any duplication.
>
> Cheers,
> Leah
>
> On Mon, Nov 30, 2020 at 12:14 PM Walker Carlson 
> wrote:
>
> > Hello Leah,
> >
> > Thank you for the KIP.
> >
> > I had a couple questions that maybe you can expand on from what is on the
> > KIP.
> >
> > 1) Why are we enabling/disabling the logging by passing in a
> > `WindowBytesStoreSupplier`?
> > It seems to me that these two things should be separate.
> >
> > 2) There is already `withThisStoreSupplier(final WindowBytesStoreSupplier
> > otherStoreSupplier)` and `withOtherStoreSupplier(final
> > WindowBytesStoreSupplier otherStoreSupplier)`. Why do we need to
> duplicate
> > them when the `retentionPeriod` can be set through them?
> >
> > Thanks,
> > Walker
> >
> > On Mon, Nov 30, 2020 at 8:53 AM Leah Thomas 
> wrote:
> >
> > > After reading through https://issues.apache.org/jira/browse/KAFKA-9921
> I
> > > removed the option to enable/disable caching for `StreamJoined`, as
> > caching
> > > will always be disabled because we retain duplicates.
> > >
> > > I updated the KIP accordingly, it now adds only `enableLogging` as a
> > > config.
> > >
> > > On Mon, Nov 30, 2020 at 9:54 AM Leah Thomas 
> > wrote:
> > >
> > > > Hi all,
> > > >
> > > > I'd like to kick-off the discussion for KIP-689: Extend
> `StreamJoined`
> > to
> > > > allow more store configs. This builds off the work of KIP-479
> > > > <
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join
> > >
> > > to
> > > > add options to enable/disable both logging and caching for stream
> join
> > > > stores.
> > > >
> > > > KIP is here:
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-689%3A+Extend+%60StreamJoined%60+to+allow+more+store+configs
> > > >
> > > >
> > > > Looking forward to hearing your thoughts,
> > > > Leah
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Leah Thomas]

Hey Walker,

Thanks for your response.

1. Ah yeah thanks for the catch, that was held over from copy/paste. Both
functions should take no parameters, as they just `loggingEnabled` to true
or false. I've removed the `WindowBytesStoreSupplier otherStoreSupplier`
from the methods in the KIP
2. I think the fix to 1 answers this question, otherwise, I'm not quite
sure what you're asking. With the updated method calls, there shouldn't be
any duplication.

Cheers,
Leah

On Mon, Nov 30, 2020 at 12:14 PM Walker Carlson 
wrote:

> Hello Leah,
>
> Thank you for the KIP.
>
> I had a couple questions that maybe you can expand on from what is on the
> KIP.
>
> 1) Why are we enabling/disabling the logging by passing in a
> `WindowBytesStoreSupplier`?
> It seems to me that these two things should be separate.
>
> 2) There is already `withThisStoreSupplier(final WindowBytesStoreSupplier
> otherStoreSupplier)` and `withOtherStoreSupplier(final
> WindowBytesStoreSupplier otherStoreSupplier)`. Why do we need to duplicate
> them when the `retentionPeriod` can be set through them?
>
> Thanks,
> Walker
>
> On Mon, Nov 30, 2020 at 8:53 AM Leah Thomas  wrote:
>
> > After reading through https://issues.apache.org/jira/browse/KAFKA-9921 I
> > removed the option to enable/disable caching for `StreamJoined`, as
> caching
> > will always be disabled because we retain duplicates.
> >
> > I updated the KIP accordingly, it now adds only `enableLogging` as a
> > config.
> >
> > On Mon, Nov 30, 2020 at 9:54 AM Leah Thomas 
> wrote:
> >
> > > Hi all,
> > >
> > > I'd like to kick-off the discussion for KIP-689: Extend `StreamJoined`
> to
> > > allow more store configs. This builds off the work of KIP-479
> > > <
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join
> >
> > to
> > > add options to enable/disable both logging and caching for stream join
> > > stores.
> > >
> > > KIP is here:
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-689%3A+Extend+%60StreamJoined%60+to+allow+more+store+configs
> > >
> > >
> > > Looking forward to hearing your thoughts,
> > > Leah
> > >
> >
>

Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Walker Carlson]

Hello Leah,

Thank you for the KIP.

I had a couple questions that maybe you can expand on from what is on the
KIP.

1) Why are we enabling/disabling the logging by passing in a
`WindowBytesStoreSupplier`?
It seems to me that these two things should be separate.

2) There is already `withThisStoreSupplier(final WindowBytesStoreSupplier
otherStoreSupplier)` and `withOtherStoreSupplier(final
WindowBytesStoreSupplier otherStoreSupplier)`. Why do we need to duplicate
them when the `retentionPeriod` can be set through them?

Thanks,
Walker

On Mon, Nov 30, 2020 at 8:53 AM Leah Thomas  wrote:

> After reading through https://issues.apache.org/jira/browse/KAFKA-9921 I
> removed the option to enable/disable caching for `StreamJoined`, as caching
> will always be disabled because we retain duplicates.
>
> I updated the KIP accordingly, it now adds only `enableLogging` as a
> config.
>
> On Mon, Nov 30, 2020 at 9:54 AM Leah Thomas  wrote:
>
> > Hi all,
> >
> > I'd like to kick-off the discussion for KIP-689: Extend `StreamJoined` to
> > allow more store configs. This builds off the work of KIP-479
> > <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join>
> to
> > add options to enable/disable both logging and caching for stream join
> > stores.
> >
> > KIP is here:
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-689%3A+Extend+%60StreamJoined%60+to+allow+more+store+configs
> >
> >
> > Looking forward to hearing your thoughts,
> > Leah
> >
>

Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Leah Thomas]

After reading through https://issues.apache.org/jira/browse/KAFKA-9921 I
removed the option to enable/disable caching for `StreamJoined`, as caching
will always be disabled because we retain duplicates.

I updated the KIP accordingly, it now adds only `enableLogging` as a config.

On Mon, Nov 30, 2020 at 9:54 AM Leah Thomas  wrote:

> Hi all,
>
> I'd like to kick-off the discussion for KIP-689: Extend `StreamJoined` to
> allow more store configs. This builds off the work of KIP-479
> 
>  to
> add options to enable/disable both logging and caching for stream join
> stores.
>
> KIP is here:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-689%3A+Extend+%60StreamJoined%60+to+allow+more+store+configs
>
>
> Looking forward to hearing your thoughts,
> Leah
>

[DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs

[Leah Thomas]

Hi all,

I'd like to kick-off the discussion for KIP-689: Extend `StreamJoined` to
allow more store configs. This builds off the work of KIP-479

to
add options to enable/disable both logging and caching for stream join
stores.

KIP is here:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-689%3A+Extend+%60StreamJoined%60+to+allow+more+store+configs


Looking forward to hearing your thoughts,
Leah