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