Thanks for the quick turnaround Sophie. My points have been addressed. I think the intended use is quite clear now.
Best, Konstantine On Tue, Feb 11, 2020 at 12:57 PM Sophie Blee-Goldman <sop...@confluent.io> wrote: > Konstantine, > Thanks for the feedback! I've updated the sections with your suggestions. I > agree > in particular that it's really important to make sure users don't call this > unnecessarily, > or for the wrong reasons: to that end I also extended the javadocs to > specify that this > API is for when changes to the subscription userdata occur. Hopefully that > should make > its intended usage quite clear. > > Bill, > The rebalance triggered by this new API will be a "normal" rebalance, and > therefore > follow the existing listener semantics. For example a cooperative rebalance > will always > call onPartitionsAssigned, even if no partitions are actually moved. > An eager rebalance will still revoke all partitions first anyway. > > Thanks for the feedback! > Sophie > > On Tue, Feb 11, 2020 at 9:52 AM Bill Bejeck <bbej...@gmail.com> wrote: > > > Hi Sophie, > > > > Thanks for the KIP, makes sense to me. > > > > One quick question, I'm not sure if it's relevant or not. > > > > If a user provides a `ConsumerRebalanceListener` and a rebalance is > > triggered from an `enforceRebalance` call, > > it seems possible the listener won't get called since partition > assignments > > might not change. > > If that is the case, do we want to possibly consider adding a method to > the > > `ConsumerRebalanceListener` for callbacks on `enforceRebalance` actions? > > > > Thanks, > > Bill > > > > > > > > On Tue, Feb 11, 2020 at 12:11 PM Konstantine Karantasis < > > konstant...@confluent.io> wrote: > > > > > 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 > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > >
