Hi Matthias, Thank You for the review of KIP and I have made the changes in it as per your suggestion. Will be moving this KIP to vote now.
With Regards, Rabi Kumar K C On Wed, Oct 23, 2019 at 7:56 AM Matthias J. Sax <matth...@confluent.io> wrote: > 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 > >> > >> > >
