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