Initial WebSocket Client Connector ("ws"/non-SSL support) #32

dlaboss · 2016-02-19T16:53:04Z

Do not merge. Pull request created for sharing the proposed API.
Compiles but no backing implementation.

ddebrunner · 2016-02-19T17:03:38Z

connectors/websocket/src/main/java/quarks/connectors/ws/WebSocketClient.java

+     * @return sink
+     * @see JsonFunctions#asString()
+     */
+    public <T> TSink<T> send(TStream<T> stream, Function<T,String> toPayload) {


Do we need this method?

Why not just have the send<TStream<String>> and if anyone can use a upstream map to convert the tuple?

Yup, could omit it. I was following the patterns present in other connectors (for better or worse).

Pulling on that thread, all that's really needed is a single send: send(TStream<byte[]>) and analogous single receive. There's a benefit to that & I'm OK with it. Start with just those? Presumably not too late to trim down other connectors too - with commensurate savings in footprint and # of test cases.

Note, if we later decide we want the convenience fn send(TStream<String>) we'll have to name it something else due to type erasure. Other connectors addressed that sort of thing by naming the, presumed less frequently used, byte payload variant sendBytes. Do likewise here?

I think a connector should have methods to support the data types users logically send/receive from it. While it may all end up as byte[] under the covers the developer should be allowed to remain at their logical level, so if I naturally send strings then I don't want to be forced to do something like map(t -> t.getBytes(StandardCharsets.UTF8))

For websockets it probably depends on the use cases, but I would almost say JsonObject, String and byte[] and almost tempted to say bytes are the least likely and JSON the most likely.

ddebrunner · 2016-02-19T17:13:40Z

I wonder if the client should be a separate module to the server.

I'm interested in feedback from @wmarshall484 about the use cases for web-sockets, is there even a use case for a server on an edge device? Maybe gateway devices?

dlaboss · 2016-02-19T18:18:55Z

Ditto on feedback. Yup, my thinking was that a server would be an important case for a quarks gateway device. I'll put the client & server in separate components.

dlaboss · 2016-02-24T21:14:20Z

@ddebrunner, looking for a +1 for merging this intermediate level of completion.

WebSocket Client connector API is fully spec'd / doc'd
files for proposed server connector were removed
full "ws" (non-SSL) support. Working on "wss"/SSL next.
a healthy collection of passing tests. None failing.

- client and server in separate components - client support for JsonObject,String and byte[] streams. - similar for server Giving priority to the client.

- "wss" support is next on the list - removed iotf/ext/{javax.servlet,ibm.json4j} from component .classpath. They're unnecessary (issue#43) and javax.servlet caused binding problems with the websocket code when running from Eclipse. - note, the various test.ext/jetty jars are needed for the test websocket server. They're not included in the connector's jar.

- remove proposed wsserver artifacts - misc cleanup

wmarshall484 · 2016-02-24T23:40:50Z

...ctors/wsclient/src/main/java/quarks/connectors/wsclient/runtime/WebSocketClientReceiver.java

+    @Override
+    public void accept(Consumer<T> eventHandler) {
+        this.eventHandler = eventHandler;
+        connector.setReceiver(this);


Could connector.setReceiver(this) be called once in the constructor, instead of on every accept()?

This accept is the setup callback from Topology.events() and it's only called once for the lifetime of the job to initialize the "source". If setReceiver() was called prior to receiving the eventHandler, there might be cases where the receiver would be given a message so it would have to check every time to ensure it had an eventHandler to give it to.

ddebrunner · 2016-02-25T15:50:33Z

Does it make sense to have a interface for a web-socket client that is separate from the implementation?

That would then allow different implementations of the same interface. Jetty may provide the best libraries for web-sockets but I wonder if smaller footprint client libraries might exist.

That's what I'm going to do for serial ports ( #27 ), just have a connector that specifies an interface.

dlaboss · 2016-02-25T18:18:54Z

re the "interface" idea, we could do that, but I wonder if it's necessary as there's already a level of indirection in this WebSocketClient connector impl. The impl uses the std jsr-356 javax.websocket API - there's no direct references to any jetty code. Might that be sufficient? Are you concerned that jsr-356 itself will be too heavweight?

[ as an aside, if we're going to start creating interfaces for connectors should there also be a common factory patten to get an impl? ]

Details on the current impl...
Under the covers, javax.websocket.ContainerProvider.getWebSocketContainer() binds to an actual impl (via the java.util.ServiceLoader mechanism). We're getting the generic javax.websocket API & provider from one jetty supplied jar. There are other jetty jars that provide an actual implementation.
Currently they're all on the wsclient connector jar's manifest classpath (needs tweaking). But at least in theory all the pieces are there to allow alternative javax.websocket based implementations to be used by just tweaking the classpath/manifest.

ddebrunner · 2016-02-25T20:54:45Z

I didn't know there was a standard api for web-sockets, so we don't need to provide an additional separation.

ddebrunner · 2016-02-25T21:05:39Z

-1 on license issue

Please include the notice file from the Eclipse Jetty distribution.

ddebrunner · 2016-02-26T22:28:09Z

I'm still not seeing the notice file from Eclipse Jetty distribution in the ext, only the licence file.

dlaboss · 2016-02-26T22:54:32Z

ah. thx for the clarification.

dlaboss · 2016-02-26T23:09:51Z

BTW, I'm having second thoughts about not doing a wsclient interface. It seems like one "bare bones" websocket implementation, Java-WebSocket, doesn't have any javax.websocket support.

ddebrunner · 2016-02-26T23:20:03Z

That almost seems like a dead project, maybe take the yagni approach.

- TODO extract a WebSocketClient connector API interface - wsclient is a jsr356 javax.websocket API client (and includes a generic javax.websocket API impl jar from jetty) - user of the connector needs to also include a javax.websocket client implementation, such as the supplied javax.websocket-client.jar - add javax.websocket-client to capture a jetty based client impl - add javax.websocket-server to capture a jetty based server impl (used today by the WebSocketServerEcho test util for the wsclient API tests)

- don't require javax.websocket based client/server impls - ws based impl is now Jsr356WebSocketClient in package ...wsclient.javax.websocket ugh?

dlaboss · 2016-02-27T17:56:35Z

To summarize where things are now:

WebSocket Client connector is fully spec'd / doc'd
- wsclient.WebSocketClient connector API (don't preclude non-jsr356 based impls)
- wsclient.javax.websocket.Jsr356WebSocketClient connector API impl - bound only to generic jsr356 API
- separate jetty-supplied impls of jsr356 javax.websocket client and server
- hence user of Jsr356WebSocketClient has to include its jar and the (or some other) jsr356 javax.websocket client impl jar
files for proposed server connector were removed for now
full "ws" (non-SSL) support. Working on "wss"/SSL next.
a healthy collection of passing tests. None failing.

ddebrunner · 2016-02-28T01:58:33Z

So is the pull request still a wip ?

dlaboss · 2016-02-29T14:30:54Z

Re: Java-WebSockets, fwiw it's what streamsx.inet.wsserver uses.
Removing WIP.

dlaboss · 2016-02-29T16:07:56Z

@ddebrunner, why is this appropriate: interface IotDevice extends TopologyElement
Will it still be appropriate if TopologyElement extends Taggable (not sure if issue #39 implied that or not)
Following that lead I blindly did the same for WebSocketClient and want to be sure it's appropriate or to clean it up.

ddebrunner · 2016-02-29T17:56:44Z

It is an element of the topology. I could image such items being of interest in a graph visualization, though additional elements would need to be added to the JSON representation.

dlaboss · 2016-02-29T21:19:54Z

thx for clarification. +1 on a merge now?

ddebrunner · 2016-02-29T23:18:29Z

-1 lifted as ext directories now contain licence and notice files.

Initial WebSocket Client Connector ("ws"/non-SSL support)

dlaboss mentioned this pull request Feb 19, 2016

Add a WebSocket connector #29

Closed

ddebrunner reviewed Feb 19, 2016
View reviewed changes

dlaboss force-pushed the websockets branch from 89c993d to 313d3a9 Compare February 24, 2016 17:52

dlaboss changed the title ~~[WIP] Web Socket Connector - proposed API~~ [WIP] Initial WebSocket Client Connector ("ws"/non-SSL support) Feb 24, 2016

dlaboss added 7 commits February 24, 2016 16:45

[WIP] Web Socket Connector - proposed API

819b5b9

address feedback

5050aa9

- client and server in separate components - client support for JsonObject,String and byte[] streams. - similar for server Giving priority to the client.

add "/reports" generated artifacts to .gitignore

c5f3982

Add AutoCloseable to send/receive functions

b88c904

tidy up to enable intermediate wsclient delivery

f22d538

- remove proposed wsserver artifacts - misc cleanup

more tidying up

ecedf3d

dlaboss force-pushed the websockets branch from cae06a6 to ecedf3d Compare February 24, 2016 21:45

wmarshall484 reviewed Feb 24, 2016
View reviewed changes

add another copy of the

5257f33

dlaboss added 3 commits February 27, 2016 09:55

some changes missed in the prior push

cefc68d

refactor to extract WebSocketClient interface

9051432

- don't require javax.websocket based client/server impls - ws based impl is now Jsr356WebSocketClient in package ...wsclient.javax.websocket ugh?

dlaboss added 6 commits February 27, 2016 11:22

missed in prior push

6a9aa29

more tidying up

fe36634

more tidying up

8a87ad5

more tidying up

26202ff

(final?) tidying up

8c42946

this time for sure?

681413e

dlaboss changed the title ~~[WIP] Initial WebSocket Client Connector ("ws"/non-SSL support)~~ Initial WebSocket Client Connector ("ws"/non-SSL support) Feb 29, 2016

dlaboss added a commit that referenced this pull request Mar 2, 2016

Merge pull request #32 from dlaboss/websockets

a13f71c

Initial WebSocket Client Connector ("ws"/non-SSL support)

dlaboss merged commit a13f71c into quarks-edge:master Mar 2, 2016

dlaboss deleted the websockets branch March 4, 2016 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial WebSocket Client Connector ("ws"/non-SSL support) #32

Initial WebSocket Client Connector ("ws"/non-SSL support) #32

dlaboss commented Feb 19, 2016

ddebrunner Feb 19, 2016

dlaboss Feb 19, 2016

ddebrunner Feb 19, 2016

ddebrunner commented Feb 19, 2016

dlaboss commented Feb 19, 2016

dlaboss commented Feb 24, 2016

wmarshall484 Feb 24, 2016

dlaboss Feb 25, 2016

ddebrunner commented Feb 25, 2016

dlaboss commented Feb 25, 2016

ddebrunner commented Feb 25, 2016

ddebrunner commented Feb 25, 2016

ddebrunner commented Feb 26, 2016

dlaboss commented Feb 26, 2016

dlaboss commented Feb 26, 2016

ddebrunner commented Feb 26, 2016

dlaboss commented Feb 27, 2016

ddebrunner commented Feb 28, 2016

dlaboss commented Feb 29, 2016

dlaboss commented Feb 29, 2016

ddebrunner commented Feb 29, 2016

dlaboss commented Feb 29, 2016

ddebrunner commented Feb 29, 2016

Initial WebSocket Client Connector ("ws"/non-SSL support) #32

Initial WebSocket Client Connector ("ws"/non-SSL support) #32

Conversation

dlaboss commented Feb 19, 2016

ddebrunner Feb 19, 2016

Choose a reason for hiding this comment

dlaboss Feb 19, 2016

Choose a reason for hiding this comment

ddebrunner Feb 19, 2016

Choose a reason for hiding this comment

ddebrunner commented Feb 19, 2016

dlaboss commented Feb 19, 2016

dlaboss commented Feb 24, 2016

wmarshall484 Feb 24, 2016

Choose a reason for hiding this comment

dlaboss Feb 25, 2016

Choose a reason for hiding this comment

ddebrunner commented Feb 25, 2016

dlaboss commented Feb 25, 2016

ddebrunner commented Feb 25, 2016

ddebrunner commented Feb 25, 2016

ddebrunner commented Feb 26, 2016

dlaboss commented Feb 26, 2016

dlaboss commented Feb 26, 2016

ddebrunner commented Feb 26, 2016

dlaboss commented Feb 27, 2016

ddebrunner commented Feb 28, 2016

dlaboss commented Feb 29, 2016

dlaboss commented Feb 29, 2016

ddebrunner commented Feb 29, 2016

dlaboss commented Feb 29, 2016

ddebrunner commented Feb 29, 2016