You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Rabi Kumar K C <ra...@gmail.com> on 2019/10/07 14:50:26 UTC
RE: [DISCUSS] KIP-530: Consider
renaming'UsePreviousTimeOnInvalidTimeStamp' class
to'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. <ra...@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
Re: [DISCUSS] KIP-530: Considerrenaming'UsePreviousTimeOnInvalidTimeStamp'
classto'UsePartitionTimeOnInvalidTimeStamp'
Posted by "RABI K.C." <ra...@gmail.com>.
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 <ma...@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 <ra...@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. <ra...@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
> >>
> >>
>
>
Re: [DISCUSS] KIP-530:
Considerrenaming'UsePreviousTimeOnInvalidTimeStamp'
classto'UsePartitionTimeOnInvalidTimeStamp'
Posted by "Matthias J. Sax" <ma...@confluent.io>.
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 <ra...@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. <ra...@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
>>
>>
Re: [DISCUSS] KIP-530: Considerrenaming'UsePreviousTimeOnInvalidTimeStamp'
classto'UsePartitionTimeOnInvalidTimeStamp'
Posted by Bruno Cadonna <br...@confluent.io>.
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 <ra...@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. <ra...@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
>
>
RE: [DISCUSS] KIP-530:
Considerrenaming'UsePreviousTimeOnInvalidTimeStamp'
classto'UsePartitionTimeOnInvalidTimeStamp'
Posted by Rabi Kumar K C <ra...@gmail.com>.
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. <ra...@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