Plugins refactor#306
Conversation
| } | ||
|
|
||
| public void addConnectionPlugin(DuplexConnectionInterceptor interceptor) { | ||
| connections.add(interceptor); |
There was a problem hiding this comment.
It doesn't look like differentiating between different server/clients but applies the same interceptor list to all server and clients..
There was a problem hiding this comment.
If you have 1 client, 1 server, then you have 3 plugin registries.
- JVM defaults via Plugins
- RSocketFactory.receive() holds one
- RSocketFactory.connect() holds one
So I think that counts.
| @@ -0,0 +1,24 @@ | |||
| package io.rsocket.plugins; | |||
|
|
|||
| /** JVM wide plugins for RSocket */ | |||
There was a problem hiding this comment.
Do you have a TODO to don't make this JVM wide?
There was a problem hiding this comment.
It's part of this PR, you can add interceptors to the RSocketFactory. But I got the impression that it may be a useful feature to apply some plugins at app startup for all clients. I'm not against that being used judiciously.
So no TODO, this was deliberate. n.b. I folded the one existing JVM wide plugin (fragmentation) in via being the default MTU for clients/servers.
| @@ -90,6 +93,8 @@ public void startup() { | |||
|
|
|||
| server = | |||
There was a problem hiding this comment.
What about creating two+ servers and two+ clients and being able to use different interceptors for them?
There was a problem hiding this comment.
I'll test that via more specific tests, e.g. a unit test of the builders that shows the right interceptors are applied. I think Integration tests are better left for testing end to end scenarios.
|
|
||
| static { | ||
| if (Boolean.getBoolean("io.rsocket.fragmentation.enable")) { | ||
| int mtu = Integer.getInteger("io.rsocket.fragmentation.mtu", 1024); |
There was a problem hiding this comment.
This is removed as its already supported in the Builder, so I can move this system property logic to become the default MTU.
If I leave it here, you get fragmentation twice if user overrides.
|
@phoad do you want to see if this works for you now? Let me know and if so I'll merge in. |
|
I have re-reviewed the update and it seems good to me. |
RFC, still hacking on this.