Thanks for the KIP, Rabi. Overall LGTM, but I think we can simplify the write up a little bit.
Note that `ExtractRecordMetadataTimestamp` is not a public class (and I am actually wondering why `ExtractRecordMetadataTimestamp#onInvalidTimestamp` is actually public? Seems it could be protected or even package private) and hence we don't need to mention it. Anyway, the KIP can focus on public APIs and just state that the class `UsePreviousTimeOnInvalidTimestamp` will be deprecated and a new class `UsePartitionTimeOnInvalidTimestamp` will be added with the same functionality to replace the existing class. No need to talk about the test classed or JavaDocs. Compare https://cwiki.apache.org/confluence/display/KAFKA/KIP-528%3A+Deprecate+PartitionGrouper+configuration+and+interface that deprecated a public interface and config parameter. It's short, crisp, and to the point. As actually also think, that you can start a VOTE thread in parallel. -Matthias On 10/7/19 8:27 AM, Bruno Cadonna wrote: > Hi Rabi, > > no need to include images for the code. There is a code block feature > in the wiki. I added one of those code blocks in your KIP as an > example. > > Best, > Bruno > > On Mon, Oct 7, 2019 at 4:55 PM Rabi Kumar K C <ravow...@gmail.com> wrote: >> >> Hi Bruno, >> >> Ha I see what you were talking about the extract method in >> UsePreviousTimeInvalidTimeStamp. Please ignore my last mail and I will >> update the KIP accordingly. >> >> With Best Regards, >> Rabi Kumar K C >> >> Sent from Mail for Windows 10 >> >> From: Rabi Kumar K C >> Sent: Monday, October 7, 2019 4:50 PM >> To: dev@kafka.apache.org >> Subject: RE: [DISCUSS] KIP-530: >> Considerrenaming'UsePreviousTimeOnInvalidTimeStamp' >> classto'UsePartitionTimeOnInvalidTimeStamp' >> >> Hi Bruno, >> >> Thank You for your suggestions. I have made necessary changes in KIP and >> hopefully it’s fine now and if not then please do let me know. >> >> To answer your question 4) >> right now in trunk we can see that extract method is not present in >> UsePreviousTimeOnInvalidTimestamp instead it implements onInvalidTimestamp >> which is abstract method of super class ExtractRecordMetadataTimestamp. I >> have only seen extract() method in ExtractRecordMetadataTimestamp. Please do >> correct me if I am wrong. >> >> And yes I do agree with you on 5) the deprecation part for compatibility, >> deprecation and migration plan >> >> >> With Best Regards, >> Rabi Kumar K C >> Sent from Mail for Windows 10 >> >> From: Bruno Cadonna >> Sent: Monday, October 7, 2019 3:47 PM >> To: dev@kafka.apache.org >> Subject: Re: [DISCUSS] KIP-530: Consider >> renaming'UsePreviousTimeOnInvalidTimeStamp' class >> to'UsePartitionTimeOnInvalidTimeStamp' >> >> Hi Rabi, >> >> Thank you for the KIP! >> >> 1.) Could you please improve the formatting of the KIP? For instance, >> use appropriate formatting for code to differentiate it from the text. >> Also, we usually do not use italics to write KIPs. Look at other KIPs >> to get an idea of the formatting. >> >> 2.) "Public Interfaces" does not directly refer to interfaces in Java. >> It rather refers to the APIs that are visible from the outside. Thus, >> you should specify the class `UsePartitionOnInvalidTimeStamp` with its >> method signatures but without implementation. >> >> 3.) Under "Public Interfaces", you should also mention whether ` >> UsePreviousTimeOnInvalidTimestamp` should be deprecated or not. >> >> 4.) What do you mean with "now extract has been removed from >> 'UsePreviousTimeOnInvalidTimestamp'"? Without `extract()`, >> `UsePreviousTimeOnInvalidTimestamp` would not implement the >> `TimestampExtractor` interface anymore. >> >> 5.) Regarding "Compatibility, Deprecation, and Migration Plan", I do >> not think that we can simply remove >> `UsePreviousTimeOnInvalidTimestamp` in the next minor release. It >> needs to be deprecated beforehand. >> >> Best, >> Bruno >> >> On Wed, Oct 2, 2019 at 4:49 PM RABI K.C. <ravow...@gmail.com> wrote: >>> >>> Hello All, >>> >>> This is KIP for the change of Class name from >>> UsePreviousTimeOnInvalidTimeStamp to UsePartitionTimeOnInvalidTimeStamp. >>> Link and Jira ticket is mentioned below: >>> >>> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=130028807 >>> https://issues.apache.org/jira/browse/KAFKA-8953 >>> >>> Would be pleased to get your feedback on this. >>> >>> With Best Regards, >>> Rabi Kumar K C >> >>
signature.asc
Description: OpenPGP digital signature
