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