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 >
