Thread safe subscription manager

[pavolloffay]

Make following classes thread safe:

• InMemorySubscriptionManager
• InventoryCDIStorage

[pavolloffay]

@Jiri-Kremser could you please re-review and merge if it is ok?

Assumptions:

• subscribe is not entirely synchronized, only critical section to avoid loss of subscription for tenant with no previous subscriptions. Getting historical metrics can take some time so it would block the method for "long" time.
• subscribe(sub1); subscription(tenant, metricId); unsubscribe(sub1); can cause SubscriptionNotFoundException (getting historical metrics in subscribe can take some time). The same behavior for isSubscribed , subscription and subscriptionOfTenant. is this ok? I do not want to synchronize entire methods for accessing subscriptions as they are called very frequently (each incoming metrics)
• unsubscribe is not synchronized at all.

[jkremser]

..only critical section to avoid loss..

I'd argue, the critical section isn't only the read part of the subscribe() method, however, you are using the concurrent hash maps (nested), so no big deal.. actually, what can go wrong? You've mentioned the "read" methods are called frequently. What will happen if it's called and the subscribe() method hasn't finished? It can't read any broken data, can it?

(un)subscribe() can cause subscription not found (getting historical metrics in subscribe can take some time).

not sure I follow, I made some changes to your comment, to be able to understand it. Hopefully, I didn't change the meaning :]

isSubscribed , subscription and subscriptionOfTenant

How isSubscribed can throw the SubscriptionNotFoundException ? What is subscriptionOfTenant?

This looks odd to me in InMemorySubscriptionManager.java

line 80: if (tenantSubscriptions.getSubscriptions().get(subscription) != null) {

line 97: tenantSubscriptions.getSubscriptions().put(subscription.getMetric().getMetricId(), subscription);

What is the key in the inner hash map (tenantSubscriptions.getSubscriptions()) is it the subscription, or the metricId?

[pavolloffay]

..what can go wrong in subscribe()

What can cause the loss of subscription if subscribe(Subscription) is not synchronized at all, even with concurrent map?

Imagine tenant with no previous subscriptions calls subscribe(subscription1): tenantSubscriptions = subscriptions.get(subscription.getMetric().getTenant()); returns null so new TenantsSubscriptionsHolder is created. Meanwhile context is switched for another subscription in subscribe(subscription2) and subscription2 belongs to the same tenant as subscription1 so again tenantSubscriptions = subscriptions.get(subscription.getMetric().getTenant()); returns null and one subscription get lost. Which one depends on how the context is switched.

By throwing SubscriptionNotFoundException was meant that any of methods subscriptionsOfTenant(String tenant), subscription(String tenant, String metricId) and unsubscribe(String tenant, String metricId, Set<Subscription.SubscriptionOwner> subscriptionOwners) thows SubscriptionNotFoundException when are called immediately after subscribe(Subscription), to avoid this behavior these entire methods would have to be synchronized. My question was if this behavior is ok (throw ex)? I think it is it, I also think if these methods would be all synchronized it would have significant performance effects.

[jkremser]

Imagine tenant with no previous subs..

This is currently handled.

By throwing SubscriptionNotFoundException ..

But subscriptionsOfTenant() doesn't currently throw the (^) exception, neither the isSubscribed(). I am pretty indifferent if you throw an exception, return null or use Optional. You know the architecture and things like how often those "read-only" methods are called, how likely it is that it wont be initialized by the time it's called. In other words how close the calls of subscribe() and subscription() methods are closed to each other in time.

You've asked about the exception. If the exception will happen frequently, then it's better to use Optional, or null.. If it's really an edge case happening (I don't know 0.1 - 1 % of calls) and the system will continue to work even if this happen, than it's fine. We gain the performance here with the small price. Otherwise, I'd synchronize everything or use the read/write locks.

What about this guy https://github.com/hawkular/hawkular-datamining/blob/subscription-manager-thread-safe/hawkular-datamining-api/src/main/java/org/hawkular/datamining/api/base/InMemorySubscriptionManager.java#L80 ?

Isn't it a typo?

[pavolloffay]

Ok, I will go with the exception approach. For the cases when there are lots of metrics to be learned I first check isSubscribed().

Yes, that is typo, thanks :+1: