Hi TengYao, Thanks for the KIP! I have a couple of comments.
1. In the motivation section, I would really start from the fundamental issue which is that the initial heartbeat is not idempotent. Then, we could describe the undesired side effects (e.g. ghost members, cannot leave without receiving the member id, etc.). This is the core issue that we want to solve with this KIP, I think. 2. In the public interface section, we need to explain that we will bump the version of the API and that we will require the member id to be provided from the new version on. I would also mention that we will return invalid request if the member id is not provided. 3. In the proposed changes section, we should elaborate the uuid generation part. Do we have recommendations there? Do we have requirements for the uuid (version, uniqueness, etc.)? 4. In the compatibility section, I wonder if we could simplify it a bit. In the end, the change is backward compatible because the version 1 of the RPC already supports a member id provided by the client. 5. In KIP-848, we actually rejected the proposed option. It would be great if we could explain why we changed our mind in the motivation section. I think that the main argument was that generating uuid was requiring extra libraries in some languages (e.g. for librdkafka). 6. Regarding the second rejected alternative, I feel like the arguments for rejecting it are quite weak. Backporting is not really an issue, compatibility neither. KIP-848 is still in preview so we could live with a few misleading logs, if any. In my opinion, the main downside of this approach is that if you leave after receiving the first HB and the member id, the server will respond with an unknown member id error because the member is not really in the group yet. We could think of having a real lobby as suggested by Andrew. I am not a fan of this because we have to bookkeep more state on the server. The client side generated member id is simpler and more reliable overall. 7. Another argument for the client side generated member id is that the member id will be equivalent to an incarnation id in the sense that it won't change at all during the lifetime of the client. 8. It seems tricky to validate the uuid on the server because it is just a string. Could you elaborate on this? Best, David On Wed, Aug 14, 2024 at 9:48 AM TengYao Chi <kiting...@gmail.com> wrote: > Hello Andrew > Thank you for your thoughtful suggestions and getting the discussion going. > > To AS1: > In the current scenario where the server generates the UUID, if the client > shuts down before receiving the memberId generated by the GC (regardless of > whether it’s a graceful shutdown or not), the GC will still have to wait > for the heartbeat timeout because the client doesn’t know its memberId. > This KIP indeed cannot completely resolve the idempotency issue, but it can > better handle shutdown scenarios under normal circumstances because the > client always knows its memberId. Even if the client shuts down immediately > after the initial heartbeat, as long as it performs a graceful shutdown and > sends a leave heartbeat, the GC can manage the situation and remove the > member. Therefore, the goal of this KIP is to address the issue where the > GC has to wait for the heartbeat timeout due to the client leaving without > knowing its memberId, which leads to reduced throughput and limited > scalability. > > The solution you suggest has also been proposed by David. The concern with > this approach is that it introduces additional complexity for > compatibility, as the new server would not immediately add the member to > the group, while the old server would. This requires clients to > differentiate whether their memberId has been added to the group or not, > which could result in unexpected logs. > > Best Regards, > TengYao > > Andrew Schofield <andrew_schofi...@live.com> 於 2024年8月14日 週三 上午12:29寫道： > > > Hi TengYao, > > Thanks for the KIP. I wonder if there’s a different way to close what > > is quite a small window. > > > > AS1: It is true that the initial heartbeat is not idempotent, but this > > remains > > true with this KIP. It’s just differently not idempotent. If the client > > makes its > > own member ID, sends a request and dies, the GC will still have added > > the member to the group and it will hang around until the session > expires. > > > > I wonder if the GC could still generate the member ID in response to the > > first > > heartbeat, and put the member in a special PENDING state with no > > assignments until the client sends the next heartbeat, thus confirming it > > has received the member ID. This would not be a protocol change at all, > > just > > a change to the GC to keep a new member in the lobby until it’s comfirmed > > it knows its member ID. > > > > > > Thanks, > > Andrew > > > > > On 13 Aug 2024, at 15:59, TengYao Chi <kiting...@gmail.com> wrote: > > > > > > Hi Chia-Ping, > > > > > > Thanks for review and suggestions. > > > I have updated the content of KIP accordingly. > > > Please take a look. > > > > > > Best regards, > > > TengYao > > > > > > Chia-Ping Tsai <chia7...@apache.org> 於 2024年8月13日 週二 下午9:45寫道： > > > > > >> hi TengYao > > >> > > >> thanks for this KIP. > > >> > > >> 1) could you please describe the before/after behavior in the > "Proposed > > >> Changes" section? IIRC, current RPC allows HB having member id > > generated by > > >> client, right? If HB has no member ID, server will generate one and > then > > >> return. The new behavior will enforce HB "must" have member ID. > > >> > > >> 2) could you please write the version number explicitly in the KIP > > >> > > >> 3) how new client code handle the old HB? Does it always generate > member > > >> ID on client-side even though that is not restricted? > > >> > > >> Best, > > >> Chia-Ping > > >> > > >> On 2024/08/13 06:20:42 TengYao Chi wrote: > > >>> Hello everyone, > > >>> > > >>> I would like to start a discussion thread on KIP-1082, which proposes > > >>> enabling id generation for clients over the ConsumerGroupHeartbeat > RPC. > > >>> > > >>> Here is the KIP Link: KIP-1082 > > >>> < > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1082%3A+Enable+ID+Generation+for+Clients+over+the+ConsumerGroupHeartbeat+RPC > > >>> > > >>> Please take a look and let me know what you think, and I would > > appreciate > > >>> any suggestions and feedback. > > >>> > > >>> Best regards, > > >>> TengYao > > >>> > > >> > > > > >
