my apologies if my comments, feedback and alternative branch were not as helpful to you as they have been to me. After all of this I now have a much better understanding of the problem domain and what you have in mind regarding clustering (in particular I wasn't aware of the load balancing aspect induced by the CoAP server's constant changing of IP addresses if behind a NAT firewall).
1) My first modification was to change the internal/advanced API to
introduce a new ObservationStore.
The main modification was:
- Before, the exchange lived all along the observe relation
lifetime.
- Now, we recreate an exchange for each notification. I mean a
new Exchange instance is created from data stored in
ObservationStore each time we receive a new notification and will be
completed/removed as soon as the notification is handled.
This means that the first response for an observe request will be
accessible via the MessageObserver of the original Request, then
next notifications will be available via NotificationListener.
This also means that now we are able to handle several
notifications transfered with block-wised(block2) at the same time.
(as there is now 1 exchange by notification)
I understood we agreed on this part. If this is not the case, we
should keep focus on this. (I mean ignore the point 2) )
My alternative approach tried to re-establish the Exchange being held in memory for the whole timespan during which notifications might arrive.
After having given this some more thought, I think that your approach of re-creating an Exchange from the information in the observation store for every notification (and discarding it again after processing) is the right way to go :-)
I have some concerns though regarding throughput:
If we hit the external shared persistence store (e.g. Redis) for every notification that is received, we will run at risk of blocking all available threads with waiting for the result returning from Redis. This might reduce the throughtput dramatically. We might be able to remedy this situation by implementing the ObservationStore as a two-level cache, the first one in memory keeping only the most recently used observation state and the second level in Redis which is only queried if we experience a first level cache miss (we already talked about hat approach). We could re-use the LeastRecentlyUsedCache that we use in Scandium for the first level cache and can can combine it with the "re-sync from Redis when notification's IP address has changed" strategy that I have introduced in the "observe_clustering_alt" branch. This way we might be able to reduce the external second level cache look-ups to a minimum and serve the majority of incoming notifications from memory.
2) As I believed the internal/advanced API modification was ok, I
tried to change the CoAPClient to make it operational with the new
internal/advanced API.
(I would like to exclude the notification orderer topic of this
point for now, I mean this is probably the next step, but let focus
on step 2) ).
So It seems this code modification concerning this point was
not really appreciate. I have no problem with that, but for now the
arguments don't really help me to provide another solution.
The main argument is "I don't like it".
Does it mean this does not works ? (What does not work ?)
Does it mean this is not elegant ? (Any idea to make it more
elegant ?)
You are absolutely right. We need some better arguments than "I don't like it".
I have already stated my concerns regarding scalability if we need to register a NotificationListener per observed resource when using CoapClient. Given that this API will not be used in a "server use-case" like leshan anyways, I guess this is not a big problem.
My other concern is regarding clarity of design. In your current proposal the NotificationListener looks like a gernal version of MessageObserver (at least it has exactly the same callback methods, only including the affected request and exchange). So we should either rename NotificationListener to something like "MultiMessageObserver" or "EndpointMessageObserver" to indicate that it in fact is the pendant to MessageObserver at the Endpoint level, or we should remove all callbacks that do not have anything to do with processing notifications (so that it looks more like the NotificationListener interface from "observe_clustering_alt") and have CoapClient register its MessageObservers as usual and in addition a NotificationListener adapter for receiving future notifications.
What do you think?
Kai
