kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthias J. Sax" <mj...@apache.org>
Subject Re: [DISCUSS] KIP-689: Extend `StreamJoined` to allow more store configs
Date Wed, 02 Dec 2020 23:39:49 GMT
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 <bruno@confluent.io> 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 <sophie@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 <lthomas@confluent.io>
>> 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 <mjsax@apache.org>
>>>> wrote:
>>>>>
>>>>>> Thanks for the KIP Leah.
>>>>>>
>>>>>> Should `withLoggingEnabled()` take a `Map<String, String> 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 <lthomas@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 <
>>>>> wcarlson@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 <lthomas@confluent.io>
>>>>>>>> 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 <lthomas@confluent.io
>>>>>
>>>>>>>>> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 

Mime
View raw message