rsocket / rsocket-java

Java implementation of RSocket
http://rsocket.io
Apache License 2.0
2.36k stars 354 forks source link

Enable per client/server plugins #298

Closed yschimke closed 7 years ago

yschimke commented 7 years ago

Related to comments in https://github.com/rsocket/rsocket-java/pull/292, splitting this out into its own task.

My one concern is that I think we should change the Plugins from being a JVM singleton registry, to per client or per server. I don't think the current model works well, if you are a client to two different reactivesocket services, or you are both a client and a server simultaneously. n.b. we already have this in our internal rsocket usage.

example: I want to connect to service A (a cluster of 20 hosts) and service B (a cluster of 1000 hosts) and my service is also a server, then it looks like difficult to have 3 sets of stats such that I can understand where latency is coming from.

Likewise the FragmentationDuplexConnection seems like something that may be different if I'm accepting public traffic, but then making requests to a backend service in the same data centre.

yschimke commented 7 years ago

cc @robertroeser @tmontgomery

tmontgomery commented 7 years ago

I would make these client/server specific. You might end up using the same infrastructure, but the way it is done might need to change.

robertroeser commented 7 years ago

I didn't want to make it part of the setup so that if you wanted to add some kind of plugin you wouldn't have to go back and change it every place you where using a setup. I was hoping for something along the lines of AOP, but without needing ASM. The way it is now you can add and remove instrumentation without having to change your application code.

Anyways, I think we can do what you want to do by including some metadata in either the RSocket or the DuplexConnection that tells your where it's from. The plugin you implement could then filter based on the metadata provided, etc.

yschimke commented 7 years ago

@robertroeser what about existing support for GLOBAL plugins with minimal tidy up e.g. an Add function instead of clients maintaining the chain themselves.

encapsulating "public static volatile DuplexConnectionInterceptor DUPLEX_CONNECTION_INTERCEPTOR"

But also optional support in setup for per client features. No matter what metadata we add to the global plugin mechanism, it will never be as simple or as obvious to add a plugin for a specific client as through setup.

I think that is a perfectly valid use case and something I want to do right now even. I'd like to use this mechanism to tamper with a single setup frame (change to an unsupported version) and see that it causes the failure I expect in an integration test.

robertroeser commented 7 years ago

You can wrap RSocket you get back from a factory per server, and you can wrap DuplexConnection you get from a Transport. We can add this to the factory when you create something, but you can do this already by just wrapping these objects. The plugin was so you can add and remove this without having to change code. If you add this per server to add/remove this you will with have to go and change setup code.

yschimke commented 7 years ago

That's what we are currently doing internally. I still feel having a clean documented API to do it as part of setup is worthwhile. Likewise, I still think minimally encapsulating the global plugins is useful also. I'm not looking to remove them.

yschimke commented 7 years ago

Consider it just hand holding that makes common patterns more obvious. Something like

  private final OkHttpClient client = new OkHttpClient.Builder()
      .addInterceptor(new LoggingInterceptor())
      .build();
robertroeser commented 7 years ago

Ok that makes sense - clarity in the API - something like this ?

Function<RSocket, Mono<RSocket> interceptor = ...
Function<DuplexConnection, Mono<DuplexConnection> connectionInterceptor = ...

RSocketFactory
.intercept(interceptor)
.interceptConnection(connectionInterceptor)...
yschimke commented 7 years ago

https://github.com/rsocket/rsocket-java/pull/306

@robertroeser I like your method suggestions, I'll adjust that soon.