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

Understanding RegistrationStore#addObservation #452

Closed
AlexITC opened this issue Dec 30, 2017 · 4 comments
Closed

Understanding RegistrationStore#addObservation #452

AlexITC opened this issue Dec 30, 2017 · 4 comments
Labels
question Any question about leshan

Comments

@AlexITC
Copy link
Contributor

AlexITC commented Dec 30, 2017

Previously I created a ticket related to some confusion with the intention of this method (see #425).

Now, I'm implementing a RegistrationStore and I'm getting confused by this method again.

  • InMemoryRegistrationStore#addObservation is removing existing observations matching the registration id, the path, and not matching the current observation id, which means that we'll keep at most 1 observation. This is because at first californium add its observation and then this method is called, which is removing every observation but the current one.

  • RedisRegistrationStore#addObservation is using the same logic with the difference that it's never executed leading to not removing any observation, see:

List<Observation> removed = new ArrayList<>();
if (!removed.isEmpty()) {
  ...

The question is, what is this method really supposed to do? I'm thinking that it might be better to just do nothing and possibly removing the method to avoid confusion.

Thanks.

@sbernard31
Copy link
Contributor

The quick answer : It's an ugly bug in RedisRegistrationStore ! Thx to report this !

About what we try to achieve in addObservation, you're right :
"This is because at first californium add its observation and then this method is called, which is removing every observation but the current one."
Why do we try to do that ? long story ... you can find some answer here eclipse-californium/californium#87.

"Why this method exists?" is explained in #425. But to summarized it here.

  1. Historically, that's about allowing to use another CoAP implementation than californium. (based on the idea that all implementation will not provide a kind of ObservationStore)
  2. be able to return "cancelled obsevations" via Leshan listeners.

I totally understand how much all of this must be confusing. I really would like to make it clearer.
But

  1. I'm not sure this is the good timing, there is currently some changes in Californium about observation and how they must be identified, this could be really impacting.
  2. We should clarify how observation should behave at californium side : Observationstore improvement eclipse-californium/californium#87

@AlexITC
Copy link
Contributor Author

AlexITC commented Jan 3, 2018

So, at the moment the idea of the leshan observation layer is to keep just 1 observation per client, right?

What would be the problem if we keep several observations for a client? In this case, they will be removed when the client is deregistered.

@sbernard31
Copy link
Contributor

the leshan observation layer is to keep just 1 observation by LWM2M client and by path.
I mean you can not create 2 observation relations for the same client with the same resource (identify by path). If you do that the previous one will be canceled.

Why not allowing 2 observe relations for same client / same path ?

  1. This does not make so much sense.
  2. The CoAP spec is not so clear about that : https://tools.ietf.org/html/rfc7641#section-3.1
  3. I don't really know how this behave at californium side.

But this is an open discussion :)

@sbernard31 sbernard31 added the question Any question about leshan label Jan 4, 2018
@AlexITC
Copy link
Contributor Author

AlexITC commented Jan 5, 2018

thanks.

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

No branches or pull requests

2 participants