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 > >> > > > -- -- Guozhang
