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