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 <br...@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 <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 <ltho...@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 <mj...@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 <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: > >>>>>> > >>>>>>> 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 > >>>> 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 <ltho...@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 > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
