Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2025-03-14 Thread TengYao Chi
Hi Emanuele Thanks for raising the question. You're absolutely right. The KIP defines the behavior of membership on close, so the other consumer API should not be affected by this one. Best, TengYao Emanuele Sabellico 於 2025年3月15日 週六 上午1:06寫道: > Hello TengYao, I have a question about this KIP

RE: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2025-03-14 Thread Emanuele Sabellico
Hello TengYao, I have a question about this KIP. Given it's not mentioned the *unsubscribe* behaviour should be the current default one in the Java client, that is to send a *LeaveGroup *or *ConsumerGroupHeartbeat(-1)* if the member is dynamic and *no LeaveGroup* or *ConsumerGroupHeartbeat(-2)* if

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2025-01-08 Thread TengYao Chi
Hi Sophie, I think you've mentioned a very important distinction between Kafka Streams and Plain Consumer behavior on closing. While KafkaStreams has a ensures state cleanup through its internal mechanism (not sure this statement is accurate or not), Plain Consumer relies on `onPartitionRevoke` ca

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2025-01-08 Thread Sophie Blee-Goldman
tl;dr: 1) I tend to agree we should keep the existing behavior, but what this means is actually different and more complicated than just "if leaves group, then invokes leave callback" 2) Personally I think we should actually *always* invoke this callback, for every case Longer version: First, to

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-12-16 Thread TengYao Chi
Hi everyone, I'd like to discuss the leave callback implementation. While this KIP focuses on sending leave requests, we should also consider how to handle the leave callback. IMHO, we should maintain the original logic: if a consumer remains in the group, the callback should not be invoked. What

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-10-02 Thread TengYao Chi
Hello everyone, It seems we have reached a consensus on how this KIP will proceed. I have updated the content accordingly. Please take a look, and if there are no further questions, I will initiate a vote. Best regards, TengYao Chia-Ping Tsai 於 2024年10月2日 週三 上午7:06寫道: > hi Matthias > > Thanks

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-10-01 Thread Chia-Ping Tsai
hi Matthias Thanks for sharing your POV, and you do remind me that this KIP should not be blocked due to this verbose discussion. Those protected methods are not Public APIs, so they can reverted without deprecation cycle. Those variables can be exposed publicly when users do need them. +1 t

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-10-01 Thread Matthias J. Sax
Chi Sent: 01 October 2024 05:23 To: dev@kafka.apache.org Subject: Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not Hello everyone, I'm also +1 on using the fluent API and having the `with` prefix in setter method names. Regarding Matthias' point, I

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-10-01 Thread TengYao Chi
24年10月1日 週二 下午2:41寫道: > +1 from me on the fluent API using the `with` prefix too. > > Thanks, > Andrew > > > From: TengYao Chi > Sent: 01 October 2024 05:23 > To: dev@kafka.apache.org > Subject: Re: [DISCUSS] KIP-1092: Extend Con

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-10-01 Thread Chia-Ping Tsai
gt; Best, > TengYao > > Andrew Schofield 於 2024年10月1日 週二 > 下午2:41寫道: > > > +1 from me on the fluent API using the `with` prefix too. > > > > Thanks, > > Andrew > > > > ____________________ > > From: TengYao Chi >

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-30 Thread Andrew Schofield
+1 from me on the fluent API using the `with` prefix too. Thanks, Andrew From: TengYao Chi Sent: 01 October 2024 05:23 To: dev@kafka.apache.org Subject: Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not Hello

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-30 Thread TengYao Chi
Hello everyone, I'm also +1 on using the fluent API and having the `with` prefix in setter method names. Regarding Matthias' point, I agree with Sophie that we should keep the `CloseOptions` classes separate. These two `CloseOptions` serve different purposes, and while they may occasionally share

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-30 Thread Matthias J. Sax
Sophie, yes, that a fair summary, and yes, it was only an alternative idea for the case that people think, allowing to disable leave-group request for the plain consumer is not desirable. Seems we are actually on the same page. (And yes, it was meant for this thread, not KIP-1094...) On 9/3

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-30 Thread Matthias J. Sax
Kirk, I think good API design principle is to expose the minimum require API to users, and users don't need getters, that's why we don't have any getters in the KS config object classes. Getters are only needed internally. From an impl POV, the internal member can be either (1) package-privat

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-30 Thread Sophie Blee-Goldman
+1 to using the fluent API and including "with" in the setter names. I also think Matthias raised a good point, that maybe it would be a good time to "fix" this issue in the CloseOptions for the KafkaStreams#close method, to conform to this API format. As for his other "random idea" about combinin

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-30 Thread Kirk True
Hi TengYao, Thanks for all the updates. I took another look at the KIP and had some more questions: KT4: Can we change the timeout() method to timeoutMs() to be more consistent? KT5: Can we add the behavior when a null CloseOptions is provided? KT6: Can we use "direct" values in CloseOptions vs.

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-30 Thread Kirk True
Hi all, On Sun, Sep 29, 2024, at 12:08 PM, Chia-Ping Tsai wrote: > > From an API POV, I think the new `CloseOptions` class should not have > any "getters" and thus, it's irrelevant how we represent the different > cases in code internally (even if I believe using `Optional` might be a > good way t

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-30 Thread Chia-Ping Tsai
> Overall, the "static method builder" pattern seems better to me, and thus I would prefer to make it the "gold standard" and we can see what we can do for `Admin API` mid/long term? Since we want to avoid complicated compatibility issues, adding a static method builder to Admin options seems mor

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-30 Thread Matthias J. Sax
I am also in favor of consistent APIs. That's very good point. I did not take `Admin` API into account, and I am not aware that consumer / producer would have config object classes? Seems we are in a tricky situation here, because "consistent API" to me means producer, consumer, admin and KS.

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-29 Thread TengYao Chi
Hi Matthias, Personally, I also like the static methods for their fluency and conciseness. However, given that many existing classes (e.g., `CreateDelegationTokenOptions`) already provide both getters and setters, I tend to align the `CloseOptions` class with those older classes to maintain consis

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-29 Thread Chia-Ping Tsai
> From an API POV, I think the new `CloseOptions` class should not have any "getters" and thus, it's irrelevant how we represent the different cases in code internally (even if I believe using `Optional` might be a good way to handle it). If we choose to avoid using getters, consumers would have t

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-29 Thread Matthias J. Sax
I am not sure, but I get the impression that we are starting to talk too much about implementation details now? From an API POV, I think the new `CloseOptions` class should not have any "getters" and thus, it's irrelevant how we represent the different cases in code internally (even if I belie

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-29 Thread TengYao Chi
Hi Chia-Ping, Thanks for your feedback. What I intended to express is that if `Optional.empty()` is passed to the `timeout`, it will eventually be converted to `DEFAULT_CLOSE_TIMEOUT_MS`, just as you mentioned. Apologies for not expressing that clearly and for any confusion caused. Best regards,

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-29 Thread Chia-Ping Tsai
hi TengYao > I have reviewed the `close()` method implementation for both the Classic and Async Consumers. I believe the `timeout` parameter could have a default value, and this default should align with the existing `Consumer#close()` method, which internally calls the overloaded `Consumer#close(

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-28 Thread TengYao Chi
Hi Sophie, Thanks for the suggestions. I have reviewed the `close()` method implementation for both the Classic and Async Consumers. I believe the `timeout` parameter could have a default value, and this default should align with the existing `Consumer#close()` method, which internally calls the

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-28 Thread Chia-Ping Tsai
hi Sophie Fewer overloads are preferable, so in my opinion, the consumer should only have close() and close(CloseOptions), with the other overloads deprecated. That means all options in CloseOptions should be optional, and we should use a fluent-style API to add setters for them. This would allow

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-28 Thread Sophie Blee-Goldman
@Matthias, regarding your questions in 101, is it fair to summarize your points here as (A) only Kafka Streams, but not plain consumer, would need to avoid leaving the group on close (for non-static members), and (B) with KIP-1088 we will soon have a Streams-specific Consumer API that would be more

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-28 Thread Chia-Ping Tsai
hi TengYao Could you please consider adding a "default" behavior for leaveGroup? As I previously mentioned, leaveGroup=true is not ideal as the default for static members, and similarly, leaveGroup=false is not suitable as the default for dynamic members. Maybe we could change the type of leaveGr

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-28 Thread TengYao Chi
Hi Sophie Thanks for feedback. I have updated the Public Interface part accordingly. Please take a look. Best, TengYao TengYao Chi 於 2024年9月28日 週六 下午1:26寫道: > Hi Matthias, > > Thanks for the explanation, particularly regarding the important > considerations for both the plain consumer and Kafk

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-27 Thread TengYao Chi
Hi Matthias, Thanks for the explanation, particularly regarding the important considerations for both the plain consumer and Kafka Streams use cases. In this case, I think it would be better to stick with my initial proposal. We should give plain consumers the ability to determine whether to send

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-27 Thread Chia-Ping Tsai
hi Matthias > 100: Why do we want to distinguish between the classic and the new async consumer? Should they not have the same (user facing) behavior? Or maybe I misunderstand something. Can one catch we up what epoch "-1" vs epoch "-2" means? I apologize for any confusion in my earlier explanat

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-27 Thread Matthias J. Sax
Thanks for the KIP. Two questions/comments: 100: Why do we want to distinguish between the classic and the new async consumer? Should they not have the same (user facing) behavior? Or maybe I misunderstand something. Can one catch we up what epoch "-1" vs epoch "-2" means? 101: I think we

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-26 Thread Sophie Blee-Goldman
Thanks for the KIP! Quick request for readability, can you please include the exact APIs that you're proposing to add or change under the "Public Interfaces" section? The KIP should display the actual method signature and any applicable javadocs for new public APIs. You can look at other KIPs for

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-26 Thread Chia-Ping Tsai
> I think I’m actually OK with leaving it as leaveGroup with a lot of documentation that warns users away from changing it arbitrarily. Pardon me, I just want to ensure we are all on the same page. 1. `leaveGroup=true`: `ClassicKafkaConsumer` sends a `LeaveGroupRequest` for either the dyna

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-26 Thread Kirk True
Hi all, I see that leaveGroup has been renamed as releaseStaticMember. I hate to ask, but what is the behavior when a user sets this to true for a non-static member? Should it just work, log a warning, throw an error, or? I think I’m actually OK with leaving it as leaveGroup with a lot of docu

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-26 Thread Chia-Ping Tsai
Dear all, The main purpose is to allow consumers to leave a group permanently, even if they have a static member ID. Additionally, we don't have insights into the use case where a dynamic member might not want to leave the group. Therefore, should we enhance the close option to support our goal

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-24 Thread Chia-Ping Tsai
hi Kirk > KT1: Why would a non-Kafka Streams application want to set leaveGroup=false? Because Kafka Streams manages the group membership assignment under the covers, it can re-assign partitions to a new Consumer when the old one closes. But in a non-Kafka Streams application, doesn’t this just le

Re: [DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-23 Thread Kirk True
Hi TengYao, Thanks for writing up this KIP :) Questions: KT1: Why would a non-Kafka Streams application want to set leaveGroup=false? Because Kafka Streams manages the group membership assignment under the covers, it can re-assign partitions to a new Consumer when the old one closes. But in a

[DISCUSS] KIP-1092: Extend Consumer#close with an option to leave the group or not

2024-09-22 Thread TengYao Chi
Hello everyone, I would like to start a discussion thread on KIP-1092 , which proposes extending Consumer#close with an option to leave the consumer group or not. Please take a look and let me know what you think, and I would appreciate any suggestion