Hi all, Thanks for all the feedback. I’ve updated the KIP and introduced a CloseOptionsInternal class that provides a getter method.
Best Regards, Jiunn-Yang > Chia-Ping Tsai <chia7...@gmail.com> 於 2025年5月8日 下午1:55 寫道： > >> One more nit: we should remove the getters. There are useless. -- In > Kafka Streams, we follow the pattern to have a (non-public) sub-class > `CloseOptionsInternal` which would add the necessary getters KS runtime > needs. > > IIRC, we had a similar discussion regarding "internal" classes previously. > To avoid bikeshedding, I'd like to emphasize the importance of code > consistency for code style. Therefore, I'm +1 on using CloseOptionsInternal > in the streams module. > > If we want to change the code style for the streams module, it would be > beneficial to have a separate KIP for that. > > Best, > Chia-Ping > > > > Matthias J. Sax <mj...@apache.org> 於 2025年5月8日 週四 下午12:02寫道： > >> Thanks for updating the KIP. >> >> One more nit: we should remove the getters. There are useless. -- In >> Kafka Streams, we follow the pattern to have a (non-public) sub-class >> `CloseOptionsInternal` which would add the necessary getters KS runtime >> needs. >> >> Cf >> >> https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/internals/AutoOffsetResetInternal.java >> that we added recently to complement the public >> >> https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/AutoOffsetReset.java >> >> >> -Matthias >> >> On 5/6/25 6:26 PM, Sophie Blee-Goldman wrote: >>> Thanks for the update! I think this is looking good now, my only feedback >>> would be to update the KIP title as well to reflect these latest changes. >>> Feel free to pick one yourself but if you want a suggestion, maybe >>> something like "Redo Kafka Streams CloseOptions in fluent style" or >>> similar. >>> >>> Let's see if Matthias has any other feedback on the proposal but >> otherwise >>> we should be ready to move to a vote >>> >>> On Mon, May 5, 2025 at 4:33 AM 黃竣陽 <s7133...@gmail.com> wrote: >>> >>>> Hi all, >>>> >>>> I've updated the KIP. If you have any comments or feedback, please feel >>>> free to share. Thank you! >>>> <https://cwiki.apache.org/confluence/x/QAq9F> >>>> >>>> Best Regards, >>>> Jiunn-Yang >>>> >>>>> Sophie Blee-Goldman <sop...@responsive.dev> 於 2025年4月26日 下午1:34 寫道： >>>>> >>>>> That's a good point about the DEFAULT enum, it would essentially be >>>>> redundant with REMAIN_IN_GROUP since this is the default behavior for >>>>> Streams regardless of whether it's dynamic or static. We only had >> DEFAULT >>>>> for the plain consumer client because the behavior differed for these >> two >>>>> cases. So +1 on the enum options being only LEAVE_GROUP and >>>> REMAIN_IN_GROUP >>>>> for Streams >>>>> >>>>> On Fri, Apr 25, 2025 at 9:24 PM Chia-Ping Tsai <chia7...@gmail.com> >>>> wrote: >>>>> >>>>>> hi all, >>>>>> >>>>>> It seems the consensus is to create CloseOptions for streams, and >>>> currently >>>>>> it is almost identical to the CloseOptions for consumers. >>>>>> >>>>>> I'm +1 to this idea, as streams may have different behavior in the >>>> future. >>>>>> Additionally, I want to raise a question that might highlight a >>>> difference >>>>>> even now :) >>>>>> >>>>>> chia_0: >>>>>> >>>>>> Does the CloseOptions for streams need a DEFAULT option if we want to >>>> use >>>>>> an enum? The use case for streams is whether to remove static members >> or >>>>>> not, and therefore it seems we can simplify the enum with only >>>> LEAVE_GROUP >>>>>> and REMAIN_IN_GROUP. >>>>>> >>>>>> Best, >>>>>> Chia-Ping >>>>>> >>>>>> >>>>>> TengYao Chi <kiting...@gmail.com> 於 2025年4月26日 週六 下午12:17寫道： >>>>>> >>>>>>> Hi Jiunn-Yang >>>>>>> >>>>>>> You could copy the code example from KIP-1092, but adjust it to fit >> the >>>>>>> Streams style (e.g. javadoc, definition). >>>>>>> It would be easier since these examples have been discussed >> previously. >>>>>>> >>>>>>> Best, >>>>>>> TengYao >>>>>>> >>>>>>> >>>>>>> 黃竣陽 <s7133...@gmail.com> 於 2025年4月26日 週六 上午11:34寫道： >>>>>>> >>>>>>>> Hello Sophie, Mathias, >>>>>>>> >>>>>>>> Thanks for your comments. >>>>>>>> >>>>>>>> I fully agree with option 2 and will proceed to update the KIP to >>>>>> reflect >>>>>>>> this choice. >>>>>>>> >>>>>>>> Best Regards, >>>>>>>> Jiunn-Yang >>>>>>>> >>>>>>>>> Sophie Blee-Goldman <sop...@responsive.dev> 於 2025年4月26日 清晨6:48 >> 寫道： >>>>>>>>> >>>>>>>>> Ah thanks Matthias, I was looking at the wrong code earlier >> whoops. I >>>>>>>>> totally agree, the #build static constructor is out of place, as I >>>>>> said >>>>>>>>> originally I believe we should follow the same pattern we used in >>>>>>>> KIP-1092 >>>>>>>>> >>>>>>>>> As for whether to literally reuse the same CloseOptions object, I'm >>>>>>>> against >>>>>>>>> that, I think Streams should have its own. Streams has different >>>>>>> default >>>>>>>>> behavior and we may want to extend the close options with >>>>>>>> streams-specific >>>>>>>>> stuff at some point in the future. >>>>>>>>> >>>>>>>>> So I'm definitely in favor of option 2. I also think that if we're >>>>>>> going >>>>>>>> to >>>>>>>>> deprecate the entire class and add a new one, then we may as well >>>>>> also >>>>>>>> use >>>>>>>>> an enum instead of a boolean for leaveGroup. >>>>>>>>> >>>>>>>>> On Fri, Apr 25, 2025 at 3:39 PM Matthias J. Sax <mj...@apache.org> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> Thanks for the KIP. >>>>>>>>>> >>>>>>>>>> Using `build()` method in not common in the current API. Why do we >>>>>>> want >>>>>>>>>> to diverge? >>>>>>>>>> >>>>>>>>>> It seems more aligned to the current API design to replace >> `build()` >>>>>>>>>> with two static builder methods: >>>>>>>>>> >>>>>>>>>> - withTimeout(Duration) >>>>>>>>>> - withLeaveGropu(boolean) >>>>>>>>>> >>>>>>>>>> (Just for to illustrate. I would not use these two method names >>>>>>> though.) >>>>>>>>>> >>>>>>>>>> The problem I see is, misalignment to the usual naming patterns >> and >>>>>>>>>> KIP-1092 >>>>>>>>>> ( >>>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>> >> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=321719077 >>>>>>>>>> ) >>>>>>>>>> >>>>>>>>>> We usually use `withXxx(...)` for the non-static method, and other >>>>>>> names >>>>>>>>>> for the static entry point method. However, the existing >>>>>>> `CloseOptions` >>>>>>>>>> already uses `timeout(During)` and `leaveGroup(boolean)` so we >>>>>> cannot >>>>>>>>>> just change it. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> I think there is two options: >>>>>>>>>> >>>>>>>>>> (1) We could just re-use the `CloseOptions` class from KIP-1092, >> and >>>>>>>>>> deprecate `KafkaStreams.close(KakfaStreams.ClosOptions)` in favor >>>>>> of a >>>>>>>>>> new one, which accepts the KIP-1092 `CloseOptions` object -- I >> want >>>>>> to >>>>>>>>>> point out, that this idea was discussed on KIP-1092 already, but >> it >>>>>>> was >>>>>>>>>> controversial. >>>>>>>>>> >>>>>>>>>> (2) If we don't want to do it, we can also deprecate the entire >>>>>>> existing >>>>>>>>>> (nested) class, `KafkaStreams.CloseOptions`, and create a new (top >>>>>>>>>> level) `o.a.k.streams.CloseOptions` and design it in a way that >>>>>> aligns >>>>>>>>>> to common naming patterns. >>>>>>>>>> >>>>>>>>>> public class CloseOptions { >>>>>>>>>> public static CloseOptions timeout(Duration); >>>>>>>>>> public static CloseOptions leaveGroup(boolean); >>>>>>>>>> >>>>>>>>>> public CloseOptions withTimeout(Duration); >>>>>>>>>> public CloseOptions withLeaveGroup(boolean); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> (Or similar names.) >>>>>>>>>> >>>>>>>>>> Btw: Instead of using a `boolean` it could also be beneficial to >> use >>>>>>> an >>>>>>>>>> enum a la KIP-1092 with valued DEFAULT, LEAVE_GROUP, >>>>>> REMAIN_IN_GROUP? >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Thoughts? >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -Matthias >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 4/25/25 3:05 PM, Sophie Blee-Goldman wrote: >>>>>>>>>>> Thanks! I personally think this looks good, as we really just >>>>>> wanted >>>>>>> to >>>>>>>>>>> remove the public constructor, but I'll ping Matthias to take a >>>>>> look >>>>>>>> and >>>>>>>>>>> make sure this is in line with his understanding >>>>>>>>>>> >>>>>>>>>>> If yes I think we can move to a vote >>>>>>>>>>> >>>>>>>>>>> On Fri, Apr 25, 2025 at 5:34 AM 黃竣陽 <s7133...@gmail.com> wrote: >>>>>>>>>>> >>>>>>>>>>>> Hello Sophie, >>>>>>>>>>>> >>>>>>>>>>>> Thanks for your comments, >>>>>>>>>>>> >>>>>>>>>>>> I’ve updated the KIP to add a new static `build()` method for >>>>>>>>>> initializing >>>>>>>>>>>> the CloseOptions object. >>>>>>>>>>>> The public constructor has been deprecated, while the existing >>>>>>>>>>>> fluent-style methods remain unchanged. >>>>>>>>>>>> >>>>>>>>>>>> Best Regards, >>>>>>>>>>>> Jiunn-Yang >>>>>>>>>>>> >>>>>>>>>>>>> Sophie Blee-Goldman <sop...@responsive.dev> 於 2025年4月25日 >> 清晨5:15 >>>>>>> 寫道： >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks for the KIP! >>>>>>>>>>>>> >>>>>>>>>>>>> This looks good but a few comments about the API: I think we >>>>>>> actually >>>>>>>>>>>> want >>>>>>>>>>>>> more of a fluent pattern than a literal builder pattern, to be >>>>>>>>>> consistent >>>>>>>>>>>>> with other APIs in Kafka Streams. You can criticize Matthias >> for >>>>>>>> saying >>>>>>>>>>>>> "builder pattern" in the ticket, he means a fluent style :P >>>>>>>>>>>>> >>>>>>>>>>>>> In other words instead of introducing a CloseOptions.Builder we >>>>>>>> should >>>>>>>>>>>> just >>>>>>>>>>>>> have a static constructor and non-static `.withParam()` methods >>>>>> for >>>>>>>> all >>>>>>>>>>>>> optional parameters. You can actually take a look at the design >>>>>> of >>>>>>>> the >>>>>>>>>>>>> CloseOptions class we just added for the consumer client, which >>>>>> we >>>>>>>>>>>> designed >>>>>>>>>>>>> specifically to match the style we wanted the Streams >>>>>> CloseOptions >>>>>>> to >>>>>>>>>>>> have. >>>>>>>>>>>>> The parameters are a bit different but the API format should be >>>>>> the >>>>>>>>>> same >>>>>>>>>>>>> >>>>>>>>>>>>> Cheers, >>>>>>>>>>>>> Sophie >>>>>>>>>>>>> >>>>>>>>>>>>> On Fri, Apr 11, 2025 at 6:43 PM 黃竣陽 <s7133...@gmail.com> >> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Hello everyone, >>>>>>>>>>>>>> >>>>>>>>>>>>>> I would like to start a discussion on KIP-1153: Kafka Streams >>>>>>>>>>>>>> `CloseOptions` should not have a public constructor >>>>>>>>>>>>>> <https://cwiki.apache.org/confluence/x/QAq9F> >>>>>>>>>>>>>> >>>>>>>>>>>>>> This proposal aims to improve KafkaStreams.CloseOptions by >>>>>>> adopting >>>>>>>> a >>>>>>>>>>>>>> builder pattern to ensure API consistency. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Best Regards, >>>>>>>>>>>>>> Jiunn-Yang >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>> >>>> >>> >> >>
