Thanks, Vinoth and John for making the last minute improvements. I have gone
through the PR and looks good to me.
On Thursday, 16 January, 2020, 12:42:09 am IST, Guozhang Wang
<wangg...@gmail.com> wrote:
Thanks for the update of the PR John! I have taken a look at 7962 and it
looks good to me overall.
Guozhang
On Wed, Jan 15, 2020 at 9:35 AM John Roesler <vvcep...@apache.org> wrote:
> Hello again all,
>
> I had a bit of inspiration last night and realized that it's not necessary
> (and maybe even inappropriate) for StreamThreadStateStoreProvider
> and WrappingStoreProvider to implement the public StateStoreProvider
> interface.
>
> By breaking this dependency, I was able to implement the flag without
> touching
> any public interfaces except adding the new overload to KafkaStreams as
> originally
> discussed.
>
> You can take a look at https://github.com/apache/kafka/pull/7962 for the
> details.
>
> Since there was no objection to that new overload, I'll go ahead and
> update the KIP
> and we can proceed with a final round of code reviews on
> https://github.com/apache/kafka/pull/7962
>
> Thanks, all,
> -John
>
> On Tue, Jan 14, 2020, at 22:52, Matthias J. Sax wrote:
> > 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
> >
> >
> > Attachments:
> > * signature.asc
>
--
-- Guozhang
