Initial Commit of uP-L2 Communication Layer APIs #119

stevenhartley · 2024-06-07T17:35:10Z

The following commit includes a proposed common L2 API interfaces that is implemented as static methods that use uTransport (stateless).

#117

github-actions · 2024-06-07T17:36:27Z

Code coverage report is ready! 📈

Code Coverage Percentage: 99.00%
Code Coverage Report: View Coverage Report

tamarafischer

I see you have a lot of tests that should give you good coverage.
That said, I am concerned about the value of those tests to really test the scenarios and functionality.

I guess I don't like those register and unregister listeners. They are state on the uTransport that I believe would be a lot more stable if it could be immutable and not have that state changed.

src/main/java/org/eclipse/uprotocol/communication/CallOptions.java

tamarafischer · 2024-06-09T12:53:31Z

src/main/java/org/eclipse/uprotocol/communication/Notifier.java

+ * Notifier is an interface that provides the APIs to send notifications (to a client) or 
+ * register/unregister listeners to receive the notifications.
+ */
+public interface Notifier {


This class does not really do anything. It just wraps the uTransport commands. I don;t see too much value in this kind of wrapper

That is correct but the point is to make it simpler for developers to use a L2 API. These L2 APIs are optional and a developer can re-implement them as they see fit or use L1 APIs directly.

tamarafischer · 2024-06-09T12:55:31Z

src/main/java/org/eclipse/uprotocol/communication/Notifier.java

+        Objects.requireNonNull(transport, UTransport.TRANSPORT_NULL_ERROR);
+        Objects.requireNonNull(topic, "Notification topic missing");
+        Objects.requireNonNull(destination, "Notification requires a destination");
+        return transport.send(UMessageBuilder.notification(topic, destination).build());


IMHO it would be better to return the UMessage for notification and then let the caller decide if there is anything to add and then send it to the transport layer.
As an example, if we needed to add a token or another layer of security, then it would need to be added on top of the Message built here and then sent on the transport.
Having the method build the notification message is useful, sending it over the transport is redundant

@tamarafischer, the passed argument is a UMessageBuilder?

@tamarafischer then the developer can call UMessageBuilder to build the notification and then call transport.send() themself, they don't need to use this API.

src/main/java/org/eclipse/uprotocol/communication/Publisher.java

src/main/java/org/eclipse/uprotocol/communication/RequestListener.java

tamarafischer · 2024-06-09T15:51:32Z

src/main/java/org/eclipse/uprotocol/communication/UPayload.java

+ */
+public class UPayload {
+
+    private ByteString data;


I was unable to deserialize the ByteString here using an ObjectMapper
I believe a better structure would be a byte[]

Only difference from ByteString and byte[] (per Google documentation) is that ByteString is immutable and byte[] is not. We don't want it to change hence ByteString.

I am a fan of immutability, but it can be enforced by the UPayload being a Java 17 record which by default is immutable with an equals, hashcode and toString implemented (they took the idea from scala case classes)

My problem was, when I wanted to serialize the UPayload to a JSON representation, the ByteString did not play nicely with the Jackson ObjectMapper.

Another thing is that if we are already working with a google byte string, why not work with a google Message instead.
Then anyone can turn it into any kind of serialization.

Personally, I am still a fan of the simple byte[]
It gives the best flexibility.

Currently, I will not be able to use UPayload like this in my code and will need to wrap it with something else

Your proposal results then in yet another copy when we build the UMessage that is passed to transport.send().

src/test/java/org/eclipse/uprotocol/communication/CallOptionsTest.java

src/test/java/org/eclipse/uprotocol/communication/NotifierExample.java

tamarafischer · 2024-06-09T16:02:51Z

src/test/java/org/eclipse/uprotocol/communication/RpcServerExample.java

+                // automatically send back the response
+                // NOTE: To indicate a failure, complete the future exceptionally passing a UStatusException
+                // and the interface will handle propagating the error to the client.
+                responseFuture.complete(UPayload.packToAny(null));


how can this be a happy path test when you are packing null into the response?
If this turns into an object it is still pretty bad and the null is just scary

The purpose of the test is to simulate a service that is not sending a response message. I added UPayload.EMPTY so it is clearer

src/test/java/org/eclipse/uprotocol/communication/SubscriberExample.java

github-actions · 2024-06-11T13:21:31Z

Code coverage report is ready! 📈

Code Coverage Percentage: 99.00%
Code Coverage Report: View Coverage Report

github-actions · 2024-06-13T02:11:36Z

Code coverage report is ready! 📈

Code Coverage Percentage: 99.00%
Code Coverage Report: View Coverage Report

github-actions · 2024-06-14T01:26:39Z

Code coverage report is ready! 📈

Code Coverage Percentage: 96.00%
Code Coverage Report: View Coverage Report

github-actions · 2024-06-17T20:49:15Z

Code coverage report is ready! 📈

Code Coverage Percentage: 99.00%
Code Coverage Report: View Coverage Report

tamarafischer · 2024-06-18T08:11:10Z

src/main/java/org/eclipse/uprotocol/communication/CallOptions.java

+     * @param timeout The timeout for the method invocation.
+     * @param priority The priority of the method invocation.
+     */
+    public CallOptions(Integer timeout, UPriority priority) {


when working with records it is custom to create static factory methods instead of constructors
just makes it nicer to use
Another thing are the exceptions barfed in the record validation making this difficult to use in a functional way - everything now needs to handle these exceptions.

I guess it is just me not liking code with exceptions and null pointer exceptions.

@tamarafischer is this a must change? what would the static methods look like?

tamarafischer · 2024-06-18T08:12:47Z

src/main/java/org/eclipse/uprotocol/communication/DefaultNotifier.java

+     */
+    @Override
+    public UStatus notify(UUri topic, UUri destination, UPayload payload) {
+        Objects.requireNonNull(transport, UTransport.TRANSPORT_NULL_ERROR);


you already tested that you have a transport in the constructor. this might be leftover from when you had the transport as a parameter

Fixed thank you.

tamarafischer · 2024-06-18T08:16:30Z

src/main/java/org/eclipse/uprotocol/communication/DefaultNotifier.java

+ */
+public class DefaultNotifier  implements Notifier {
+    // The transport to use for sending the RPC requests
+    private UTransport transport;


so if we wanted a notifier that uses different transports - something you said in a different comment - then I would have to create another instance of the notifier to wrap the transport.

I am not liking the wrappers of uTransport much.
I feel more value is provided by helping create UMessage instances correctly and letting the developer send them to the uTransport of their choice, managing the listeners accordingly.

I feel more value would come from methods that let up build a pipeline of sending messages, reacting upon the data and then performing maybe another task.

An example of a discovery request, when the discovery response comes back, we open the payload and decide what we are going to subscribe to according to the returned calibration information

In that case the developer is free to use the UMessageBuilder and utransport API directly

tamarafischer · 2024-06-18T08:18:02Z

src/main/java/org/eclipse/uprotocol/communication/DefaultNotifier.java

+     * @return Returns the {@link UStatus} with the status of the listener registration.
+     */
+    @Override
+    public UStatus registerNotificationListener(UUri topic, UListener listener) {


Hmmm, looking at the functions that return UStatus... maybe we could add a map and flatMap to UStatus so we can easily compose functionality.
There is an example in the RpcMapper class

In RpcResult you mean. That would mean I no longer use proto defined uStatus. could we table this as something we can do later? I agree that building the ustatus right now is clunky.

tamarafischer · 2024-06-18T08:19:20Z

src/main/java/org/eclipse/uprotocol/communication/DefaultPublisher.java

+     */
+    @Override
+    public UStatus publish(UUri topic, UPayload payload) {
+        Objects.requireNonNull(topic, "Publish topic missing");


same comment as before, you don't need to recheck

tamarafischer · 2024-06-18T08:56:30Z

src/main/java/org/eclipse/uprotocol/communication/Subscriber.java

+     * @param listener The listener to be called when a message is received on the topic.
+     * @return Returns {@link UStatus} with the status of the listener unregister request.
+     */
+    UStatus unregisterListener(UUri topic, UListener listener);


unregister listener and I don't even know what the register listener did...
maybe the class cloud help provide the topic that I registered the listener with ...
If the client already needs to keep track for the unregister listener, I see no value in the listener being registered for him.
I would rather have support for a workflow

subscribe

process data from subscription

unsubscribe

that is what my program (a very simple one) would look like

the flow you mentioned works for 2 scenarios:

For temporal subscriptions

For permanent subscriptions when the uE is always running

It doesn't work for the 3rd scenarios where uEs are temporal but we need to be permanently subscribed, (i.e. for events that I want to be woken up for).
How do we support the 3rd use case without the unregisterListener()?

tamarafischer · 2024-06-18T08:57:29Z

src/main/java/org/eclipse/uprotocol/communication/UPClient.java

+    }
+
+    @Override
+    public UStatus unregisterListener(UUri topic, UListener listener) {


would make the name unregisterSubscriptionListener like the others

tamarafischer · 2024-06-18T08:58:15Z

src/main/java/org/eclipse/uprotocol/communication/UPClient.java

+
+
+    @Override
+    public CompletionStage<UPayload> invokeMethod(UUri methodUri, UPayload requestPayload, CallOptions options) {


more wrappers?!
oh dear

tamarafischer · 2024-06-18T08:58:50Z

src/main/java/org/eclipse/uprotocol/communication/UPayload.java

+     * 
+     * @return the payload data
+     */
+    public ByteString getData() {


ah, this is nice

tamarafischer · 2024-06-18T09:06:05Z

src/main/java/org/eclipse/uprotocol/transport/UTransport.java

+     * Close the connection to the transport that will trigger any registered listeners
+     * to be unregistered.
+     */
+    void close();


if the close had a problem, maybe a connection error with the MQTT server or any other network glitch, who's responsibility is it to make sure that everything was indeed cleaned up or maybe things need to be retried?
Is the uTransport responsible to make sure it retried all the cleanup?

The uE that created the uTransport instance is responsible to call close() when it shuts down to clean up.

github-actions · 2024-06-18T14:47:37Z

Code coverage report is ready! 📈

Code Coverage Percentage: 99.00%
Code Coverage Report: View Coverage Report

github-actions · 2024-06-18T15:11:02Z

Code coverage report is ready! 📈

Code Coverage Percentage: 99.00%
Code Coverage Report: View Coverage Report

The following commit includes a proposed common L2 API interfaces that is implemented as static methods that use uTransport (stateless).

github-actions · 2024-06-18T21:45:33Z

Code coverage report is ready! 📈

Code Coverage Percentage: 99.00%
Code Coverage Report: View Coverage Report

github-actions · 2024-06-19T13:10:34Z

Code coverage report is ready! 📈

Code Coverage Percentage: 99.00%
Code Coverage Report: View Coverage Report

stevenhartley requested review from neelam-kushwah and tamarafischer June 7, 2024 17:35

tamarafischer reviewed Jun 9, 2024

View reviewed changes

stevenhartley force-pushed the comm-layer-apis branch 2 times, most recently from 64fda67 to 62e13c0 Compare June 13, 2024 02:09

stevenhartley force-pushed the comm-layer-apis branch from 62e13c0 to 695e401 Compare June 13, 2024 17:34

stevenhartley force-pushed the comm-layer-apis branch from 51fe837 to 8c564fe Compare June 17, 2024 20:48

tamarafischer reviewed Jun 18, 2024

View reviewed changes

mikaelperrin approved these changes Jun 18, 2024

View reviewed changes

Initial Commit of uP-L2 Communication Layer APIs

5c2e598

The following commit includes a proposed common L2 API interfaces that is implemented as static methods that use uTransport (stateless).

stevenhartley force-pushed the comm-layer-apis branch from 345eb8a to 5c2e598 Compare June 18, 2024 21:44

neelam-kushwah approved these changes Jun 19, 2024

View reviewed changes

Incorporated feedback from Tamara.

5f0bd42

stevenhartley merged commit f883070 into main Jun 19, 2024
6 checks passed



		@Override
		public CompletionStage<UPayload> invokeMethod(UUri methodUri, UPayload requestPayload, CallOptions options) {

Initial Commit of uP-L2 Communication Layer APIs #119

Initial Commit of uP-L2 Communication Layer APIs #119

Conversation

stevenhartley commented Jun 7, 2024

github-actions bot commented Jun 7, 2024

tamarafischer 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenhartley Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jun 11, 2024

github-actions bot commented Jun 13, 2024

github-actions bot commented Jun 14, 2024

github-actions bot commented Jun 17, 2024

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

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

github-actions bot commented Jun 18, 2024

github-actions bot commented Jun 18, 2024

github-actions bot commented Jun 18, 2024

github-actions bot commented Jun 19, 2024

stevenhartley Jun 11, 2024 •

edited

Loading