Hi Tom, Thanks for your review.
My original intent was to use the naming available in TimeUnit doc : https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/TimeUnit.html This led me to follow their logic : "toMicros", "toNanos", etc. By looking around the kafka code to see if there was some sort of consistency, ms/millis/milliseconds are present at multiple places but millis seems to be the least present wording. I've modified the KIP accordingly to use the full naming (seconds, milliseconds, ...) as per your suggestion. I've also added a note on precision loss but I'm not sure my phrasing is clear enough. Please let me know, Julien Le ven. 21 janv. 2022 à 13:20, Tom Bentley <tbent...@redhat.com> a écrit : > Hi Julien, > > Thanks! A couple of other points sprang to mind: > > 1. seconds is a unit, but millis etc are really just prefixes. I can > imagine what my old physics teacher would have to say about mixing these > concepts :-). I would have preferred to use the abbreviations s, ms, µs and > ns (ms in particular would be consistent with the unit names used in > configs), but I'm quite sure many people would struggle to know how to type > µ. Maybe 'us' is an acceptable alternative (it's a pretty common > convention), or perhaps we should use 'seconds', 'milliseconds', > 'microseconds' and 'nanoseconds' as the allowed values, even though they're > a bit long. > > 2. I think the truncation and sub-milliseconds behaviour when converting > to/from java.util.Date etc. should be part of the documentation of the > config. > > Apart from these minor points this is looking good to me. > > Kind regards, > > Tom > > On Wed, 19 Jan 2022 at 22:06, Julien Chanaud <chanaud.jul...@gmail.com> > wrote: > > > Hi Tom and thanks for your review, > > > > I agree and I have renamed the field name to unix.precision. > > I've replaced all references to the word "epoch" which I mistakenly used > to > > describe measurements throughout the KIP. > > I have also modified the KIP name as well. > > Before : Add support for unix epoch precision in TimestampConverter SMT > > Now : Add support for different unix precisions in TimestampConverter SMT > > > > Let me know what you think, > > > > Julien > > > > > > Le mer. 19 janv. 2022 à 19:27, Tom Bentley <tbent...@redhat.com> a > écrit : > > > > > Hi Julien, > > > > > > I wonder if the name epoch.precision is a little confusing. An epoch > is a > > > point in time chosen as a origin for a particular calendar system. As > > such > > > it doesn't have a precision. It's only measurements from this point in > > time > > > which have a precision. In the unix case, precisions of seconds, ms, µs > > and > > > ns seem to make the most sense. So I wonder if the name should be > > > unix.precision instead. That also makes it clearer that it only applies > > in > > > the type=unix case. Wdyt? > > > > > > Kind regards, > > > > > > Tom > > > > > > On Thu, 13 Jan 2022 at 13:42, Julien Chanaud <chanaud.jul...@gmail.com > > > > > wrote: > > > > > > > Hi Mickael, > > > > > > > > Thank you very much for your feedback and direction. > > > > > > > > I have added the documentation to the "Public Interfaces" chapter > > > > (formatted in a table as I've seen in other KIPs) and I'll put this > > > > KIP to a vote next week as per your suggestion. > > > > > > > > Regards, > > > > > > > > Julien > > > > > > > > Le mer. 12 janv. 2022 à 18:19, Mickael Maison > > > > <mickael.mai...@gmail.com> a écrit : > > > > > > > > > > Hi Julien, > > > > > > > > > > Thanks for the KIP. I looks like a useful improvement to the > > > > > TimestampConverter SMT. > > > > > > > > > > I'd suggest adding the documentation for the new setting to the > KIP. > > > > > I've had to go check your PR to fully understand how you want to > use > > > > > it, both for input and output. > > > > > Apart from that, if you don't get any further feedback, feel free > to > > > > > start a vote. > > > > > > > > > > Thanks, > > > > > Mickael > > > > > > > > > > On Tue, Dec 21, 2021 at 2:19 PM Julien Chanaud < > > > chanaud.jul...@gmail.com> > > > > wrote: > > > > > > > > > > > > Hi everyone, > > > > > > > > > > > > Bumping this KIP discussion. > > > > > > It's a small change, entirely backward compatible and I'd love > your > > > > > > feedback on it. > > > > > > Thanks, > > > > > > Julien > > > > > > > > > > > > > > > > > > Le jeu. 9 déc. 2021 à 21:56, Julien Chanaud < > > > chanaud.jul...@gmail.com> > > > > a écrit : > > > > > > > > > > > > > > Hi everyone, > > > > > > > > > > > > > > I would like to start a discussion for KIP-808 > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-808%3A+Add+support+for+unix+epoch+precision+in+TimestampConverter+SMT > > > > > > > > > > > > > > This seems like a simple change but I suspect there are several > > > > things to consider, most notably regarding the java.util.Date object, > > > which > > > > is at the heart of the conversions. > > > > > > > > > > > > > > Let me know what you think. > > > > > > > > > > > > > > Julien > > > > > > > > > > > > > > > > > > > > >
