Fixes Payload#hasMetadata to strictly match the flag in the frame #783
Conversation
There was a problem hiding this comment.
This looks generally okay but I'd like to suggest some further improvements.
First, (the pre-existing) DataAndMetadataFlyweight#encodeOnlyMetadata I think is intended for use in frames with metadata only, and therefore it's confusing to have it in DataAndMetadataFlyweight and now conceptually overlaps with the (newly added) encode with metadata only. I propose to remove encodeOnlyMetadata. It is used only in LeaseFrameFlyweight and it's a one liner, so it can be copied there (like MetaDataPushFrameFlyweight does it).
For the remaining encode methods in DataAndMetadataFlyweight I think they should be consolidated into one method that takes both metadata and data, as well as the hasData and hasMetadata flag. This will allow encapsulating the if-else condition that is currently used to decide which encode to call form multiple places.
|
Also, following the discussion at the specification thread mentioned earlier, EmptyMetadata should be supported with |
|
Yes I like having |
Initially that issue was hidden because both sides uses limitRate which does prefetch 256 elements in advance so it is almost impossible to track underflow in request Signed-off-by: Oleh Dokuka <[email protected]>
ensures that ByteBufPayload is not accessible anymore when it has been released Signed-off-by: Oleh Dokuka <[email protected]>
Signed-off-by: Oleh Dokuka <[email protected]>
…ed correctly Right now there was observed a few issues related to that Payload with no metadata was incorrectly incoded so it results to hasMetadata true on the received side Also, optimized the API of Flyweights to ensure that we have common things in one place Signed-off-by: Oleh Dokuka <[email protected]>
Signed-off-by: Oleh Dokuka <[email protected]>
Signed-off-by: Oleh Dokuka <[email protected]>
Signed-off-by: Oleh Dokuka <[email protected]>
OlegDokuka
changed the title
Signed-off-by: Oleh Dokuka <[email protected]>
OlegDokuka
changed the title
rstoyanchev
changed the title
This PR alights with the rsocket/rsocket#302 proposal.
Also, it ensures that ByteBufPayload throws an exception when one tries to use the released instance
Signed-off-by: Oleh Dokuka [email protected]