Hey, I wanted to re-enable and implement some additional partition merge tests when I noticed, what seems, some unfinished work in merge event notification. I see that Manik added Merged annotation and MergeEvent in 4.2 which are similar to ViewChanged annotation and ViewChangedEvent except that they are exclusively used for merge view notifications. Anyway, JGroups MergeView is detected in JGroupsTransport, listeners are invoked except that Merge notification is not allowed to propagate in CacheManagerNotifierImpl! In addition to this DistributionManagerImpl does not register listener for Merge event notification! Does anyone know what is going on here? Why is this mechanism left unfinished? Regards, Vladimir -- Vladimir Blagojevic JBoss Clustering Team JBoss, by Red Hat _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Good point. It is related to https://jira.jboss.org/browse/ISPN-609 Seem like Manik wanted to introduce separate event for merge scenarios. But why did he not finish full implementation of it? Maybe an oversight. I am tempted to complete it because we can not push 4.2 without this finished... On 2010-10-07, at 9:10 AM, Galder Zamarreño wrote: > At first glance, no idea. Did you check the commit that introduce these classes to see if you could some clues through that? > > On Oct 6, 2010, at 8:00 PM, Vladimir Blagojevic wrote: > >> Hey, >> >> I wanted to re-enable and implement some additional partition merge tests when I noticed, what seems, some unfinished work in merge event notification. I see that Manik added Merged annotation and MergeEvent in 4.2 which are similar to ViewChanged annotation and ViewChangedEvent except that they are exclusively used for merge view notifications. Anyway, JGroups MergeView is detected in JGroupsTransport, listeners are invoked except that Merge notification is not allowed to propagate in CacheManagerNotifierImpl! In addition to this DistributionManagerImpl does not register listener for Merge event notification! >> >> Does anyone know what is going on here? Why is this mechanism left unfinished? >> >> >> Regards, >> Vladimir >> >> -- >> Vladimir Blagojevic >> JBoss Clustering Team >> JBoss, by Red Hat >> >> >> >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > -- > Galder Zamarreño > Sr. Software Engineer > Infinispan, JBoss Cache > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Vladimir Blagojevic JBoss Clustering Team JBoss, by Red Hat _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Ok here is what I have done. All instance of handling ViewChangeEvent now handle MergeEvent as well. I believe this is how it used to be before the ViewChangeEvent and MergeEvent split in 4.2. I made MergeEvent extend ViewChangeEvent, therefore all we had to do is to attach @Merged to already existing listeners. I have verified handling of merge for DistributionManager. I also enabled rehashing test under merge scenario. It now passes. There were no other regressions on test suite. Will you (if you are familiar) have a look at SingletoStore, KeyAffinityService and TransactionTable? The relevant change set is http://fisheye.jboss.org/changelog/Infinispan/?cs=2496 Talk to you guys Tuesday (Long weekend in Canada:)), Vladimir On 2010-10-08, at 10:23 AM, Vladimir Blagojevic wrote: > Wherever we used to listen for ViewChangeEvents we now have to listen for MergeEvents as well, don't we? I've looked around and I believe that there are four internal areas of our codebase that need to adapt to these changes: KeyAffinityService, DistributionManager, SingletonStore and TransactionTable. I'll handle DsitributionManager, and the others, if you are familiar with the codebase please have a look and talk to me privately so we can coordinate further. > > Regards, > Vladimir > > > On 2010-10-07, at 10:47 AM, Vladimir Blagojevic wrote: > >> Good point. It is related to https://jira.jboss.org/browse/ISPN-609 >> Seem like Manik wanted to introduce separate event for merge scenarios. But why did he not finish full implementation of it? Maybe an oversight. I am tempted to complete it because we can not push 4.2 without this finished... >> >> >> >> >> On 2010-10-07, at 9:10 AM, Galder Zamarreño wrote: >> >>> At first glance, no idea. Did you check the commit that introduce these classes to see if you could some clues through that? >>> >>> On Oct 6, 2010, at 8:00 PM, Vladimir Blagojevic wrote: >>> >>>> Hey, >>>> >>>> I wanted to re-enable and implement some additional partition merge tests when I noticed, what seems, some unfinished work in merge event notification. I see that Manik added Merged annotation and MergeEvent in 4.2 which are similar to ViewChanged annotation and ViewChangedEvent except that they are exclusively used for merge view notifications. Anyway, JGroups MergeView is detected in JGroupsTransport, listeners are invoked except that Merge notification is not allowed to propagate in CacheManagerNotifierImpl! In addition to this DistributionManagerImpl does not register listener for Merge event notification! >>>> >>>> Does anyone know what is going on here? Why is this mechanism left unfinished? >>>> >>>> >>>> Regards, >>>> Vladimir >>>> >>>> -- >>>> Vladimir Blagojevic >>>> JBoss Clustering Team >>>> JBoss, by Red Hat >>>> >>>> >>>> >>>> >>>> >>>> _______________________________________________ >>>> infinispan-dev mailing list >>>> infinispan-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> >>> -- >>> Galder Zamarreño >>> Sr. Software Engineer >>> Infinispan, JBoss Cache >>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> -- >> Vladimir Blagojevic >> JBoss Clustering Team >> JBoss, by Red Hat >> >> >> >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > -- > Vladimir Blagojevic > JBoss Clustering Team > JBoss, by Red Hat > > > > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Vladimir Blagojevic JBoss Clustering Team JBoss, by Red Hat _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Hey, No need to register a new listener for merge event. There is no addMergeEventListener, only addListener! Manik's mechanism looks for annotated callback methods in a listener and registers them with event notifiers. So when you do addListener(x) and x has methods annotated with ViewChaged and Merged annotations then x will get registered in appropriate list in CacheManagerNotifierImpl. Very ingenious mechanism. Yes, I made MergeEvent subclass of ViewChangeEvent. It was not before. To all our view change listener classes that had ViewChange annotated method callback I added Merged as well.

This is excellent solution: you can have single method implement both type of events but in case you need separate handling for merge event you are free to do so. Cheers. On 2010-10-11, at 5:30 AM, Mircea Markus wrote: > Well spotted! > I took a look at other listeners, looks good. > Right now, when a merge happens are the ViewChange listeners going to be notified or do we have to explicitly register for both events? IMO MergeEvent is an ViewChange, and all the listeners that registered for ViewChange should be notified whenever a merge takes place. wdyt? > Enjoy the long weekend! > > Cheers, > Mircea > > On 8 Oct 2010, at 22:16, Vladimir Blagojevic wrote: > >> Ok here is what I have done. All instance of handling ViewChangeEvent now handle MergeEvent as well. I believe this is how it used to be before the ViewChangeEvent and MergeEvent split in 4.2. I made MergeEvent extend ViewChangeEvent, therefore all we had to do is to attach @Merged to already existing listeners. I have verified handling of merge for DistributionManager. I also enabled rehashing test under merge scenario. It now passes. There were no other regressions on test suite. >> >> Will you (if you are familiar) have a look at SingletoStore, KeyAffinityService and TransactionTable? >> The relevant change set is http://fisheye.jboss.org/changelog/Infinispan/?cs=2496 >> >> Talk to you guys Tuesday (Long weekend in Canada:)), >> Vladimir >> >> >> >> On 2010-10-08, at 10:23 AM, Vladimir Blagojevic wrote: >> >>> Wherever we used to listen for ViewChangeEvents we now have to listen for MergeEvents as well, don't we? I've looked around and I believe that there are four internal areas of our codebase that need to adapt to these changes: KeyAffinityService, DistributionManager, SingletonStore and TransactionTable. I'll handle DsitributionManager, and the others, if you are familiar with the codebase please have a look and talk to me privately so we can coordinate further. >>> >>> Regards, >>> Vladimir >>> >>> >>> On 2010-10-07, at 10:47 AM, Vladimir Blagojevic wrote: >>> >>>> Good point. It is related to https://jira.jboss.org/browse/ISPN-609 >>>> Seem like Manik wanted to introduce separate event for merge scenarios. But why did he not finish full implementation of it? Maybe an oversight. I am tempted to complete it because we can not push 4.2 without this finished... >>>> >>>> >>>> >>>> >>>> On 2010-10-07, at 9:10 AM, Galder Zamarreño wrote: >>>> >>>>> At first glance, no idea. Did you check the commit that introduce these classes to see if you could some clues through that? >>>>> >>>>> On Oct 6, 2010, at 8:00 PM, Vladimir Blagojevic wrote: >>>>> >>>>>> Hey, >>>>>> >>>>>> I wanted to re-enable and implement some additional partition merge tests when I noticed, what seems, some unfinished work in merge event notification. I see that Manik added Merged annotation and MergeEvent in 4.2 which are similar to ViewChanged annotation and ViewChangedEvent except that they are exclusively used for merge view notifications. Anyway, JGroups MergeView is detected in JGroupsTransport, listeners are invoked except that Merge notification is not allowed to propagate in CacheManagerNotifierImpl! In addition to this DistributionManagerImpl does not register listener for Merge event notification! >>>>>> >>>>>> Does anyone know what is going on here? Why is this mechanism left unfinished? >>>>>> >>>>>> >>>>>> Regards, >>>>>> Vladimir >>>>>> >>>>>> -- >>>>>> Vladimir Blagojevic >>>>>> JBoss Clustering Team >>>>>> JBoss, by Red Hat >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> infinispan-dev mailing list >>>>>> infinispan-dev(a)lists.jboss.org >>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>>> >>>>> -- >>>>> Galder Zamarreño >>>>> Sr. Software Engineer >>>>> Infinispan, JBoss Cache >>>>> >>>>> >>>>> _______________________________________________ >>>>> infinispan-dev mailing list >>>>> infinispan-dev(a)lists.jboss.org >>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>> >>>> -- >>>> Vladimir Blagojevic >>>> JBoss Clustering Team >>>> JBoss, by Red Hat >>>> >>>> >>>> >>>> >>>> >>>> _______________________________________________ >>>> infinispan-dev mailing list >>>> infinispan-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> >>> -- >>> Vladimir Blagojevic >>> JBoss Clustering Team >>> JBoss, by Red Hat >>> >>> >>> >>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> -- >> Vladimir Blagojevic >> JBoss Clustering Team >> JBoss, by Red Hat >> >> >> >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Vladimir Blagojevic JBoss Clustering Team JBoss, by Red Hat _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 2010-10-11, at 1:22 PM, Mircea Markus wrote: > > On 11 Oct 2010, at 18:12, Vladimir Blagojevic wrote: > >> Hey, >> >> No need to register a new listener for merge event. There is no addMergeEventListener, only addListener! Manik's mechanism looks for annotated callback methods in a listener and registers them with event notifiers. So when you do addListener(x) and x has methods annotated with ViewChaged and Merged annotations then x will get registered in appropriate list in CacheManagerNotifierImpl. Very ingenious mechanism. >> >> Yes, I made MergeEvent subclass of ViewChangeEvent. It was not before. To all our view change listener classes that had ViewChange annotated method callback I added Merged as well. > I see! My question is: if a cluster merge happens, and I have a method annotated with ViewChange *only* (i.e. doesn't have @Merged) - will the @ViewChange annotated method be called? IMO it should because merge is only a special kind of view change. Aha, I see :) It will not get called! The root cause is split of view notification in JGroups transport! We have to make this clear for 4.2 release!

Vladimir _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Yes, both types of notifications should be emitted. I'll take a look at this today. Reopening ISPN-609. On 11 Oct 2010, at 20:42, Mircea Markus wrote: > > On 11 Oct 2010, at 20:37, Vladimir Blagojevic wrote: > >> >> On 2010-10-11, at 1:22 PM, Mircea Markus wrote: >> >>> >>> On 11 Oct 2010, at 18:12, Vladimir Blagojevic wrote: >>> >>>> Hey, >>>> >>>> No need to register a new listener for merge event. There is no addMergeEventListener, only addListener! Manik's mechanism looks for annotated callback methods in a listener and registers them with event notifiers. So when you do addListener(x) and x has methods annotated with ViewChaged and Merged annotations then x will get registered in appropriate list in CacheManagerNotifierImpl. Very ingenious mechanism. >>>> >>>> Yes, I made MergeEvent subclass of ViewChangeEvent. It was not before. To all our view change listener classes that had ViewChange annotated method callback I added Merged as well. >>> I see! My question is: if a cluster merge happens, and I have a method annotated with ViewChange *only* (i.e. doesn't have @Merged) - will the @ViewChange annotated method be called? IMO it should because merge is only a special kind of view change. >> >> Aha, I see :) It will not get called! The root cause is split of view notification in JGroups transport! We have to make this clear for 4.2 release! > That's my point :) I think it should be called as merge is a particular type of view change (the view literally changes) >> >> Vladimir >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Manik Surtani manik(a)jboss.org Lead, Infinispan Lead, JBoss Cache http://www.infinispan.org http://www.jbosscache.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 25 Oct 2010, at 15:17, Vladimir Blagojevic wrote: > I think this should already be fine. I took a liberty of concluding that there was a small oversight in completion of ISPN-609 and I completed it. Please verify. How did you complete it? :)

Hmm. I might revert that change, since it isn't just our code that is affected. Any public code may be interested in a @ViewChanged callback during a merge as well. On 25 Oct 2010, at 15:27, Vladimir Blagojevic wrote: > http://fisheye.jboss.org/changelog/Infinispan/?cs=2496 > > I made a MergeEvent subclass of ViewChangeEvent. In our code base all current callbacks annotated with @ViewChanged I annotated with @Merged. And then I enabled firing of MergeEvent in CacheManagerNotifierImpl. > > > On 2010-10-25, at 10:20 AM, Manik Surtani wrote: > >> >> On 25 Oct 2010, at 15:17, Vladimir Blagojevic wrote: >> >>> I think this should already be fine. I took a liberty of concluding that there was a small oversight in completion of ISPN-609 and I completed it. Please verify. >> >> How did you complete it? :) > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Manik Surtani manik(a)jboss.org Lead, Infinispan Lead, JBoss Cache http://www.infinispan.org http://www.jbosscache.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 25 Oct 2010, at 15:58, Vladimir Blagojevic wrote: > What do you mean by "may be interested in a @ViewChanged callback during a merge as well"? Removing @Merged altogether? No, just that a merge event as detected from JGroups should trigger both view changed as well as merged notifications. +1 to most of your changes in the changeset though, including the MergeEvent extends ViewChangedEvent. I'm just removing the unnecessary @Merged annotations in our code since they wil be triggered by the @ViewChanged annotation anyway.