Hi Nikolay, Thanks for this very nice KIP!
To answer your questions: 1. Correct, we should not delete existing methods that have been released, but ... 2. Yes, we should deprecate the 'long' variants so that we can drop them later on. Personally, I like to mention which version deprecated the method so everyone can see later on how long it's been deprecated, but this may be controversial, so let's let other weigh in. 3. I think you're asking whether it's appropriate to drop the "Ms" suffix, and I think yes. So "long inactivityGapMs" would become "Duration inactivityGap". In the places where the parameter's name is just "duration", I think we can pick something more descriptive (I realize it was already "durationMs"; this is just a good time to improve it). Also, you're correct that we shouldn't use a Duration to represent a moment in time, like "startTime". The correct choice is actually "Instant", not "LocalDateTime", though. https://stackoverflow.com/questions/32437550/whats-the-difference-between-instant-and-localdatetime explains why. I also had a few notes on the KIP itself: 4. You might want to pull trunk again. I noticed some recent APIs are missing (see KIP-328). 5. Speaking of KIP-328: those APIs were just added and have never been released, so there's no need to deprecate the methods, you can just replace them. 6. For any existing method that's already deprecated, don't bother transitioning it to Duration. I think the examples I noticed were deprecated in KIP-328, so you'll see what I'm talking about when you pull trunk again. 7. Any method taking a Duration argument may throw an IllegalArgumentException (we choose to convert ArithmeticException to IllegalArgumentException, as I mentioned in the Jira ticket). We don't need a "throws" declaration, but we should plan to mention this in the javadoc for those methods. 8. In Stores, windowSize and segmentInterval should also be durations. 9. In StreamsMetrics, recordLatency could be just a Duration, but I actually think this one is better left alone. IMO, it's more effort for little gain to require users to construct a Duration before they call the method, since they vary likely call System.currentTimeNanos before and after the code in question. These are quite a few notes, but they're all minor. Overall the KIP looks really good to me. Thanks for picking this up! -John On Thu, Aug 16, 2018 at 9:55 AM Nikolay Izhikov <nizhi...@apache.org> wrote: > Hello, Kafka developers. > > I would like to start a discussion of KIP-358 [1]. > It based on a ticket KAFKA-7277 [2]. > > I crawled through Stream API and made my suggestions for API changes. > > I have several questions about changes. > Please, share your comments: > > 1. I propose do not remove existing API methods with long ms parameters. > Is it correct? > > 2. Should we mark existing methods as deprecated? > > 3. Suggested changes in ticket description are `long durationMs` to > `Duration duration` and similar. > I suggest to change 'long startTimeMs` to `LocalDateTime startTime` also. > Should we do it? > > Please, note, it very first KIP for me, so tell me if I miss something > obvious for experienced Kafka developers. > > [1] > https://cwiki.apache.org/confluence/display/KAFKA/KIP-358%3A+Migrate+Streams+API+to+Duration+instead+of+long+ms+times > [2] https://issues.apache.org/jira/browse/KAFKA-7277
