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
