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