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