Thanks. SGTM. -Matthias
On 1/14/20 4:52 PM, John Roesler wrote: > 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
signature.asc
Description: OpenPGP digital signature
