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