Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Observationstore improvement #87

Closed
wants to merge 1 commit into from

Conversation

sbernard31
Copy link
Contributor

Add the possibility to return observations removed from the store pending an addition.

This could be useful to limit the number of observations for a given target resource.
(There is a discussion about that here)

@sbernard31 sbernard31 changed the base branch from master to 2.0.x September 2, 2016 13:34
Signed-off-by: Simon Bernard <sbernard@sierrawireless.com>
@sbernard31 sbernard31 force-pushed the observationstore_improvement branch from 9290ee7 to aa9eef1 Compare September 2, 2016 13:39
@sbernard31
Copy link
Contributor Author

@sophokles73 does it sounds good for you ?

@sophokles73
Copy link
Contributor

I think that I do not understand the purpose of this. Can you give an example use case?

@sbernard31
Copy link
Contributor Author

^^ !
This is what I try to explain here

Observe Coap spec say that we can only have 1 observe relation for a given resource target.
The observationStore is responsible to keep the observation data consistent.
So when we add a new observation, if a relation already exist for this target resource, we should remove the previous one and this should be done in an atomic way.

@sophokles73
Copy link
Contributor

That's not how I understand the spec. The spec says that you need to aggregate all observations of the same resource. IMHO this means that we need to keep all requests that had been used to establish an observation on the same resource and notify all clients (that have issued the requests) about incoming notifications individually. We cannot simply cut off all previous observations established by (potentially other) clients.

@sbernard31
Copy link
Contributor Author

I talk about CoAP client side (not server side).

In the observationStore, at CoAP client side, we should not keep several observation for the same resource target.

So when we add a new observation in the observationStore on a target resource already observed. We should remove the previous one.

@sophokles73
Copy link
Contributor

You seem to imply that there can always only be a single client application observing a resource. How does this work for leshan as the client side? If I have multiple applications connected to the leshan server and they all want to observe the same resource, how is this handled? Does leshan create a single observation with Californium for the resource only and then distributes all incoming notifications to the observing applications? Or does leshan create separate observe requests (one for each application) and expects Californium to forward notifications to leshan for all observations individually?

@sbernard31
Copy link
Contributor Author

From my point of view, there is only one observe relation between Leshan Server (Coap Client) and a LWM2M client (CoAP Server).

This seems to be not really efficient to force LWM2M client to send several time the same notification for the same resource (with just a different token). And as I understand the spec this is not really allowed.

About the Leshan interface, this is a good question... For now there is 2 differents features, established an observe relation (sending a request) and listen for notification ( add a listener). This is 2 independent seperate features. So one application can send a request, several can listen for notification.

@sophokles73
Copy link
Contributor

From my point of view, there is only one observe relation between Leshan Server (Coap Client) and a LWM2M client (CoAP Server).

Yes, that's what the CoAP Observe spec mandates. However, the question remains, how leshan and Californium handle this between them.

Let's make an example: the first application starts observing a resource using leshan's API. leshan creates a corresponding GET request and sends it to the device using Californium. leshan keeps track of this observation in its own ObservationStore. When a second application wants to observe the same resource, we have several options

  1. leshan simply registers the new application's interest in the resource internally but does not issue a new GET request (with Observe option) via Californium. Whenever a notifcation is received, leshan sends it to both applications.
  2. leshan registers the new application's interest internally and also issues a new GET request to the device. Californium registers this second observation internally but does not send the second GET request to the device because it already has an observation registered for the resource. Whenever a notification arrives, Californium forwards it twice to leshan (once for each observation/application).

@balsmn
Copy link

balsmn commented Sep 7, 2016

IMHO, as CoAP Observe spec brings the restriction (only one observe relation between CoAP Client and CoAP Server), it should be handled exactly in Californium;i.e:- the solution I would opt for, would be the 2nd one what @sophokles73 mentioned above.

leshan registers the new application's interest internally and also issues a new GET request to the device. Californium registers this second observation internally but does not send the second GET request to the device because it already has an observation registered for the resource. Whenever a notification arrives, Californium forwards it twice to leshan (once for each observation/application).

This way Leshan doesn't have to be concerned with the CoAP's restriction.

@sbernard31
Copy link
Contributor Author

sbernard31 commented Sep 7, 2016

3 peoples, 3 ideas ^^

I see the observe feature more like the possibility to have a synchronized resource.
From my point of view, sending a Observe request (at CoAP or LWM2M side) means "begin the synchronization".
Cancel the observation, means "stop synchronization".
If a resource is in a synchronized state, anybody can get new values. (even if it does not initiate the observation)
If somebody resend an Observe request on a synchronized resource. the resource will still be synchronized (at coap level this will be a new relation if a new token is used or the same one if this is the same token).

So for me, the behavior described by Kai seems out of scope. If a platfrom needs to isolate observation by application, it should be implemented on top of Leshan/Californium.

@balsmn
Copy link

balsmn commented Sep 7, 2016

hmm, may be I was not much clear in my previous comment. I read the spec thrice now.

CoAP RFC 7641 - Section 3.1 titled under Section 3 titled Client-side requirements states

Like a fresh response can be used to satisfy a request without contacting the server, the stream of updates resulting from one observation request can be used to satisfy another (observation or normal GET) request if the target resource is the same. A client MUST aggregate such requests and MUST NOT register more than once for the same target resource.

My understanding (for satisfy another (observation or normal GET)) is that CoAP-client can get more GET request, possibly from an upper layer like Leshan.

and

My understanding (for A client MUST aggregate such requests and MUST NOT register more than once for the same target resource) is that CoAP-Client adds all upper layer calls, possibly from Leshan per application to its list of listeners like @sophokles73 mentioned in 2nd option above.

So IMO, that if CoAP-Client has the responsiblity for such agrregation of GET/Observation requests, then why replicate this responsiblity again in Leshan?

May be I am approaching this totally from a wrong prespective. May be this is something that has to be clarified in LWM2M Spec and is a wrong place here in PR to be discussed. I am not sure if LWM2M Spec has anything defined for this specifically.

@@ -199,7 +200,7 @@ public void sendRequest(final Exchange exchange, final Request request) {
if (request.getOptions().hasObserve() && request.getOptions().getObserve() == 0 && (!request.getOptions().hasBlock2()
|| request.getOptions().getBlock2().getNum() == 0 && !request.getOptions().getBlock2().isM())) {
// add request to the store
observationStore.add(new Observation(request, null));
List<Observation> observationRemoved = observationStore.add(new Observation(request, null));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we say there can be at most only one observe per resource, should not add(Observation) return one Observation instead of a List?

@sbernard31
Copy link
Contributor Author

I read the spec again and I skipped "A client MUST aggregate such requests".(It's not so clear for me)
If you're right, this describes the interface between a CoAP client and the application layer.
(I'm bit surprised that CoAP spec is so strict on this kind of interface...)

So supposing we implement this.

  1. I suppose an observe relation is already established. I use the CoAPEndpoint.send() API to send another observe on the same target resource. I expect to get a response. The CoAP client should not established a new relation,but the API should return a response for this new request (with the current value, and an observe option to say if the observation relation is established). I suppose we can return the last value received if it is fresh enough, but this means we need to cache the last value received for each observation. If the value is not fresh enough we need to send the request to the CoAP client with the token of the previous relation (to reinforce the relation and get a recent value)

  2. What happens if I set the token on my observe request, does it means it will be ignored and I will receive a request and response a with different token on notificationListener ?

void onNotification(Request request, Response response);
  1. What about cancellation, the current API is not really adapted to this new behavior :
void cancelObservation(byte[] token);

Currently we face some problems about race conditions, clustering. I feel adding more logic and state like this will not really help.

@boaks
Copy link
Contributor

boaks commented Jun 20, 2017

Is this PR still proposed?

@sbernard31
Copy link
Contributor Author

The problem is still here, i still think that this PR could help, but no consensus emerge of this discussion. :'(

@sbernard31
Copy link
Contributor Author

I close this for now as I'm not sure we will find a consensus.

Let's reopen it later if the problems become insistent.

@sbernard31 sbernard31 closed this Nov 13, 2017
@boaks boaks deleted the observationstore_improvement branch December 19, 2017 11:40
@sbernard31
Copy link
Contributor Author

sbernard31 commented Feb 11, 2020

@boaks, I look at old Leshan issues and reading the #322 eclipse-leshan/leshan#332 one makes me rethink about this old case ^^.

Do you have any opinion about this long topic ?
My principal concern would be to make cleaner the observe stuff in Leshan when we send twice the same observe request. (you could have a look at ObservationServiceImpl and InMemoryRegistrationStore or RedisRegistrationStore)

@boaks
Copy link
Contributor

boaks commented Feb 12, 2020

Both seems to be quite long ... too long for me ;-)

My principal concern would be to make cleaner the observe stuff in Leshan when we send twice the same observe request. (you could have a look at ObservationServiceImpl and InMemoryRegistrationStore or RedisRegistrationStore)

That depends more on the definition of "twice the same observe request".

@sbernard31
Copy link
Contributor Author

Uups I made a mistake about the issue. I would like to talk about eclipse-leshan/leshan#332 (not #322 ...)

That depends more on the definition of "twice the same observe request".

Something like :

server.send(myDevice, new ObserveRequest("/3/0/13", responseCallback, errorCallback);
server.send(myDevice, new ObserveRequest("/3/0/13", responseCallback, errorCallback);

Currently in leshan we try to cancel the previous observation and keep only the last one. But the californium API does not really help to do that. (so we have a kind of strange workaround in code to do that)
I proposed this PR longtime ago but this finished in a more complex discussion about how this should be done in CoAP.

@boaks
Copy link
Contributor

boaks commented Feb 12, 2020

Currently in leshan we try to cancel the previous observation and keep only the last one. But the californium API does not really help to do that. (so we have a kind of strange workaround in code to do that)

Hm, my feeling is, that help is in CoapClient and CoapObserveRelation, which is not used by leshan. So I'm not sure, what is missing.

Shouldn't this be prevented in the server?

I'm not sure, if this refers to the lwm2m server (and so coap-client).
If this refers to the coap-client, the observation-store may be a good point either cancel the previous or block the new observation.

AFAIK, currently (at least for the last years), some client constraints, e.g. NSTART-1 or this MUST NOT register more than once for the same target resource are pushed to the application. I'm not sure, which implications will show up, if that get's now moved to the californium library.

@sbernard31
Copy link
Contributor Author

I'm not sure, if this refers to the lwm2m server (and so coap-client).

It refers to lwm2m server (coap client)

If this refers to the coap-client, the observation-store may be a good point either cancel the previous or block the new observation.

I agree.
cancel the previous => That's what I tried to do in this PR a long time ago
block the new => Could be implemented with the new #1207

AFAIK, currently (at least for the last years), some client constraints, e.g. NSTART-1 or this MUST NOT register more than once for the same target resource are pushed to the application. I'm not sure, which implications will show up, if that get's now moved to the californium library.

I'm not sure to get your point but we can can imagine to let the ObservationStore default implementation like this but just allow to behave differently (block with #1207 or cancel with a PR like this one ?)

@boaks
Copy link
Contributor

boaks commented Feb 12, 2020

Sure ... that seems to be more a javadoc change, right?

@sbernard31
Copy link
Contributor Author

Yes but it also means changing a bit the observationStore API by changing the addObservation method.
(like in this PR) or I missed something ?

@boaks
Copy link
Contributor

boaks commented Feb 12, 2020

Assuming, that the "other" request already received the response, the main cleanup is to remove the token from the observation store itself. That could be done within the add.

The returned list may only be used to cleanup ongoing requests, potentially blockwise requests. I would consider that as optimization.

@sbernard31
Copy link
Contributor Author

The returned list may only be used to cleanup ongoing requests, potentially blockwise requests. I would consider that as optimization.

🤔 you probably right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants