Remove UPayload #108
Remove UPayload #108
Conversation
AnotherDaniel
force-pushed
the
remove_upayload
branch
from
6501f29
to
e3c6808
Compare
There was a problem hiding this comment.
LGTM.
Just one question: Should we pin at a specific commit of up-spec instead of main in build.rs? When the up-spec (up-core-api) changes, it'll break the up-rust and affect other libraries depending on up-rust.
I think it'll be better to fix it at a specific commit and keep updating regularly.
Actually, that is the main intention of referring to |
src/umessage/umessagebuilder.rs
Outdated
| @@ -70,7 +37,7 @@ pub struct UMessageBuilder { | |||
| permission_level: Option<u32>, | |||
| comm_status: Option<EnumOrUnknown<UCode>>, | |||
| request_id: Option<UUID>, | |||
| payload: Option<Bytes>, | |||
| payload: Option<Vec<u8>>, | |||
There was a problem hiding this comment.
what is the reason for changing this? FMPOV Bytes is much more flexible when it comes to writing/reading and does require much less copying ...
src/umessage/umessagebuilder.rs
Outdated
| ) | ||
| }) | ||
| ) -> Result<UMessage, UMessageError> { | ||
| if !(self.payload_format == UPayloadFormat::UPAYLOAD_FORMAT_PROTOBUF |
There was a problem hiding this comment.
where is self.payload_format set? Before your change, the payload format always was UPayloadFormat::UPAYLOAD_FORMAT_PROTOBUF. If we now want to also support UPayloadFormat::UPAYLOAD_FORMAT_PROTOBUF_WRAPPED_IN_ANY, then we either need to pass in the payload format as a param, add a dedicated function for building with wrapped payload or detect if the passed in payload actually is an Any. FMPOV we should go with option 2, making the whole thing very explicit and also making it very easy to later deprecate and remove support for it again ...
The UPayload structure has been removed from UMessage. The code has been adapted accordingly. Also-by: Kai Hudalla <kai.hudalla@bosch.com>
The Protobuf crate supports using Bytes instead of Vec<u8> when parsing byte arrays from protobuf structs. This can help reduce copying of data shared by multiple consumers of messages. Protobuf's support for using Bytes has been enabled. UMessageBuilder's functions for building with payload have been adapted to accept any payload that can be converted to Bytes.
sophokles73
force-pushed
the
remove_upayload
branch
from
e3c6808
to
a15872c
Compare
|
@evshary Would you mind taking a look at the changes that I have pushed in addition to Daniel's original PR? @AnotherDaniel is on vacation this week and we agreed to get this merged this week ... |
|
@evshary can you check if this (still) works for you? |
Yes, I give my pass to it. |
Following up-spec #145, remove UPayload from the rust library. This let's us get rid of RpcMapper entirely, which is nice. For now I did not 'save' the
RpcMapper::pack_any()andRpcMapper::unpack_any()methods, because they are not needed anywhere I can see and also have little to do with the actual uProtocol scope. Shout if you're missing them nonetheless!