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 >
