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
signature.asc
Description: This is a digitally signed message part
