Thanks for looking into it. I also needed to refresh my memory. In Scala, it's sufficient to expose the `static` methods for a native Scala integration. If you look into the code, those static methods return the actually Java objects, and thus all non-static Java methods are available automatically.
As we only add new non-static methods, we don't need to do anything for the Scala API, and the new methods will be available for Scala users automatically. -Matthias On 12/3/20 7:16 AM, Leah Thomas wrote: > 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 >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
