Thanks for thinking of that Matthias. After looking at what we currently have for `StreamJoined` in scala, we just have bare bones functionality, allowing users to use `StreamJoined` through the two `with()` functions and one `as()` function. This is the same for `Materialized` in scala, which does not include implementation for `withLoggingEnabled()` etc. Neither of these scala versions include `withKeySerde()` or `withValueSerde()`.
I'm not sure if we consciously made the decision to not implement additional configs for `Materialized` and `StreamJoined` in Scala, but I don't think it makes a lot of sense to add functionality for `withLoggingEnabled()` and `withLoggingDisabled()` if the same cannot be said for Materialized or the other configs we allow in the Java API. My thought is to leave it out of this KIP and, if we so choose, create a follow-up ticket to implement all the additional functionality at once for the Scala API. WDYT? On Wed, Dec 2, 2020 at 5:40 PM Matthias J. Sax <mj...@apache.org> wrote: > 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 > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
