I will remove the connectionId, btw I reworked the EventSubscriber (its checked in) its exactly what you illustrated in your mail. It also gives the subscriber the benefit of registering once rather than multiple calls to register event subscriptions for each event.
On Sun, Mar 23, 2014 at 2:52 PM, kishore g <[email protected]> wrote: > I don't see the need for connectionId. We should have a way to track > connections irrespective of having connection id or not. > > I like the idea of having subscriberId. > > Regarding filters, I think both points are valid. > - We don't want to make changes to EventService api, so pushing it to > subscriber interface allows us to expand subscriber functionality without > breaking compatibility. For instance, we don't have the ability to filter > currently and we can easily support the existing functionality and design > the filter later. > - EventSubscriber must be able to handle different types of events. I think > the subscriber interface must return List<Class<HelixEvent>> instead of > single one. > > I think we need some more time to design the filter api. I will be ok with > not providing the filter functionality right now, we can revisit it after > we get the api package ready. > > thanks, > Kishore G > > > > > > > > > On Sun, Mar 23, 2014 at 2:24 PM, Kanak Biscuitwala <[email protected]>wrote: > >> >> What I was saying is that instead of extending the AbstractSubscriber >> class, I think the same flexibility can be achieved by extending the Filter >> class. The problem with pushing filtering into the subscriber is that you >> have no flexibility on using the same EventSubscriber to handle two >> different types of events. >> >> Kishore, do you have any thoughts about this? >> >> Kanak >> ---------------------------------------- >> > Date: Sun, 23 Mar 2014 14:07:28 -0700 >> > Subject: Re: Helix API: Update for 03/23/2014 >> > From: [email protected] >> > To: [email protected] >> > >> > Hi Kanak, >> > >> > See comments inline >> > >> > On Sun, Mar 23, 2014 at 1:42 PM, Kanak Biscuitwala <[email protected]> >> wrote: >> >> >> >> Hi Sandeep, >> >> >> >> (1) Isn't the Filter class already generic enough to provide this level >> of pluggability? >> > [Sandeep] I did not change the Filter class, I simply made the >> > EventService take a Subscriber for subscription registration and >> > pushed the parameters to be interface methods on the Subscriber. That >> > way if we need subscribers to pass in more parameters during >> > subscription we enhance it through the Subscriber interface, we always >> > ask that they extend the AbstractSubscriber provided to shield >> > themselves from the Subscriber interface changes. >> > >> >> >> >> (2) Sure, I agree with this. >> >> >> >> (3) Looks fine. Do we need a ConnectionId and a SubscriberId? >> > >> > [Sandeep] The SubscriberId is tied to events subscribers so they can >> > unregister their subscriptions simply by passing in the id and not >> > having to hold on to the Filter and all the objects in the subscribe >> > call as it was before my changes. The connection id is used to track >> > connections for HelixConnection, we can do away with it if we think we >> > don't ConnectionId. >> > >> >> >> >> Kanak >> >> ---------------------------------------- >> >>> Date: Sun, 23 Mar 2014 13:15:36 -0700 >> >>> Subject: Helix API: Update for 03/23/2014 >> >>> From: [email protected] >> >>> To: [email protected] >> >>> >> >>> Here are the changes >> >>> >> >>> (1) I reworked the EventManager API to make it a bit more coarse >> >>> grained to allow easier extensions. >> >>> >> >>> (2) I have removed the id packages from model.id, statemachine.id into >> >>> a singular package api.id since some of the other packages had one or >> >>> two classes and did not make sense to have too many packages just for >> >>> ids. >> >>> >> >>> (3) I added a api.client package with interfaces for >> >>> HelixConnectionFactory, HelixConnection, HelixSession as place holders >> >>> which will get fleshed out after discussions. I did not move the >> >>> HelixConnection from core> org.apache.helix as it has too many >> >>> dependencies. I think we need to rework it into API piece by piece >> >>> before we remove it. >> >>> >> >>> Please take a look. I will step away for a bit but will check back in >> soon. >> >>> >> >>> Thanks, >> >>> >> >>> Sandeep >> >> >> >>
