I agree with Guozhang. Breaking compatibility is not acceptable.
If we want the change to use `Optional`, we should deprecate the current method and explain that it return type will change in next major release 3.0.0 and create a ticket for this change that we can tackle when time comes. -Matthias On 8/2/18 9:10 AM, Guozhang Wang wrote: > I think leaving the current return value to be null-able is okay, as long > as it is well documented in java doc. > > > Guozhang > > On Thu, Aug 2, 2018 at 3:13 AM, Damian Guy <damian....@gmail.com> wrote: > >> You have 3 binding votes, so i'll defer to the others. >> >> On Thu, 2 Aug 2018 at 04:41 Nishanth Pradeep <nishanth...@gmail.com> >> wrote: >> >>> The only issue I see with this is that Sink#topic would also need to be >>> Optional as was pointed out already. Since Sink#topic is a preexisting >>> method, changing its return type would break backwards compatibility. >>> >>> On the other hand, it might be worth it to rip that bandaid now. >>> >>> Best, >>> Nishanth Pradeep >>> >>> On Wed, Aug 1, 2018 at 11:56 AM Guozhang Wang <wangg...@gmail.com> >> wrote: >>> >>>> For source node, only one of `Set<topics> topicsSet` and `TopicPattern >>>> topicPattern()` will be specified by the user. Similarly for sink node, >>>> only one of `String` and `TopicNameExtractor` will be specified by the >>>> user. Although I've not seen Nishanth's updated PR, I think when it is >> not >>>> specified today we will return null in that case. >>>> >>>> If we want to improve on this situation with Optional, we'd need to do >> it >>>> on all of these functions. Also note that for `Source#toString()` and >>>> `Sink#toString()` we should only include the specified field in the >>>> resulted representation. >>>> >>>> >>>> Guozhang >>>> >>>> On Wed, Aug 1, 2018 at 5:08 AM, Damian Guy <damian....@gmail.com> >> wrote: >>>> >>>>> Ewen - no as I don't believe they are never null. Whereas the >>>>> topicNameExtractor method returns null if it is the default extractor >> or >>>>> the extractor. So i think this would be better to be optional as it is >>>>> optionally returning a TopicNameExtractor >>>>> >>>>> On Tue, 31 Jul 2018 at 23:01 Ewen Cheslack-Postava <e...@confluent.io >>> >>>>> wrote: >>>>> >>>>>> Generally +1 (binding) >>>>>> >>>>>> It would be helpful to just provide the full, updated interfaces in >>>> the >>>>>> KIP and mark things as new with comments if needed. I had to go back >>>> and >>>>>> read the discussion thread to make sure I was understanding the >> intent >>>>>> correctly. >>>>>> >>>>>> Damian -- if we make that Optional, shouldn't the methods on Source >>>> also >>>>>> be Optional types? >>>>>> >>>>>> -Ewen >>>>>> >>>>>> On Mon, Jul 30, 2018 at 11:13 PM Damian Guy <damian....@gmail.com> >>>>> wrote: >>>>>> >>>>>>> Hi Nishanth, >>>>>>> >>>>>>> I have one nit on the KIP. I think the topicNameExtractor method >>>> should >>>>>>> return Optional<TopicNameExtractor> rather than null. >>>>>>> Sorry I'm late here. >>>>>>> >>>>>>> Thanks, >>>>>>> Damian >>>>>>> >>>>>>> On Tue, 31 Jul 2018 at 01:14 Nishanth Pradeep < >> nishanth...@gmail.com >>>>> >>>>>>> wrote: >>>>>>> >>>>>>>> We need one more binding vote. >>>>>>>> >>>>>>>> Binding Votes: >>>>>>>> >>>>>>>> - Matthias J. Sax >>>>>>>> - Guozhang Wong >>>>>>>> >>>>>>>> Community Votes: >>>>>>>> >>>>>>>> - Bill Bejeck >>>>>>>> - Ted Yu >>>>>>>> >>>>>>>> Best, >>>>>>>> Nishanth Pradeep >>>>>>>> >>>>>>>> On Fri, Jul 27, 2018 at 10:02 AM Bill Bejeck <bbej...@gmail.com> >>>>> wrote: >>>>>>>> >>>>>>>>> Thanks for the KIP! >>>>>>>>> >>>>>>>>> +1 >>>>>>>>> >>>>>>>>> -Bill >>>>>>>>> >>>>>>>>> On Thu, Jul 26, 2018 at 2:39 AM Guozhang Wang < >>>> wangg...@gmail.com> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> +1 >>>>>>>>>> >>>>>>>>>> On Wed, Jul 25, 2018 at 11:13 PM, Matthias J. Sax < >>>>>>>> matth...@confluent.io >>>>>>>>>> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> +1 (binding) >>>>>>>>>>> >>>>>>>>>>> -Matthias >>>>>>>>>>> >>>>>>>>>>> On 7/25/18 7:47 PM, Ted Yu wrote: >>>>>>>>>>>> +1 >>>>>>>>>>>> >>>>>>>>>>>> On Wed, Jul 25, 2018 at 7:24 PM Nishanth Pradeep < >>>>>>>>>> nishanth...@gmail.com> >>>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Hello, >>>>>>>>>>>>> >>>>>>>>>>>>> I'm calling a vote for KIP-321: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- >>>>> 321%3A+Update+ >>>>>>>>>>> TopologyDescription+to+better+represent+Source+and+Sink+ >> Nodes >>>>>>>>>>>>> >>>>>>>>>>>>> Best, >>>>>>>>>>>>> Nishanth Pradeep >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> -- Guozhang >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>>> >>>> >>>> -- >>>> -- Guozhang >>>> >>> >> > > >
signature.asc
Description: OpenPGP digital signature
