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 > > > >>>>> > > > >>>> > > > >>> > > > >> > > > > > > > > > >
