Multi-timestamp Send implementation #1217

Michal-Wadowski · 2022-03-08T12:57:06Z

The solution for #1192

sbernard31

A first review without looking at small details.

sbernard31 · 2022-03-08T15:03:50Z

leshan-core/src/main/java/org/eclipse/leshan/core/node/TimestampedLwM2mNodes.java

+    /**
+     * Get all collected nodes represented as {@link LwM2mPath}-{@link LwM2mNode} map.
+     */
+    Map<LwM2mPath, LwM2mNode> getNodes();


I don't get what returns getNodes() ?

The TimestampedLwM2mNodes is collection of nodes with information about path and timestamp. This methods just returns nodes (path map). Should be called getAllNodes()? I'm not quite understand the problem.

My concern is that I'm not sure what means "all nodes".

Eg. :

t1 => path1 => nodeValue1a t1 => path2 => nodeValue2 t2 => path1 => nodeValue1b t3 => path1 => nodeValue1c

In this case, with a map from path to node, we will have only 1 value for path1 or I missed something ? so this is not really all node ? or ?

My intention in this case is provide way to get nodes as usual, ignoring timestamps (if you don't care about timestamps). In other case you can use getTimestampedNodes(), getNodesForTimestamp() to be time-aware. What name should I use to improve readability or give more context? And maybe javadoc is not enough decriptive?

Ok I get it.
So I guess if you don't care about timestamped you want to get more recent value right ? So maybe this should be added in javadoc and/or the method is renamed in getMostRecentNodes() (or something like this) ?

Bonus quesiton :
Is it possible to have timestamped value mixed with "not timestamped" value ?
If it is possible, null timestamped value is more or less recent ?

I want to avoid the "most recent" phrase because it suggest to gets only "t3 => path1 => nodeValue1c" value from example above (because it's really most recent). My intention is to ignore timestamp segregation. Of course in example above we have conflicts and we have to choose one of "nodeValue1a", "nodeValue1b", "nodeValue1c" and the last one seems to be most appropriate indeed.

Is it possible to have timestamped value mixed with "not timestamped" value ?

If client send this kind of data and it's technically valid, the server should be able to handle it. So the answer is "yes"

If it is possible, null timestamped value is more or less recent ?

It's undefined by definition, and until you use methods getTimestampedNodes(), getNodesForTimestamp() and getTimestamps() it doesn't really matter (the consumer implementation have ability to manage the order). I had to provide some order for technical reasons (default TreeMap doesn't support null keys), so I recreated Comparator.nullsFirst() equivalent from java 8 (it's not copy paste!).

Maybe I should add some tests to valid order in cases pointed above?

I want to avoid the "most recent" phrase because it suggest to gets only "t3 => path1 => nodeValue1c" value from example above (because it's really most recent). My intention is to ignore timestamp segregation. Of course in example above we have conflicts and we have to choose one of "nodeValue1a", "nodeValue1b", "nodeValue1c" and the last one seems to be most appropriate indeed.

I understand but this seems more confusing to me to not explained how returned nodes are selected.
Even if the wording "most recent" is not crystal clear it gives at least some hints about the behavior which is only most recent value of each node is selected.

Anyway, not a big deal if the method name does not contains the "most recent" idea but I think at least the javadoc should clearly define the behavior.

It's undefined by definition, and until you use methods getTimestampedNodes(), getNodesForTimestamp() and getTimestamps() it doesn't really matter (the consumer implementation have ability to manage the order). I had to provide some order for technical reasons (default TreeMap doesn't support null keys), so I recreated Comparator.nullsFirst() equivalent from java 8 (it's not copy paste!).

I'm a bit confused by the idea because I don't know how this could be undefined.
At implementation level we must decide which node we keep in this path=>node map when we have something like :
E.g. :

null => path 1 => nodeValue1a t1 => path 1 => nodeValue1b t2 => path 1 => nodeValue1c

I guess null can be consider as now and so probably the most recent one and timestamp is for past value. (future value does not really make sense 🤔 )

Maybe I should add some tests to valid order in cases pointed above?

It sounds a good idea.

I'm a bit confused by the idea because I don't know how this could be undefined.

I mean this order is nowhere directly described, so we have to establish how order null timestamp.

I guess null can be consider as now and so probably the most recent one and timestamp is for past value.

Ok for me. I've updated implementation to take null timestamp as the latest.

sbernard31 · 2022-03-08T15:10:20Z

leshan-core/src/main/java/org/eclipse/leshan/core/node/TimestampedLwM2mNodesImpl.java

+/**
+ * The default implementation of {@link TimestampedLwM2mNodes}
+ */
+public class TimestampedLwM2mNodesImpl implements TimestampedLwM2mNodes {


Do we really need a Interface + class (impl) ?
Currently the LwM2mNode hierarchy and TimestampedLwM2mNode are just class.
Maybe for consistency we should just have a class for TimestampedLwM2mNodes too ? unless there is a real need to create a dedicated interface ?

The core functionality is extracted as interface and - as you ask before - there is read only. The concrete implementation have some options that allow to create and manipulate instance (add elements, create instance etc). There is functional separation where read only interface should be used in most cases and concrete implementation to create one.

sbernard31 · 2022-03-08T15:11:58Z

leshan-core/src/main/java/org/eclipse/leshan/core/node/TimestampedLwM2mNodesImpl.java

+        return timestampedPathNodesMap.keySet();
+    }
+
+    public void put(Long timestamp, LwM2mPath path, LwM2mNode node) {


TimestampedLwM2mNodesImpl seems not to be immutable.
See #1192 (comment)

If possible I would prefer to make it immutable.

Immutable is TimestampedLwM2mNodes interface. Concrete implementation should not be exposed outside.

Not a big deal but I think we don't understand Immutable in a same way.

I understand that immutable means that an object can not be modified once it is created.
So this is not really applicable to an interface because an interface can not be created.
And in this case, the object behind the interface can be modified by using cast.

Anyway, you can let this point aside if you want :-)

There are many ways to achieve the goal. I changed the implementation to make TimestampedLwM2mNodes immutable (without interface).

sbernard31 · 2022-03-08T15:20:10Z

...ation-tests/src/test/java/org/eclipse/leshan/integration/tests/send/SendTimestampedTest.java

+        public TimestampedLwM2mNodes decodeMultiTimestampedNodes(byte[] content, ContentFormat format,
+                LwM2mModel model) throws CodecException {
+            Map<Long, LwM2mNode> timestampedNodes = getExampleTimestampedNodes();
+
+            TimestampedLwM2mNodesImpl data = new TimestampedLwM2mNodesImpl();
+            for (Map.Entry<Long, LwM2mNode> entry : timestampedNodes.entrySet()) {
+                data.put(entry.getKey(), getExamplePath(), entry.getValue());
+            }
+
+            return data;
+        }
+


I'm not sure I get the point here because it seems that the server will ignore the byte[] content ?
Should we rather hack the client instead of server ?
I mean the purpose of this PR is to add the feature at server side without the client side.
So I can understand that we hack the client to make it send a timestampted payload but if we hack the server I'm not sure what we are testing here ? 🤔

It was the starting point for searching solution, checking what happen if we get already decoded nodes in SendResource.handlePOST(). Where is no direct tests for SendResource and SendHandler so I just used integration tests to check this layer. I didn't hack the client because there was no real implementation of encoder/decoder yet.
But for now this test seems obsolete and it looks like it's actually better to modify client side indeed.
I'll check if I can provide better solution.

If you want a kind of hack you can look at https://github.com/eclipse/leshan/blob/master/leshan-integration-tests/src/test/java/org/eclipse/leshan/integration/tests/observe/ObserveTimeStampTest.java

Another solution, If you plan to add support of this at client side soon, we can wait for client implementation before to add Integration Test ?

In the meantime I modified test to hack client side rather than the server side

sbernard31 · 2022-03-10T17:46:37Z

@Michal-Wadowski thx for last changes.
I reviewed it more in details and fix some littles issue/details at #1218.
Could you review if this is OK for you ?

sbernard31 · 2022-03-18T13:43:23Z

This is included in #1218.
So we can close this.

Michał Wadowski added 4 commits March 8, 2022 13:57

Add multiple timestamped nodes container and encoder/decoder placeholder

6b2e8ed

Add timestamps support in decodeMultiTimestampedNodes()

91c70b0

Tests fixed

45689a0

Cleaning the code

c2137a8

Michal-Wadowski force-pushed the opl/send_timestamp4 branch from 2ee77ba to c2137a8 Compare March 8, 2022 12:58

Michal-Wadowski mentioned this pull request Mar 8, 2022

Unable to Send and receive multiple values with different timestamps of the same resource #1192

Closed

sbernard31 reviewed Mar 8, 2022

View reviewed changes

Modify SendTimestampedTest context from server side to client side

0ca697d

Michal-Wadowski force-pushed the opl/send_timestamp4 branch from ca77907 to 0ca697d Compare March 9, 2022 09:50

Michał Wadowski added 2 commits March 10, 2022 09:22

Make TimestampedLwM2mNodes immutable

d6e5a92

Establish defined behaviour for null and not-null mixed timestamps

59b7c58

sbernard31 mentioned this pull request Mar 10, 2022

Add Timestamped Nodes support to Send Operation at Server side. #1218

Closed

sbernard31 closed this Mar 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-timestamp Send implementation #1217

Multi-timestamp Send implementation #1217

Michal-Wadowski commented Mar 8, 2022

sbernard31 left a comment

sbernard31 Mar 8, 2022

Michal-Wadowski Mar 9, 2022

sbernard31 Mar 9, 2022

Michal-Wadowski Mar 9, 2022

sbernard31 Mar 9, 2022

Michal-Wadowski Mar 9, 2022

sbernard31 Mar 9, 2022

Michal-Wadowski Mar 10, 2022 •

edited

Loading

sbernard31 Mar 8, 2022

Michal-Wadowski Mar 9, 2022

sbernard31 Mar 8, 2022

Michal-Wadowski Mar 9, 2022

sbernard31 Mar 9, 2022

Michal-Wadowski Mar 10, 2022

sbernard31 Mar 8, 2022

Michal-Wadowski Mar 9, 2022

sbernard31 Mar 9, 2022

Michal-Wadowski Mar 9, 2022

sbernard31 commented Mar 10, 2022

sbernard31 commented Mar 18, 2022

Multi-timestamp Send implementation #1217

Multi-timestamp Send implementation #1217

Conversation

Michal-Wadowski commented Mar 8, 2022

sbernard31 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michal-Wadowski Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbernard31 commented Mar 10, 2022

sbernard31 commented Mar 18, 2022

Michal-Wadowski Mar 10, 2022 •

edited

Loading