Hello again, all, I belatedly had a better idea for adding grace period to the Windows class hierarchy (TimeWindows, UnlimitedWindows, JoinWindows). Instead of providing the grace-setter in the abstract class and having to retract it in UnlimitedWindows, I've made the getter abstract method in Windows and only added setters to Time and Join windows.
This should not only improve the ergonomics of grace period, but make the whole class hierarchy more maintainable. See the PR for more details: https://github.com/apache/kafka/pull/5536 I've updated the KIP accordingly. Here's the diff: https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=87295409&selectedPageVersions=11&selectedPageVersions=9 Please let me know if this changes your vote. Thanks, -John On Mon, Aug 13, 2018 at 5:20 PM John Roesler <j...@confluent.io> wrote: > Hey all, > > I just wanted to let you know that a few small issues surfaced during > implementation and review. I've updated the KIP. Here's the diff: > https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=87295409&selectedPageVersions=9&selectedPageVersions=8 > > Basically: > * the metrics named "*-event-*" are inconsistent with existing > nomenclature, and will be "*-record-*" instead (late records instead of > late events, for example) > * the apis taking and returning Duration will use long millis instead. We > do want to transition to Duration in the future, but we shouldn't do it > piecemeal. > > Thanks, > -John > > On Tue, Aug 7, 2018 at 12:07 PM John Roesler <j...@confluent.io> wrote: > >> Thanks everyone, KIP-328 has passed with 3 binding votes (Guozhang, >> Damian, and Matthias) and 3 non-binding (Ted, Bill, and me). >> >> Thanks for your time, >> -John >> >> On Mon, Aug 6, 2018 at 6:35 PM Matthias J. Sax <matth...@confluent.io> >> wrote: >> >>> +1 (binding) >>> >>> Thanks for the KIP. >>> >>> >>> -Matthias >>> >>> On 8/3/18 12:52 AM, Damian Guy wrote: >>> > Thanks John! +1 >>> > >>> > On Mon, 30 Jul 2018 at 23:58 Guozhang Wang <wangg...@gmail.com> wrote: >>> > >>> >> Yes, the addendum lgtm as well. Thanks! >>> >> >>> >> On Mon, Jul 30, 2018 at 3:34 PM, John Roesler <j...@confluent.io> >>> wrote: >>> >> >>> >>> Another thing that came up after I started working on an >>> implementation >>> >> is >>> >>> that in addition to deprecating "retention" from the Windows >>> interface, >>> >> we >>> >>> also need to deprecate "segmentInterval", for the same reasons. I >>> simply >>> >>> overlooked it previously. I've updated the KIP accordingly. >>> >>> >>> >>> Hopefully, this doesn't change anyone's vote. >>> >>> >>> >>> Thanks, >>> >>> -John >>> >>> >>> >>> On Mon, Jul 30, 2018 at 5:31 PM John Roesler <j...@confluent.io> >>> wrote: >>> >>> >>> >>>> Thanks Guozhang, >>> >>>> >>> >>>> Thanks for that catch. to clarify, currently, events are "late" only >>> >> when >>> >>>> they are older than the retention period. Currently, we detect this >>> in >>> >>> the >>> >>>> processor and record it as a "skipped-record". We then do not >>> attempt >>> >> to >>> >>>> store the event in the window store. If a user provided a >>> >> pre-configured >>> >>>> window store with a retention period smaller than the one they >>> specify >>> >>> via >>> >>>> Windows#until, the segmented store will drop the update with no >>> metric >>> >>> and >>> >>>> record a debug-level log. >>> >>>> >>> >>>> With KIP-328, with the introduction of "grace period" and moving >>> >>> retention >>> >>>> fully into the state store, we need to have metrics for both "late >>> >>> events" >>> >>>> (new records older than the grace period) and "expired window >>> events" >>> >>> (new >>> >>>> records for windows that are no longer retained in the state >>> store). I >>> >>>> already proposed metrics for the late events, and I've just updated >>> the >>> >>> KIP >>> >>>> with metrics for the expired window events. I also updated the KIP >>> to >>> >>> make >>> >>>> it clear that neither late nor expired events will count as >>> >>>> "skipped-records" any more. >>> >>>> >>> >>>> -John >>> >>>> >>> >>>> On Mon, Jul 30, 2018 at 4:22 PM Guozhang Wang <wangg...@gmail.com> >>> >>> wrote: >>> >>>> >>> >>>>> Hi John, >>> >>>>> >>> >>>>> Thanks for the updated KIP, +1 from me, and one minor suggestion: >>> >>>>> >>> >>>>> Following your suggestion of the differentiation of >>> `skipped-records` >>> >>> v.s. >>> >>>>> `late-event-drop`, we should probably consider moving the scenarios >>> >>> where >>> >>>>> records got ignored due the window not being available any more in >>> >>>>> windowed >>> >>>>> aggregation operators from the `skipped-records` metrics recording >>> to >>> >>> the >>> >>>>> `late-event-drop` metrics recording. >>> >>>>> >>> >>>>> >>> >>>>> >>> >>>>> Guozhang >>> >>>>> >>> >>>>> >>> >>>>> On Mon, Jul 30, 2018 at 1:36 PM, Bill Bejeck <bbej...@gmail.com> >>> >> wrote: >>> >>>>> >>> >>>>>> Thanks for the KIP! >>> >>>>>> >>> >>>>>> +1 >>> >>>>>> >>> >>>>>> -Bill >>> >>>>>> >>> >>>>>> On Mon, Jul 30, 2018 at 3:42 PM Ted Yu <yuzhih...@gmail.com> >>> wrote: >>> >>>>>> >>> >>>>>>> +1 >>> >>>>>>> >>> >>>>>>> On Mon, Jul 30, 2018 at 11:46 AM John Roesler <j...@confluent.io >>> > >>> >>>>> wrote: >>> >>>>>>> >>> >>>>>>>> Hello devs, >>> >>>>>>>> >>> >>>>>>>> The discussion of KIP-328 has gone some time with no new >>> >> comments, >>> >>>>> so I >>> >>>>>>> am >>> >>>>>>>> calling for a vote! >>> >>>>>>>> >>> >>>>>>>> Here's the KIP: https://cwiki.apache.org/confluence/x/sQU0BQ >>> >>>>>>>> >>> >>>>>>>> The basic idea is to provide: >>> >>>>>>>> * more usable control over update rate (vs the current state >>> >> store >>> >>>>>>> caches) >>> >>>>>>>> * the final-result-for-windowed-computations feature which >>> >>> several >>> >>>>>> people >>> >>>>>>>> have requested >>> >>>>>>>> >>> >>>>>>>> Thanks, >>> >>>>>>>> -John >>> >>>>>>>> >>> >>>>>>> >>> >>>>>> >>> >>>>> >>> >>>>> >>> >>>>> >>> >>>>> -- >>> >>>>> -- Guozhang >>> >>>>> >>> >>>> >>> >>> >>> >> >>> >> >>> >> >>> >> -- >>> >> -- Guozhang >>> >> >>> > >>> >>>
