Hi Sophie. Thanks for the KIP. I liked how focused the proposal is. Also, its motivation is clear after carefully reading the KIP and its references.
Yet, I think it'd be a good idea to call out explicitly on the Rejected Alternatives section that an automatic and periodic triggering of rebalances that would not require exposing this capability through the Consumer interface does not cover your specific use cases and therefore is not chosen as a desired approach. Maybe, even consider mentioning again here that this method is expected to be used to respond to system changes external to the consumer and its membership logic and is not proposed as a way to resolve temporary imbalances due to membership changes that should inherently be resolved by the assignor logic itself with one or more consecutive rebalances. Also, in your javadoc I'd add some context similar to what someone can read on the KIP. Specifically where you say: "for example if some condition has changed that has implications for the partition assignment." I'd rather add something like "for example, if some condition external and invisible to the Consumer and its group membership has changed in ways that would justify a new partition assignment". That's just an example, feel free to reword, but I believe that saying explicitly that this condition is not visible to the consumer is useful to understand that this is not necessary under normal circumstances. In Compatibility, Deprecation, and Migration Plan section I think it's worth mentioning that this is a new feature that affects new implementations of the Consumer interface and any such new implementation should override the new method. Implementations that wish to upgrade to a newer version should be extended and recompiled, since no default implementation will be provided. Naming is hard here, if someone wants to emphasize the ad hoc and irregular nature of this call. After some thought I'm fine with 'enforceRebalance' even if it could potentially be confused to a method that is supposed to be called to remediate one or more previously unsuccessful rebalances (which is partly what StreamThread#enforceRebalance is used for). The best I could think of was 'onRequestRebalance' but that's not perfect either. Best, Konstantine On Mon, Feb 10, 2020 at 5:18 PM Sophie Blee-Goldman <sop...@confluent.io> wrote: > Thanks John. I took out the KafkaConsumer method and moved the javadocs > to the Consumer#enforceRebalance in the KIP -- hope you're happy :P > > Also, I wanted to point out one minor change to the current proposal: make > this > a blocking call, which accepts a timeout and returns whether the rebalance > completed within the timeout. It will still reduce to a nonblocking call if > a "zero" > timeout is supplied. I've updated the KIP accordingly. > > Let me know if there are any further concerns, else I'll call for a vote. > > Cheers! > Sophie > > On Mon, Feb 10, 2020 at 12:47 PM John Roesler <vvcep...@apache.org> wrote: > > > Thanks Sophie, > > > > Sorry I didn't respond. I think your new method name sounds good. > > > > Regarding the interface vs implementation, I agree it's confusing. It's > > always bothered me that the interface redirects you to an implementation > > JavaDocs, but never enough for me to stop what I'm doing to fix it. > > It's not a big deal either way, I just thought it was strange to propose > a > > "public interface" change, but not in terms of the actual interface > class. > > > > It _is_ true that KafkaConsumer is also part of the public API, but only > > really > > for the constructor. Any proposal to define a new "consumer client" API > > should be on the Consumer interface (which you said you plan to do > anyway). > > I guess I brought it up because proposing an addition to Consumer implies > > it would be added to KafkaConsumer, but proposing an addition to > > KafkaConsumer does not necessarily imply it would also be added to > > Consumer. Does that make sense? > > > > Anyway, thanks for updating the KIP. > > > > Thanks, > > -John > > > > > > On Mon, Feb 10, 2020, at 14:38, Sophie Blee-Goldman wrote: > > > Since this doesn't seem too controversial, I'll probably call for a > vote > > by > > > end of day. > > > If there any further comments/questions/concerns, please let me know! > > > > > > Thanks, > > > Sophie > > > > > > On Sat, Feb 8, 2020 at 12:19 AM Sophie Blee-Goldman < > sop...@confluent.io > > > > > > wrote: > > > > > > > Thanks for the feedback! That's a good point about trying to prevent > > users > > > > from > > > > thinking they should use this API during normal processing and > > clarifying > > > > when/why > > > > you might need it -- regardless of the method name, we should > > explicitly > > > > call this out > > > > in the javadocs. > > > > > > > > As for the method name, on reflection I agree that "rejoinGroup" does > > not > > > > seem to be > > > > appropriate. Of course that's what the consumer will actually be > doing, > > > > but that's just an > > > > implementation detail -- the name should reflect what the API is > doing, > > > > not how it does it > > > > (which can always change). > > > > > > > > How about "enforceRebalance"? This is stolen from the StreamThread > > method > > > > that does > > > > exactly this (by unsubscribing) so it seems to fit. I'll update the > KIP > > > > with this unless anyone > > > > has another suggestion. > > > > > > > > Regarding the Consumer vs KafkaConsumer matter, I included the > > > > KafkaConsumer method > > > > because that's where all the javadocs redirect to in the Consumer > > > > interface. Also, FWIW > > > > I'm pretty sure KafkaConsumer is also part of the public API -- we > > would > > > > be adding a new > > > > method to both. > > > > > > > > On Fri, Feb 7, 2020 at 7:42 PM John Roesler <vvcep...@apache.org> > > wrote: > > > > > > > >> Hi all, > > > >> > > > >> Thanks for the well motivated KIP, Sophie. I had some alternatives > in > > > >> mind, which > > > >> I won't even bother to relate because I feel like the motivation > made > > a > > > >> compelling > > > >> argument for the API as proposed. > > > >> > > > >> One very minor point you might as well fix is that the API change is > > > >> targeted at > > > >> KafkaConsumer (the implementation), but should be targeted at > > > >> Consumer (the interface). > > > >> > > > >> I agree with your discomfort about the name. Adding a "rejoin" > method > > > >> seems strange > > > >> since there's no "join" method. Instead the way you join the group > the > > > >> first time is just > > > >> by calling "subscribe". But "resubscribe" seems too indirect from > what > > > >> we're really trying > > > >> to do, which is to trigger a rebalance by sending a new JoinGroup > > request. > > > >> > > > >> Another angle is that we don't want the method to sound like > something > > > >> you should > > > >> be calling in normal circumstances, or people will be "tricked" into > > > >> calling it unnecessarily. > > > >> > > > >> So, I think "rejoinGroup" is fine, although a person _might_ be > > forgiven > > > >> for thinking they > > > >> need to call it periodically or something. Did you consider > > > >> "triggerRebalance", which > > > >> sounds pretty advanced-ish, and accurately describes what happens > when > > > >> you call it? > > > >> > > > >> All in all, the KIP sounds good to me, and I'm in favor. > > > >> > > > >> Thanks, > > > >> -John > > > >> > > > >> On Fri, Feb 7, 2020, at 21:22, Anna McDonald wrote: > > > >> > This situation was discussed at length after a recent talk I gave. > > This > > > >> KIP > > > >> > would be a great step towards increased availability and in > > facilitating > > > >> > lightweight rebalances. > > > >> > > > > >> > anna > > > >> > > > > >> > > > > >> > > > > >> > On Fri, Feb 7, 2020, 9:38 PM Sophie Blee-Goldman < > > sop...@confluent.io> > > > >> > wrote: > > > >> > > > > >> > > Hi all, > > > >> > > > > > >> > > In light of some recent and upcoming rebalancing and > availability > > > >> > > improvements, it seems we have a need for explicitly triggering > a > > > >> consumer > > > >> > > group rebalance. Therefore I'd like to propose adding a new > > > >> > > rejoinGroup()method > > > >> > > to the Consumer client (better method name suggestions are very > > > >> welcome). > > > >> > > > > > >> > > Please take a look at the KIP and let me know what you think! > > > >> > > > > > >> > > KIP document: > > > >> > > > > > >> > > > > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-568%3A+Explicit+rebalance+triggering+on+the+Consumer > > > >> > > > > > >> > > JIRA: https://issues.apache.org/jira/browse/KAFKA-9525 > > > >> > > > > > >> > > Cheers, > > > >> > > Sophie > > > >> > > > > > >> > > > > >> > > > > > > > > > >
