Ahh, that makes sense. Thanks for clarifying! On Thu, Dec 3, 2020 at 3:01 PM Matthias J. Sax <mj...@apache.org> wrote:
> 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 > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
