Hi Nikolay, Thanks for the PR. I will review it.
-John On Sat, Sep 22, 2018 at 2:36 AM Nikolay Izhikov <nizhi...@apache.org> wrote: > Hello > > I've opened a PR [1] for this KIP. > > [1] https://github.com/apache/kafka/pull/5682 > > John, can you take a look? > > В Пн, 17/09/2018 в 20:16 +0300, Nikolay Izhikov пишет: > > John, > > > > Got it. > > > > Will do my best to meet this deadline. > > > > В Пн, 17/09/2018 в 11:52 -0500, John Roesler пишет: > > > Yay! Thanks so much for sticking with this Nikolay. > > > > > > I look forward to your PR! > > > > > > Not to put pressure on you, but just to let you know, the deadline for > > > getting your pr *merged* for 2.1 is _October 1st_, > > > so you basically have 2 weeks to send the PR, have the reviews, and > get it > > > merged. > > > > > > (see > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=91554044) > > > > > > Thanks again, > > > -John > > > > > > On Mon, Sep 17, 2018 at 10:29 AM Nikolay Izhikov <nizhi...@apache.org> > > > wrote: > > > > > > > This KIP is now accepted with: > > > > - 3 binding +1 > > > > - 2 non binding +1 > > > > > > > > Thanks, all. > > > > > > > > Especially, John, Matthias, Guozhang, Bill, Damian! > > > > > > > > В Чт, 13/09/2018 в 22:16 -0700, Guozhang Wang пишет: > > > > > +1 (binding), thank you Nikolay! > > > > > > > > > > Guozhang > > > > > > > > > > On Thu, Sep 13, 2018 at 9:39 AM, Matthias J. Sax < > matth...@confluent.io> > > > > > wrote: > > > > > > > > > > > Thanks for the KIP. > > > > > > > > > > > > +1 (binding) > > > > > > > > > > > > > > > > > > -Matthias > > > > > > > > > > > > On 9/5/18 8:52 AM, John Roesler wrote: > > > > > > > I'm a +1 (non-binding) > > > > > > > > > > > > > > On Mon, Sep 3, 2018 at 8:33 AM Nikolay Izhikov < > nizhi...@apache.org> > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Dear commiters. > > > > > > > > > > > > > > > > Please, vote on a KIP. > > > > > > > > > > > > > > > > В Пт, 31/08/2018 в 12:05 -0500, John Roesler пишет: > > > > > > > > > Hi Nikolay, > > > > > > > > > > > > > > > > > > You can start a PR any time, but we cannot per it (and > probably > > > > > > > > won't > > > > > > > > > > > > do > > > > > > > > > serious reviews) until after the KIP is voted and approved. > > > > > > > > > > > > > > > > > > Sometimes people start a PR during discussion just to help > > > > > > > > provide more > > > > > > > > > context, but it's not required (and can also be distracting > > > > > > > > because the > > > > > > > > > > > > > > > > KIP > > > > > > > > > discussion should avoid implementation details). > > > > > > > > > > > > > > > > > > Let's wait one more day for any other comments and plan to > start > > > > > > > > the > > > > > > > > > > > > vote > > > > > > > > > on Monday if there are no other debates. > > > > > > > > > > > > > > > > > > Once you start the vote, you have to leave it up for at > least 72 > > > > > > > > hours, > > > > > > > > > > > > > > > > and > > > > > > > > > it requires 3 binding votes to pass. Only Kafka Committers > have > > > > > > > > binding > > > > > > > > > votes (https://kafka.apache.org/committers). > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > -John > > > > > > > > > > > > > > > > > > On Fri, Aug 31, 2018 at 11:09 AM Bill Bejeck < > bbej...@gmail.com> > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Nickolay, > > > > > > > > > > > > > > > > > > > > Thanks for the clarification. > > > > > > > > > > > > > > > > > > > > -Bill > > > > > > > > > > > > > > > > > > > > On Fri, Aug 31, 2018 at 11:59 AM Nikolay Izhikov < > > > > > > > > nizhi...@apache.org > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hello, John. > > > > > > > > > > > > > > > > > > > > > > This is my first KIP, so, please, help me with kafka > > > > > > > > development > > > > > > > > > > > > > > > > process. > > > > > > > > > > > > > > > > > > > > > > Should I start to work on PR now? Or should I wait for > a > > > > > > > > "+1" from > > > > > > > > > > > commiters? > > > > > > > > > > > > > > > > > > > > > > В Пт, 31/08/2018 в 10:33 -0500, John Roesler пишет: > > > > > > > > > > > > I see. I guess that once we are in the PR-reviewing > phase, > > > > > > > > we'll > > > > > > > > > > > > > > > > be in > > > > > > > > > > > > > > > > > > > > a > > > > > > > > > > > > better position to see what else can/should be done, > and > > > > > > > > we can > > > > > > > > > > > > > > > > talk > > > > > > > > > > > > > > > > > > > > > > about > > > > > > > > > > > > follow-on work at that time. > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the clarification, > > > > > > > > > > > > -John > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Aug 31, 2018 at 1:19 AM Nikolay Izhikov < > > > > > > > > > > > > > > > > nizhi...@apache.org> > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hello, Bill > > > > > > > > > > > > > > > > > > > > > > > > > > > In the "Proposed Changes" section, there is "Try > to > > > > > > > > reduce the > > > > > > > > > > > > > > > > > > > > > > > > > > visibility of methods in next tickets" does that > mean > > > > > > > > eventual > > > > > > > > > > > > > > > > > > > > > > deprecation > > > > > > > > > > > > > and removal? > > > > > > > > > > > > > > > > > > > > > > > > > > 1. Some methods will become deprecated. I think > they > > > > > > > > will be > > > > > > > > > > > > > > > > removed > > > > > > > > > > > > > > > > > > > > in > > > > > > > > > > > > > the future. > > > > > > > > > > > > > You can find list of deprecated methods in KIP. > > > > > > > > > > > > > > > > > > > > > > > > > > 2. Some internal methods can't be deprecated or > hid from > > > > > > > > the > > > > > > > > > > > > > > > > user for > > > > > > > > > > > > > > > > > > > > > > now. > > > > > > > > > > > > > I was trying to say that we should research > possibility > > > > > > > > to reduce > > > > > > > > > > > > > visibility of *internal* methods that are *public* > now. > > > > > > > > > > > > > That kind of changes is out of the scope of > current KIP, > > > > > > > > so we > > > > > > > > > > > > > > > > have > > > > > > > > > > > > > > > > > > > > to > > > > > > > > > > > do > > > > > > > > > > > > > it in the next tickets. > > > > > > > > > > > > > > > > > > > > > > > > > > I don't expect that internal methods will be > removed. > > > > > > > > > > > > > > > > > > > > > > > > > > В Чт, 30/08/2018 в 18:59 -0400, Bill Bejeck пишет: > > > > > > > > > > > > > > Sorry for chiming in late, there was a lot of > detail > > > > > > > > to catch > > > > > > > > > > > > > > > > up > > > > > > > > > > > > > > > > > > > > on. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Overall I'm +1 in the KIP. But I do have one > question > > > > > > > > about > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > > KIP > > > > > > > > > > > in > > > > > > > > > > > > > > regards to Matthias's comments about defining > dual use. > > > > > > > > > > > > > > > > > > > > > > > > > > > > In the "Proposed Changes" section, there is "Try > to > > > > > > > > reduce the > > > > > > > > > > > > > > > > > > > > > > visibility > > > > > > > > > > > > > > of methods in next tickets" does that mean > eventual > > > > > > > > > > > > > > > > deprecation and > > > > > > > > > > > > > > > > > > > > > > > > > > removal? > > > > > > > > > > > > > > I thought we were aiming to keep the dual use > methods? > > > > > > > > Or does > > > > > > > > > > > > > > > > that > > > > > > > > > > > > > > > > > > > > > > imply > > > > > > > > > > > > > > we'll strive for more clear delineation between > DSL and > > > > > > > > > > > > > > > > internal > > > > > > > > > > > > > > > > > > > > use? > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > Bill > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Aug 30, 2018 at 5:59 PM Nikolay Izhikov < > > > > > > > > > > > > > > > > > > > > nizhi...@apache.org > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > John, thank you. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I've updated KIP. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Dear commiters, please take a look and share > your > > > > > > > > opinion. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > В Чт, 30/08/2018 в 14:58 -0500, John Roesler > пишет: > > > > > > > > > > > > > > > > Oh! I missed one minor thing: > UnlimitedWindows > > > > > > > > doesn't > > > > > > > > > > > > > > > > need to > > > > > > > > > > > > > > > > > > > > > > set > > > > > > > > > > > > > > > > > > > > > > > > > > grace > > > > > > > > > > > > > > > > (it currently does not either). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Otherwise, it looks good to me! > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks so much, > > > > > > > > > > > > > > > > -John > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Aug 30, 2018 at 5:30 AM Nikolay > Izhikov < > > > > > > > > > > > > > > > > > > > > > > nizhi...@apache.org > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hello, John. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I've updated KIP according on your > comments. > > > > > > > > > > > > > > > > > Please, take a look. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Are we ready to vot now? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > В Ср, 29/08/2018 в 14:51 -0500, John > Roesler > > > > > > > > пишет: > > > > > > > > > > > > > > > > > > Hey Nikolay, sorry for the silence. I'm > taking > > > > > > > > another > > > > > > > > > > > > > > > > look > > > > > > > > > > > > > > > > > > > > > > at > > > > > > > > > > > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > KIP > > > > > > > > > > > > > > > > > > before voting... > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1. I think the Window constructor > should > > > > > > > > actually be > > > > > > > > > > > > > > > > > > > > > > > > > > protected. I > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > don't > > > > > > > > > > > > > > > > > > know if we need a constructor that > takes > > > > > > > > Instant, > > > > > > > > > > > > > > > > but if > > > > > > > > > > > > > > > > > > > > > > we > > > > > > > > > > > > > > > > > > > > > > > > > > do add > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > one, it > > > > > > > > > > > > > > > > > > should definitely be protected. > > > > > > > > > > > > > > > > > > 2. `long JoinWindows#size()` is > overridden > > > > > > > > from > > > > > > > > > > > > > > > > `long > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Windows#size()`, > > > > > > > > > > > > > > > > > > and should not be deprecated. Also, I > don't > > > > > > > > think we > > > > > > > > > > > > > > > > > > > > need > > > > > > > > > > > a > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > `Duration > > > > > > > > > > > > > > > > > > JoinWindows#windowSize()`. > > > > > > > > > > > > > > > > > > 3. Likewise, > `JoinWindows#windowsFor()` is > > > > > > > > > > > > > > > > overridden > > > > > > > > > > > > > > > > > > > > from > > > > > > > > > > > > > > > > > > `Windows#windowsFor()` and should > also not > > > > > > > > be > > > > > > > > > > > > > > > > > > > > deprecated, > > > > > > > > > > > and > > > > > > > > > > > > > > > > > > > > > > > > > > we > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > also > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > don't > > > > > > > > > > > > > > > > > > need a `Map<Instant, Window> > > > > > > > > windowsForTime(final > > > > > > > > > > > > > > > > > > > > Instant > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > timestamp)` > > > > > > > > > > > > > > > > > > version. > > > > > > > > > > > > > > > > > > 4. TimeWindowedDeserializer is a bit > of a > > > > > > > > puzzle > > > > > > > > > > > > > > > > for me. > > > > > > > > > > > > > > > > > > > > > > It > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > actually > > > > > > > > > > > > > > > > > > looks like it's incorrectly > implemented! > > > > > > > > I'm not > > > > > > > > > > > > > > > > sure if > > > > > > > > > > > > > > > > > > > > > > we > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > want/need > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > > > update any of its methods or > constructors. > > > > > > > > > > > > > > > > > > 5. TimeWindows: see my feedback on > > > > > > > > JoinWindows > > > > > > > > > > > > > > > > > > 6. UnlimitedWindows: see my feedback > on > > > > > > > > JoinWindows > > > > > > > > > > > > > > > > > > 7. ReadOnlyWindowStore: the existing > `long` > > > > > > > > methods > > > > > > > > > > > > > > > > > > > > > > should be > > > > > > > > > > > > > > > > > > deprecated. (we should add > > > > > > > > `WindowStoreIterator<V> > > > > > > > > > > > > > > > > > > > > fetch(K > > > > > > > > > > > > > > > > > > > > > > > > > > key, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > long > > > > > > > > > > > > > > > > > > timeFrom, long timeTo)` to > WindowStore) > > > > > > > > > > > > > > > > > > 8. SessionBytesStoreSupplier: Both of > those > > > > > > > > methods > > > > > > > > > > > > > > > > are > > > > > > > > > > > > > > > > > > > > > > > > > > "internal > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > use > > > > > > > > > > > > > > > > > > methods", so we should just leave them > > > > > > > > alone and > > > > > > > > > > > > > > > > not add > > > > > > > > > > > > > > > > > > > > > > new > > > > > > > > > > > > > > > > > > > > > > > > > > ones. > > > > > > > > > > > > > > > > > > 9. SessionStore: I don't think these > are > > > > > > > > "external > > > > > > > > > > > > > > > > use" > > > > > > > > > > > > > > > > > > > > > > > > > > methods > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > (only > > > > > > > > > > > > > > > > > > ReadOnlySessionStore is used in IQ) > maybe > > > > > > > > we should > > > > > > > > > > > > > > > > just > > > > > > > > > > > > > > > > > > > > > > leave > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > them > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > alone? > > > > > > > > > > > > > > > > > > 10. Stores: I think we can just > deprecate > > > > > > > > without > > > > > > > > > > > > > > > > > > > > > > replacement > > > > > > > > > > > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > method > > > > > > > > > > > > > > > > > > that takes `segmentInterval`. > > > > > > > > > > > > > > > > > > 11. WindowBytesStoreSupplier: I think > this > > > > > > > > > > > > > > > > interface is > > > > > > > > > > > > > > > > > > > > > > also > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > "internal > > > > > > > > > > > > > > > > > > use" and can be left alone > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thank you for the very clear KIP that > makes > > > > > > > > this > > > > > > > > > > > > > > > > discussion > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > possible. In > > > > > > > > > > > > > > > > > > general, to justify some of those > comments, > > > > > > > > it's > > > > > > > > > > > > > > > > easier to > > > > > > > > > > > > > > > > > > > > > > add > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > missing > > > > > > > > > > > > > > > > > > methods later on than to remove them, so > I'm > > > > > > > > erring on > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > > > > side > > > > > > > > > > > > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > only > > > > > > > > > > > > > > > > > > adding new variants when they show up in > DSL > > > > > > > > code, not > > > > > > > > > > > > > > > > > > > > > > worrying > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > about the > > > > > > > > > > > > > > > > > > lower-level APIs. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > What do you think about this? > > > > > > > > > > > > > > > > > > -John > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Aug 29, 2018 at 11:14 AM Nikolay > > > > > > > > Izhikov < > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > nizhi...@apache.org> > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hello, All. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Calling a vote on KIP-358 [1] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > > > > > > > > > 358%3A+Migrate+Streams+API+to+Duration+instead+of+long+ms+times > > > > > > > > > > > > > > > > > > > > > >
