Sorry I didn't see this in time but we actually ended up removing the CloseOptionsInternal in the consumer version of this API. And I personally prefer this approach, especially for such a small and simple class -- feels silly to create a new separate class just to hide two methods that don't mutate state. Just my two cents.
Anyways if we want to be consistent with the consumer version then no CloseOptionsInternal would be the way to go here On Mon, May 19, 2025 at 7:46 AM 黃竣陽 <s7133...@gmail.com> wrote: > Hi all, > > I’d like to manually bump this thread. If there’s no further feedback, > I’ll start the vote tomorrow. > > Best Regards, > Jiunn-Yang > > > 黃竣陽 <s7133...@gmail.com> 於 2025年5月8日 晚上8:35 寫道： > > > > 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 > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>> > >>>>> > >>>> > >>> > >>> > > > >
