Unregister topic listener on region shutdown

[EzequielB]

Fix for #163

• Signed Contributor Agreement

[devOpsHazelcast]

All committers have signed the CLA.

[devOpsHazelcast]

Can one of the admins verify this patch?

[pivovarit]

Thanks! Looks solid.

[enozcan]

Thanks for the contribution! I want to make some points clear.

When I run the test

/* LocalRegionFactorySlowTest.java */

@Test
public void cleanUpRegisteredTopicOnRegionShutdown() {
    HazelcastInstance sf1Instance = HazelcastAccessor.getHazelcastInstance(sf);
    List<String> regions = new ArrayList<>(Arrays.asList(sf.getStatistics().getSecondLevelCacheRegionNames()));    
    // 'Query' cache is not subscribed to a topic in 5 and 52. 
    regions.removeIf(n -> n.contains("Query")); 
    regions.forEach(r ->
            assertEquals(r,2, Accessors.getNodeEngineImpl(sf1Instance)
                    .getEventService().getRegistrations(TopicService.SERVICE_NAME, r).size()));

    sf2.close();
    regions.forEach(r ->
            assertEquals(r,1, Accessors.getNodeEngineImpl(sf1Instance)
                    .getEventService().getRegistrations(TopicService.SERVICE_NAME, r).size()));
}

I get

hazelcast-hibernate5:  com.hazelcast.hibernate.entity.AnnotatedEntity##NaturalId expected:<1> but was:<2>
hazelcast-hibernate52:  com.hazelcast.hibernate.entity.AnnotatedEntity##NaturalId expected:<1> but was:<2>
hazelcast-hibernate53:  default-update-timestamps-region expected:<1> but was:<2>

As I added some logs at the points where a listener is registered to and removed from the topic, I noticed NaturalId region listener is not removed in 5 and 52 but 53. Also timestamps region listener is not removed in 53.

Any idea about this behavior?

[EzequielB]

@enozcan I checked and it seems Hibernate is not calling the Region.destroy method for all regions (StorageAccess.release method in case of version 5.3)
To prevent regions not being destroyed by Hibernate, I added the logic for calling the destroy method of LocalRegion instances into the CleanupService.
Also I had to change the order on the Factory since otherwise the Hz instance would be shutdown first before any unsubscription can happen.

Outdated

[pivovarit]

Wouldn't it be easier to actually store LocalRegionCache instances and then iterate over them and call destroy() on each?

[EzequielB]

Yes, it would be simpler. Also, it would increase the coupling for future changes, the idea was to just use the needed parts and hide the rest inside the CleanupService.
We can do it that other way too.

[pivovarit]

I don't think that that would result in extra coupling. Keep in mind that the registerCache() method accepts LocalRegionCache objects and the rest is encapsulated within the CleanupService class and doesn't really leak out in any way.

So I'd suggest we just store them as List<LocalRegionCache>

[pivovarit]

Any particular reason why LinkedList is better over ArrayList here?

[EzequielB]

Since is was just add at the end, one iteration on stop and an unknown size, I thought it would perform better. But doing some review, the cases an ArrayList would underperform are too edgy, so we should go with an ArrayList. I'll move to that.

[pivovarit]

Comments already provided

[EzequielB]

@pivovarit , changes committed
Let me know how that goes

[pivovarit]

Just one last thing and I think we're good to go

[EzequielB]

Done

[pivovarit]

Looks good to me. Let's wait for @enozcan to have a final look