Hey Matthias, Thanks for taking a look! I felt a little uneasy about it, but didn’t think about the case you pointed out. Throwing an exception would indeed be safer.
Given a choice between throwing in the default method or defining a new interface and throwing if the wrong interface is implemented, it seems nicer for everyone to go the default method route. Since we’re not referencing the other method anymore, I should probably deprecate it, too. Thanks again for your help. I really appreciate it. -John On Tue, Jan 14, 2020, at 18:15, Matthias J. Sax wrote: > Thanks for the PR. That helps a lot. > > I actually do have a concern: the proposed default method, would disable > the new feature to allow querying an active task during restore > automatically. Hence, if a user has an existing custom store type, and > would use the new > > KafkaStreams.store(..., true) > > method to enable querying during restore it would not work, and it would > be unclear why. It would even be worth if there are two developers and > one provide the store type while the other one just uses it. > > Hence, the default implementation should maybe throw an exception by > default? Or maybe, we would introduce a new interface that extends > `QueryableStoreType` and add this new method. For this case, we could > check within > > KafkaStreams.store(..., true) > > if the StoreType implements the new interface and if not, throw an > exception there. > > Those exceptions would be more descriptive (ie, state store does not > support querying during restore) and give the user a chance to figure > out what's wrong. > > Not sure if overwriting a default method or a new interface is the > better choice to let people opt-into the feature. > > > > -Matthias > > On 1/14/20 3:22 PM, John Roesler wrote: > > Hi again all, > > > > I've sent a PR including this new option, and while implementing it, I > > realized we also have to make a (source-compatible) addition to the > > QueryableStoreType interface, so that the IQ store wrapper can pass the > > flag down to the composite store provider. > > > > See https://github.com/apache/kafka/pull/7962 > > In particular: > > https://github.com/apache/kafka/pull/7962/files#diff-d0242b7289f4e0886490351a5a803d41 > > > > If there are no objections to these additions, I'll update the KIP tomorrow. > > > > Thanks, > > -John > > > > On Tue, Jan 14, 2020, at 14:11, John Roesler wrote: > >> Thanks for calling this out, Matthias. You're correct that this looks like > >> a > >> harmful behavioral change. I'm fine with adding the new overload you > >> mentioned, just a simple boolean flag to enable the new behavior. > >> > >> I'd actually propose that we call this flag "queryStaleData", with a > >> default > >> of "false". The meaning of this would be to preserve exactly the existing > >> semantics. I.e., that the store must be both active and running to be > >> included. > >> > >> It seems less severe to just suddenly start returning queries from > >> standbys, > >> but in the interest of safety, the easiest thing is just flag the whole > >> feature. > >> > >> If you, Navinder, and Vinoth agree, we can just update the KIP. It seems > >> like > >> a pretty uncontroversial amendment to avoid breaking query consistency > >> semantics. > >> > >> Thanks, > >> -John > >> > >> > >> On Tue, Jan 14, 2020, at 13:21, Matthias J. Sax wrote: > >>> During the discussion of KIP-216 > >>> (https://cwiki.apache.org/confluence/display/KAFKA/KIP-216%3A+IQ+should+throw+different+exceptions+for+different+errors) > >>> we encountered that KIP-535 introduces a behavior change that is not > >>> backward compatible, hence, I would like to request a small change. > >>> > >>> KIP-535 suggests, that active tasks can be queried during recovery and > >>> no exception would be thrown and longer. This is a change in behavior > >>> and in fact introduces a race condition for users that only want to > >>> query consistent state. Querying inconsistent state should be an opt-in, > >>> and for StandbyTasks, user can opt-in by querying them or opt-out by not > >>> querying them. However, for active task, if we don't throw an exception > >>> during recovery, users cannot opt-out of querying potentially > >>> inconsistent state, because they would need to check the state (ie, == > >>> RUNNING) before they would query the active task -- however, the state > >>> might change at any point in between, and there is a race. > >>> > >>> Hence, I would suggest to actually not change the default behavior of > >>> existing methods and we should throw an exception during restore if an > >>> active task is queried. To allow user to opt-in to query an active task > >>> during restore, we would add an overload > >>> > >>>> KafkaStream#store(..., boolean allowQueryWhileStateIsRestoring) > >>> > >>> (with an hopefully better/short variable name). Developers would use > >>> this new method to opt-into querying active tasks during restore. > >>> > >>> Thoughts? > >>> > >>> > >>> -Matthias > >>> > >>> On 11/18/19 10:45 AM, Vinoth Chandar wrote: > >>>> Thanks, everyone involved! > >>>> > >>>> On Mon, Nov 18, 2019 at 7:51 AM John Roesler <j...@confluent.io> wrote: > >>>> > >>>>> Thanks to you, also, Navinder! > >>>>> > >>>>> Looking forward to getting this feature in. > >>>>> -John > >>>>> > >>>>> On Sun, Nov 17, 2019 at 11:34 PM Navinder Brar > >>>>> <navinder_b...@yahoo.com.invalid> wrote: > >>>>>> > >>>>>> Hello all, > >>>>>> > >>>>>> With 4 binding +1 votes from Guozhang Wang, Matthias J. Sax, Bill > >>>>>> Bejeck, > >>>>>> and John Roesler, the vote passes. > >>>>>> Thanks Guozhang, Matthias, Bill, John, Sophie for the healthy > >>>>> discussions and Vinoth for all the help on this KIP. > >>>>>> Best, > >>>>>> Navinder > >>>>>> > >>>>>> On Friday, 15 November, 2019, 11:32:31 pm IST, John Roesler < > >>>>> j...@confluent.io> wrote: > >>>>>> > >>>>>> I'm +1 (binding) as well. > >>>>>> > >>>>>> Thanks, > >>>>>> -John > >>>>>> > >>>>>> On Fri, Nov 15, 2019 at 6:20 AM Bill Bejeck <bbej...@gmail.com> wrote: > >>>>>>> > >>>>>>> +1 (binding) > >>>>>>> > >>>>>>> On Fri, Nov 15, 2019 at 1:11 AM Matthias J. Sax <matth...@confluent.io > >>>>>> > >>>>>>> wrote: > >>>>>>> > >>>>>>>> +1 (binding) > >>>>>>>> > >>>>>>>> > >>>>>>>> On 11/14/19 3:48 PM, Guozhang Wang wrote: > >>>>>>>>> +1 (binding), thanks for the KIP! > >>>>>>>>> > >>>>>>>>> Guozhang > >>>>>>>>> > >>>>>>>>> On Fri, Nov 15, 2019 at 4:38 AM Navinder Brar > >>>>>>>>> <navinder_b...@yahoo.com.invalid> wrote: > >>>>>>>>> > >>>>>>>>>> Hello all, > >>>>>>>>>> > >>>>>>>>>> I'd like to propose a vote for serving interactive queries during > >>>>>>>>>> Rebalancing, as it is a big deal for applications looking for high > >>>>>>>>>> availability. With this change, users will have control over the > >>>>>>>> tradeoff > >>>>>>>>>> between consistency and availability during serving. > >>>>>>>>>> The full KIP is provided here: > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>> > >>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-535%3A+Allow+state+stores+to+serve+stale+reads+during+rebalance > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Thanks, > >>>>>>>>>> Navinder > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >>> > >>> Attachments: > >>> * signature.asc > >> > > > Attachments: > * signature.asc
