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