Use ByteBuf instead of DirectBuffer#267
Conversation
| // Lease must not be received here as this is the server end of the socket which sends leases. | ||
| return Mono.empty(); | ||
| case NEXT: | ||
| synchronized (ServerReactiveSocket.this) { |
There was a problem hiding this comment.
I'd suggest pulling this out to a synchronized method to avoid the number of places that enforce this synchronization block
| UnicastProcessor<Frame> frames = UnicastProcessor.create(); | ||
| Flux<Payload> payloads = frames | ||
| .doOnCancel(() -> { | ||
| if (connection.availability() > 0.0) { |
There was a problem hiding this comment.
tangential to this review. Wouldn't this check be better handled within the connection? it could always change between the check and the call anyway.
Specifically you don't handle it when its false anyway.
| break; | ||
| case CONNECTION_ERROR: | ||
| ex = new ConnectionException(message); | ||
| ex = new ConnectionException(StandardCharsets.UTF_8.decode(frame.getData()).toString()); |
There was a problem hiding this comment.
Be good to have a utility method for this, or frame.getDataAsUtf8String()?
| */ | ||
| public static Frame allocate(final MutableDirectBuffer directBuffer) { | ||
| return new Frame(directBuffer); | ||
| public ByteBuffer getData() { |
There was a problem hiding this comment.
What are the pros/con for keeping this as ByteBuffer? The design seems very netty centric anyway. Is there value in making this ByteBuf and then putting additional work into the Aeron specific connectors? I haven't considered whether that is possible to do efficiently. Just asking the question.
There was a problem hiding this comment.
The code for the Aeron version will basically be the same because we need to copy the data. We need the bytes outside the fragment handler so you'd have to copy them. We were trying to keep the same signature as before for payload. I thought people might care negatively if we exposed bytebuf in the signature of the payload.
There was a problem hiding this comment.
I kind of think Netty is good enough/ubiquitous enough that its ok. It's a subtle change from reactivesocket-java is a network library that includes netty implementations, to reactivesocket-java is based on netty but also supports non-netty transports.
There was a problem hiding this comment.
The main drawback of exposing ByteBuf in Payload from my point of view is that consumers of the library will have to worry about reference counting -- it would be nice to be zero copy all the way through, but I don't think it's worth the impact to usability.
|
|
||
| public PayloadImpl(String data, String metadata) { | ||
| this(fromString(data), fromString(metadata)); | ||
| this(data, Charset.defaultCharset(), metadata, Charset.defaultCharset()); |
There was a problem hiding this comment.
Does defaultCharset() make sense for a network centric application? Should we just be more UTF_* opinionated instead?
|
LGTM cc @lexs |
| switch (errorCode) { | ||
| case APPLICATION_ERROR: | ||
| ex = new ApplicationException(frame); | ||
| ex = new ApplicationException(new PayloadImpl(frame)); |
There was a problem hiding this comment.
Would the API make more sense if the Frame return a Payload object instead Payload taking a Frame?
There was a problem hiding this comment.
You're right, it would probably be better to have a payload() method on the frame.
3 participants
Replaces the dependency on Agrona with netty-buffer, which provides similar functionality but allows us to avoid copying frame data for netty based transports and properly implement frame pooling.
New vs old allocations in TcpPing, for example, shows the effect this has on allocation pressure: