hi Sophie I totally understand the concerns regarding internal classes, but they have been introduced to the streams module for a while, including AutoOffsetResetInternal, JoinWindowsInternal, NamedInternal, and ToInternal.
Obstructing this KIP solely due to a code taste may not be worthwhile, so I have opened https://issues.apache.org/jira/browse/KAFKA-19401 to separate the discussion. This a kind of delayed tactic, but it can help the KIP progress :) Best, Chia-Ping On 2025/06/11 23:57:36 Sophie Blee-Goldman wrote: > 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 > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>> > > >>>>>>> > > >>>>> > > >>>>> > > >>>> > > >>> > > >>> > > > > > > > >
