Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Thanks Nishanth, I've taken a look at the updated KIP and it looks good to me. I think you can start a new VOTE thread on the current proposal. Guozhang On Tue, Jul 24, 2018 at 5:56 PM, Nishanth Pradeep wrote: > I have updated the KIP >

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

I have updated the KIP . Changes to the KIP: - Removed topics() from the Public Interface and Proposed Changes sections. - Added topics() to the

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

We should not remove it immediately in the up coming 2.1 release. Usually we first mark an API as deprecated, and consider removing it only after it has been deprecated for at least one major release period. Guozhang On Mon, Jul 23, 2018 at 7:40 PM, Nishanth Pradeep wrote: > Sounds good to me

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Sounds good to me too. As far as deprecating goes -- should the topics() method removed completely or should it have a @deprecated annotation for removal in some future version? Best, Nishanth Pradeep On Sun, Jul 22, 2018 at 1:32 PM Matthias J. Sax wrote: > Works for me. > > On 7/22/18 9:48

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Works for me. On 7/22/18 9:48 AM, Guozhang Wang wrote: > I think I can be convinced with deprecating topics() to keep API minimal. > > About renaming the others with `XXNames()`: well, to me it feels still not > very worthy since although it is not a big burden, it seems also not a big >

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

I think I can be convinced with deprecating topics() to keep API minimal. About renaming the others with `XXNames()`: well, to me it feels still not very worthy since although it is not a big burden, it seems also not a big "return" if we name the newly added function `topicSet()`. Guozhang

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

I definitely agree with you on deprecating topics(). I also think changing the method names for consistency is reasonable, since there is no functionality change. Although, I can be convinced either way on this one. Best, Nishanth Pradeep On Fri, Jul 20, 2018 at 12:15 PM Matthias J. Sax wrote:

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

I would still deprecate existing `topics()` method. If users need a String, they can call `topicSet().toString()`. It's just a personal preference, because I believe it's good to keep the API "minimal". About renaming the other methods: I thinks it's a very small burden to deprecate the existing

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Understood, Guozhang. Thanks for the help, everyone! I have updated the KIP. Let me know if you any other thoughts or suggestions. Best, Nishanth Pradeep On Thu, Jul 19, 2018 at 7:33 PM Guozhang Wang wrote: > I see. > > Well, I think if we add a new function like topicSet() it is less needed

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

I see. Well, I think if we add a new function like topicSet() it is less needed to deprecate topics() as it returns "{topic1, topic2, ..}" which is sort of non-overlapping in usage with the new API. Guozhang On Thu, Jul 19, 2018 at 5:31 PM, Nishanth Pradeep wrote: > That is what I meant. I

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

That is what I meant. I will add topicSet() instead of changing the signature of topics() for compatibility reasons. But should we not add a @deprecated flag for topics() or do you want to keep it around for the long run? On Thu, Jul 19, 2018 at 7:27 PM Guozhang Wang wrote: > We cannot change

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

We cannot change the signature of the function named "topics" from "String" to "Set", as Matthias mentioned it is a compatibility breaking change. That's why I was proposing add a new function like "Set topicSet()", while keeping "String topics()" as is. Guozhang On Thu, Jul 19, 2018 at 5:22

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Right, adding topicNames() instead of changing the return type of topics() in order preserve backwards compatibility is a good idea. But is it not better to depreciate topics() because it would be redundant? In our case, it would only be calling topicNames/topicSet#toString(). I still agree that

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Personally I'd prefer to keep the deprecation-related changes as small as possible unless they are really necessary, and hence I'd prefer to just add List topicList() /* or Set topicSet() */ in addition to topicPattern to Source, in addition to `topicNameExtractor` to Sink, and leaving the

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Thanks for updating the KIP. The current `Source` interface has a method `String topics()` atm. Thus, we cannot just add `Set Source#topics()` because this would replace the existing method and would be an incompatible change. I think, we should deprecate `String topics()` and add a method with

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

I have revised the KIP . Here is a summary of the changes. 1. Changed return type from String to Set for Source#topics(). Set Source#topics() 2. Added

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Hi Nishanth, Since even combining these two the scope is still relatively small I'd suggest just do it in one KIP if you're willing to work on them. If you do not then pleas feel free to create the JIRA for the second step so that others can pick it up. Guozhang On Sun, Jul 15, 2018 at 6:14

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

There is no general protocol. We can include the changes in the current KIP or do a second KIP. If you don't want to include the change in this KIP, please create a new JIRA to track the other changes. You can assign the JIRA to yourself and start a second KIP if you want. You can also "link"

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Thank you for the comments! I agree with these changes. So is the general protocol to update the same KIP, or is to scrap the current KIP and create a new one since it's beyond the scope of the original KIP? I am happy to do either. On Wed, Jul 4, 2018 at 1:48 PM Matthias J. Sax wrote: >

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Sounds good to me. -Matthias On 7/4/18 10:53 AM, Guozhang Wang wrote: > After looked through the current TopologyDescription I think I'd want to > combine the suggestions from John and Matthias on the API proposal. The > motivations is that we have two relatively different functionalities >

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

After looked through the current TopologyDescription I think I'd want to combine the suggestions from John and Matthias on the API proposal. The motivations is that we have two relatively different functionalities provided from the APIs today: 1. Each interface's public functions, like

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

I just double checked the discussion thread of KIP-120 that introduced `TopologyDescription`. Back than the argument was, that using the simplest option might be sufficient because the description is mostly used for debugging. Not sure if this argument holds. It seem that people built first more

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

John, I am a little bit on the fence. In retrospective, it might have been better to add `topic()` and `topicPattern()` to source node and return a proper `Pattern` object instead of the pattern as a String. All other "payload" is just names and thus String naturally. From my point of view

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Sorry for the late comment, Looking at the other pieces of TopologyDescription, I noticed that pretty much all of the "payload" of these description nodes are strings. Should we consider returning a string from `topicNameExtractor()` instead? In fact, if we did that, we could consider calling

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

My previous response was talking about the new method in InternalTopologyBuilder. The exception just means there is no uniform extractor for all the sinks. On Mon, Jun 25, 2018 at 8:02 PM, Matthias J. Sax wrote: > Ted, > > Why? Each sink can have a different TopicNameExtractor. > > > -Matthias

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Ted, Why? Each sink can have a different TopicNameExtractor. -Matthias On 6/25/18 5:19 PM, Ted Yu wrote: > If there are different TopicNameExtractor classes from multiple sink nodes, > the new method should throw exception alerting user of such scenario. > > > On Mon, Jun 25, 2018 at 2:23

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

If there are different TopicNameExtractor classes from multiple sink nodes, the new method should throw exception alerting user of such scenario. On Mon, Jun 25, 2018 at 2:23 PM, Bill Bejeck wrote: > Thanks for the KIP! > > Overall I'm +1 on the KIP. I have one question. > > The KIP states

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Good catch. I think the proposed change is to add that function in InternalTopologyBuilder#Sink class. Guozhang On Mon, Jun 25, 2018 at 2:23 PM, Bill Bejeck wrote: > Thanks for the KIP! > > Overall I'm +1 on the KIP. I have one question. > > The KIP states that the method

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Thanks for the KIP! Overall I'm +1 on the KIP. I have one question. The KIP states that the method "topicNameExtractor()" is added to the InternalTopologyBuilder.java. It could be that I'm missing something, but wow does this work if a user has provided different TopicNameExtractor instances

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Yup I agree, generally speaking the `toString()` output is not recommended to be relied on programmatically in user's code, but we've observed convenience-beats-any-other-reasons again and again in development unfortunately. I think we should still not claiming it is part of the public APIs that

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Thanks for the KIP! I am don't have any further comments. For Guozhang's comment: if we mention anything about `toString()`, we should make explicit that `toString()` output is still not public contract and users should not rely on the output. Furhtermore, for the actual uses output, I would

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Thanks for writing the KIP! I'm +1 on the proposed changes over all. One minor suggestion: we should also mention that the `Sink#toString` will also be updated, in a way that if `topic()` returns null, use the other call, etc. This is because although we do not explicitly state the following

[Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

Hello Everyone, I have created a new KIP to discuss extending TopologyDescription. You can find it here: https://cwiki.apache.org/confluence/display/KAFKA/KIP-321%3A+Add+method+to+get+TopicNameExtractor+in+TopologyDescription Please provide any feedback that you might have. Best, Nishanth