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

Ideas for evolution #1

Open
kaifox opened this issue Apr 22, 2019 · 2 comments
Open

Ideas for evolution #1

kaifox opened this issue Apr 22, 2019 · 2 comments

Comments

@kaifox
Copy link
Member

kaifox commented Apr 22, 2019

This is rather not a simple issue, but more a collection of ideas for improvements. I will try to update it with more ideas as soon as they come up. They all stem from the fact that I started to try the properties as they are (barely copied from cern sources).

I think we could consider the following things to improve (probably along the way as we move them to a different namespace):

  • Decouple the properties from the reactive streams: I think a user should be able to choose what combination he/she wants to use. Currently the properties are coupled by design to flux. However, probably, a simple observer pattern would be ok here as a start (we could consider tow listeners: update and onChange).

  • For remote properties, it would be nice to have the transport layer choosable (e.g. websocket or webflux for subscription). This might already be possible with the current design.

  • The Sink/Sources concept is not fully clear yet ... do we really need this?

  • Remote properties has a lot of constructor arguments ... probably we can simplify this more

  • JavaFx properties: WE should be able to do the 'wrapping' or conversion also the other way around (ossgang-properties --> javafx properties). This way one could use bindings etc.... Nevertheless, this seems to be a bit cumbersome due to the vast amount of classes of javafx properties...

  • As there is already a websocket connection, couldnt we use this also for the get/set? This would avoid using 2 different protocols for subscribe and get/set.

  • Not sure if the 'get' of the RemoteProperty should change its internal state ... ? Currently it changes its latestValue.... In any case ... decouplling all these cases (with bindings) is tricky... ... Also the remote property seems to have some threading issues ...

  • Potentially, we could even consider implementing binding on the ossgang property level ... To me it looks as the javafx is quite complex and copy-pasty.... probably there is a simpler solution, which does everything we need....

@michi42
Copy link
Member

michi42 commented Apr 22, 2019

Hi,

Thanks for the good suggestions!

Decouple the properties from the reactive streams:

I agree. This is from the time where we were very enthusiastic about Flux, but that has been going down a bit again when we saw their caveats.
A simple observer pattern would be fine for me. I would only do an on-change listener for the time being, update looks redundant to me, or what would be the difference?
We could then provide an utility class providing streams, e.g. PropertyFlux.fromProperty(Property ...).

For remote properties, it would be nice to have the transport layer choosable

This is already the case I think. A RemoteRestProperty is just a property, which is internally backed by a REST/websocket transport layer and a remote property.
Likewise on the server side, the exposed server properties are "just" regular properties, which are set/observed by the REST server.

The Sink/Sources concept is not fully clear yet ... do we really need this?

Probably not if we decouple from reactive streams. IIRC this was just to encapsulate some stream manipulations, e.g. providing reactive "sources" (actually a ReplayProcessor with certain settings) etc.

Remote properties has a lot of constructor arguments ... probably we can simplify this more

I agree. If we go for pure websockets, that would get rid of the extra URL information for get/set.

JavaFx properties: WE should be able to do the 'wrapping' or conversion also the other way around

I didn't do this on purpose, as the JavaFX properties have a lot of dedicated classes and the binding mechanisms are very cumbersome, they were made at pre-Lambda times, and actually...

Potentially, we could even consider implementing binding on the ossgang property level

... this is already done ;-) But the file ended up in the server part instead of in Core:
https://github.com/ossgang/ossgang-properties/blob/master/ossgang-properties-web-server/src/main/java/cern/lhc/commons/web/property/Properties.java
See bind/bindBirdirectional(). The idea is that a binding requires a simple Function<I,O> which maps I to O. For a bidirectional binding, we require both a forward and a backward mapper.
Then there is also 'facade' properties, which are just 'views' of other properties, doing e.g. a transformation on the fly without keeping an internal state.

@andreacalia
Copy link
Member

Ciao kajetan,

I agree with all the points that you both presented :)

We have to just be careful about the Property interface.. at the moment it has a clear structure (get, set and subscribe). The Source interface is there only to express the need to have a stream and a "latest value" concept that is not present in Flux, Publisher, etc.. Using something like .onChange(Consumer<T>) could in principle be the base for then connecting the Reactive streams while still being able to simply subscribe to a Property. In this case we give up then the "latest value" idea and I think this is used quite some times in our applications.. (you can still store this value by subscribing of course, but then I would argue if this is then better..)

We could have some brainstorming about this of course :) I fully agree that we should not bound the core implementation to a specific technology (Flux in this case)

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

No branches or pull requests

3 participants