Simplify the API

[ppalaga]

No description provided.

[ppalaga]

@jmazzitelli this is not for M5. And I do not have Agent working on top of this yet. This is a very rough idea how the cmd gw could be simplified. Could you please have a look if I am not oversimplifying somewhere?

[ppalaga]

Actually after this change, the gw does not depend on Inventory anymore and could go back to bus

[jmazzitelli]

this adds complexity where I don't think it is needed. What is the use-case for needing to define recipient type? first, there could be messages that could flow in either direction (for example, the simple EchoCommand, but in the future we could have Notification type commands where either a ui or feed will be given a notification message. Second, even for those messages that are destined for just one side or the other, it will be obvious by the command type itself (for example "DeployApplication" is clearly not going to have the UI as a recipient). So to me this just adds complexity where it isn't needed.

[ppalaga]

This is not carved in stone, you may well be right, that we can live without that. My motivation for introducing it was to make the command gateway as stupid and generic as possible. I think that the gateway should be just that: transfer the messages between the web sockets and the bus, do some authentication (and perhaps authorization) but nothing more. If the messages know where they go from and to, the gateway does not need to know which makes the gateway simpler.

Yeah, and recipientType allows for sending one message type to both kind of destinations, regardless from where it was sent.

[ppalaga]

4db0e03 fails because it depends on hawkular/hawkular-bus#44

[ppalaga]

An important message about 4db0e03 is that I have all except for AddJdbcDriver itests working on top of it in hawkular/hawkular-agent#65

@jmazzitelli I hope, 4db0e03 is much more acceptable for you than the previous proposal.

[jmazzitelli]

the AbstractBasicMessageListener has its own onMessage() impmementation that does alot of stuff. overriding it here scares me because you are no longer doing any of it anymore. Where is the "onBasicMessage" implementation?

[ppalaga]

AbstractBasicMessageListener has its own onMessage()

You certainly mean BasicMessageListener that has its onMessage() and onBasicMessage(). I could not use those because they take the BasicMessage class to deserialize into type T from type argument T extends BasicMessage of the listener, which in turn assumes that listeners are implemented one per message class, which is not the case in my proposal. I rather have one listener for all message classes, that dynamically selects the command to handle the given message. But of course, to deserialize the message, I need another source of the class to deserialize into: I introduced a dedicated JMS message header to store that info #14 (diff)

[jmazzitelli]

curious why there is no setter here for this? setSenderRequestId() is missing. Is this on purpose?

[ppalaga]

An interesting topic :) I am a proponent of making classes immutable by default. Here on messages that can perhaps get processed asynchronously, the immutability would make especially good sense. I had no time so far to have a look how and if at all the jsonschema2pojo-maven-plugin supports immutability, but to make a potential switch to immutable message classes easier, I am adding only those setters that I need in the server code for now (before we have builders or other similar mechanism in place that can produce a modified copy of an immutable object).

[jmazzitelli]

again, rename "queue" to just "endpoint" so we don't lock ourselves out of being able to use topics in the future.

[ppalaga]

done

[ppalaga]

8c4ae14 is the next iteration that adds/removes the bus listeners dynamincally as the WebSocket clients connect/disconnect. It also renamed fields and variables from queue to endpoint.

[ppalaga]

BusEndpointProcessors register a SessionListenerProducer of type BiFunction<String, Session, WsSessionListener> on the appropriate WsSessions: 8c4ae14#diff-fd9415db85bdbac610f95e75aae2504cR226

On Ws Session open, these SessionListenerProducer s are then asked to produce a WsSessionListener that is attached to the given session inside WsSessions: 8c4ae14#diff-3b0c55a107f13d3053ebf1c73f22c09aR158

In this way, BusEndpointProcessors do not need to maintain a map of sessionKeys to ListenersList to keep track of opened listeners that need to be removed on session close. Listeners removing happens in WsSessionListener.sessionRemoved() 8c4ae14#diff-fd9415db85bdbac610f95e75aae2504cR104 that is invoked directly from WsSessions 8c4ae14#diff-3b0c55a107f13d3053ebf1c73f22c09aR212

[ppalaga]

Another important note about 8c4ae14 is that I had to introduce some explicit locking to solve the problem that a parallel thread can add a new entry to sessions map between sessions.get() and sessions.remove() in 8c4ae14#diff-23032fb61a809dbbda23f3ef3686b9b0L127 A neat consequence of the explicit locking is also a guarantee for the implementors of WsSessionListener that their sessionAdded() will be called before sessionRemoved() and that there will be no overlapping calls of these methods from distinct threads: 8c4ae14#diff-93bd88af23765e7e0d381d2842ce0223R24

This problem existed before the present change already: 8c4ae14#diff-23032fb61a809dbbda23f3ef3686b9b0L127

[ppalaga]

The itests in ppalaga/hawkular-agent@4d1776d are passing on top of the present changeset 8c4ae14

[jmazzitelli]

This mentions "UI Client" but this class is for both feeds and UI clients, so just make the javadocs more general. Same thing in the @return description