Hi Bruno, Thanks for your feedback. I think it makes sense to add the hasCode() and equals() definitions with its guarantees on the interface level.
I'm also inclined to add the ThreadMetadata conversion to an interface into this KIP. Implementation-wise is not that different. And on API level modification is at a similar complexity as the current proposed change. I'll shortly update the KIP. Best, On Tue, Jun 1, 2021 at 3:09 PM Bruno Cadonna <cado...@apache.org> wrote: > Hi Josep, > > Thank you for the KIP! > > Specifying TaskMetadata as an interface lifts the guarantee that > equals() and hashCode() on the TaskMetadata return consistent values for > the same content. Note that the current TaskMetadata implements both > methods, so theoretically users might already rely on them. > > The List interface in Java defines its equals()[1] and hashCode()[2] > methods specifically and all correct implementations must adhere to that > definition. > > Maybe it makes sense to add such a guarantee to the TaskMetadata > interface on which users can rely similarly to the Java's List interface. > > Regarding your question about ThreadMetadata, I had the same thought. I > would add it to the KIP unless the extended KIP risks not to be accepted > before KIP freeze for 3.0. > > Best, > Bruno > > > [1] > > https://docs.oracle.com/javase/8/docs/api/java/util/List.html#equals-java.lang.Object- > [2] > https://docs.oracle.com/javase/8/docs/api/java/util/List.html#hashCode-- > > > > > On 31.05.21 12:36, Josep Prat wrote: > > I'm starting to wonder if we should make ThreadMetadata follow the same > > path as TaskMetadata and be converted to an interface + internal > > implementation. What are your thoughts? > > > > > > Thanks in advance, > > Josep > > > > > > > > On Mon, May 31, 2021 at 12:10 PM Josep Prat <josep.p...@aiven.io> wrote: > > > >> Hi there, > >> While looking closely at the classes affected by this change, I realized > >> that ThreadMetadata has a reference in its public API to the soon to be > >> deprecated TaskMetadata. For this reason, I updated the KIP to reflect > that > >> 2 more changes are needed for this KIP: > >> ThreadMetadata: > >> - Deprecate activeTasks() and standbyTasks() > >> - Add getActiveTasks() and getStandbyTasks() that return the new > Interface > >> instead. > >> > >> I also wrote it on the KIP, but this thing crossed my mind, do we need > to > >> keep source compatibility for this particular change? > >> > >> > >> I would be really grateful if you could provide any feedback on the KIP > ( > >> https://cwiki.apache.org/confluence/x/XIrOCg) > >> > >> Thanks in advance, > >> > >> On Fri, May 28, 2021 at 10:24 AM Josep Prat <josep.p...@aiven.io> > wrote: > >> > >>> Hi there, > >>> I updated the KIP page with Sophie's feedback. As she already > mentioned, > >>> the intention would be to include this KIP in release 3.0.0 so we can > avoid > >>> a deprecation cycle for the getTaskID method introduced in KIP-740, I > hope > >>> I managed to capture this in the KIP description. > >>> > >>> Just adding the link again for convenience: > >>> https://cwiki.apache.org/confluence/x/XIrOCg > >>> > >>> Thanks in advance, > >>> > >>> On Thu, May 27, 2021 at 10:08 PM Josep Prat <josep.p...@aiven.io> > wrote: > >>> > >>>> Hi Sophie, > >>>> > >>>> Thanks for the feedback, I'll update the KIP tomorrow with your > >>>> feedback. They are all good points, and you are right, my phrasing > could be > >>>> misleading. > >>>> > >>>> > >>>> Best, > >>>> > >>>> On Thu, May 27, 2021 at 10:02 PM Sophie Blee-Goldman > >>>> <sop...@confluent.io.invalid> wrote: > >>>> > >>>>> Thanks for the KIP! I'm on board with the overall proposal, just a > few > >>>>> comments: > >>>>> > >>>>> 1) The motivation section says > >>>>> > >>>>> TaskMetadata should have never been a class available for the general > >>>>>> public, but more of an internal class > >>>>> > >>>>> > >>>>> which is a bit misleading as it seems to imply that TaskMetadata > itself > >>>>> was > >>>>> never meant to be part of the public API > >>>>> at all. It might be better to phrase this as "TaskMetadata was never > >>>>> intended to be a public class that a user might > >>>>> need to instantiate, but rather an API for exposing metadata which is > >>>>> better served as an interface" --- or something > >>>>> to that effect. > >>>>> > >>>>> 2) You touch on this in a later section, but it would be good to call > >>>>> out > >>>>> directly in the *Public Interfaces* section that > >>>>> you are proposing to remove the `public TaskId getTaskId()` method > that > >>>>> we > >>>>> added in KIP-740. Also I just want to > >>>>> note that to do so will require getting this KIP into 3.0, otherwise > >>>>> we'll > >>>>> need to go through a deprecation cycle for > >>>>> that API. I don't anticipate this being a problem as KIP freeze is > still > >>>>> two weeks away, but it would be good to clarify. > >>>>> > >>>>> 3) nit: we should put the new internal implementation class under > >>>>> the org.apache.kafka.streams.processor.internals > >>>>> package instead of under org.apache.kafka.streams.internals. But this > >>>>> is an > >>>>> implementation detail and as such > >>>>> doesn't need to be covered by the KIP in the first place. > >>>>> > >>>>> - Sophie > >>>>> > >>>>> On Thu, May 27, 2021 at 1:55 AM Josep Prat > <josep.p...@aiven.io.invalid > >>>>>> > >>>>> wrote: > >>>>> > >>>>>> I deliberately picked the most conservative approach of creating a > new > >>>>>> Interface, instead of transforming the current class into an > >>>>> interface. > >>>>>> Feedback is most welcome! > >>>>>> > >>>>>> Best, > >>>>>> > >>>>>> On Thu, May 27, 2021 at 10:26 AM Josep Prat <josep.p...@aiven.io> > >>>>> wrote: > >>>>>> > >>>>>>> Hi there, > >>>>>>> I would like to propose KIP-744, to introduce TaskMetadata as an > >>>>>>> interface, to keep the its implementation as internal use. > >>>>>>> This KIP can be seen as a spin-off of KIP-740. > >>>>>>> > >>>>>>> https://cwiki.apache.org/confluence/x/XIrOCg > >>>>>>> > >>>>>>> Best, > >>>>>>> -- > >>>>>>> > >>>>>>> Josep Prat > >>>>>>> > >>>>>>> *Aiven Deutschland GmbH* > >>>>>>> > >>>>>>> Immanuelkirchstraße 26, 10405 Berlin > >>>>>>> > >>>>>>> Amtsgericht Charlottenburg, HRB 209739 B > >>>>>>> > >>>>>>> Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen > >>>>>>> > >>>>>>> *m:* +491715557497 > >>>>>>> > >>>>>>> *w:* aiven.io > >>>>>>> > >>>>>>> *e:* josep.p...@aiven.io > >>>>>>> > >>>>>> > >>>>>> > >>>>>> -- > >>>>>> > >>>>>> Josep Prat > >>>>>> > >>>>>> *Aiven Deutschland GmbH* > >>>>>> > >>>>>> Immanuelkirchstraße 26, 10405 Berlin > >>>>>> > >>>>>> Amtsgericht Charlottenburg, HRB 209739 B > >>>>>> > >>>>>> Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen > >>>>>> > >>>>>> *m:* +491715557497 > >>>>>> > >>>>>> *w:* aiven.io > >>>>>> > >>>>>> *e:* josep.p...@aiven.io > >>>>>> > >>>>> > >>>> > >>>> > >>>> -- > >>>> > >>>> Josep Prat > >>>> > >>>> *Aiven Deutschland GmbH* > >>>> > >>>> Immanuelkirchstraße 26, 10405 Berlin > >>>> > >>>> Amtsgericht Charlottenburg, HRB 209739 B > >>>> > >>>> Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen > >>>> > >>>> *m:* +491715557497 > >>>> > >>>> *w:* aiven.io > >>>> > >>>> *e:* josep.p...@aiven.io > >>>> > >>> > >>> > >>> -- > >>> > >>> Josep Prat > >>> > >>> *Aiven Deutschland GmbH* > >>> > >>> Immanuelkirchstraße 26, 10405 Berlin > >>> > >>> Amtsgericht Charlottenburg, HRB 209739 B > >>> > >>> Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen > >>> > >>> *m:* +491715557497 > >>> > >>> *w:* aiven.io > >>> > >>> *e:* josep.p...@aiven.io > >>> > >> > >> > >> -- > >> > >> Josep Prat > >> > >> *Aiven Deutschland GmbH* > >> > >> Immanuelkirchstraße 26, 10405 Berlin > >> > >> Amtsgericht Charlottenburg, HRB 209739 B > >> > >> Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen > >> > >> *m:* +491715557497 > >> > >> *w:* aiven.io > >> > >> *e:* josep.p...@aiven.io > >> > > > > > -- Josep Prat *Aiven Deutschland GmbH* Immanuelkirchstraße 26, 10405 Berlin Amtsgericht Charlottenburg, HRB 209739 B Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen *m:* +491715557497 *w:* aiven.io *e:* josep.p...@aiven.io
