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