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
