Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide ability to publish and consume binary data directly #267

Closed
laserg opened this issue Aug 20, 2019 · 9 comments
Closed

Provide ability to publish and consume binary data directly #267

laserg opened this issue Aug 20, 2019 · 9 comments
Labels
enhancement New feature or request
Milestone

Comments

@laserg
Copy link

laserg commented Aug 20, 2019

CASE:
OutboundMessage#msg is a String,
In this way, it forces us to use marshalling (XML, JSON, YML) insted of serialization (CBOR, protobuf)
In the case of converting a binary array to we at least lose time. In addition, the string representation of the binary array will weigh twice as much since char = 16 bit vs byte = 8 bit

For instance you can compare implementtion of methods dump and dumps in kotlinx/serialization/SerialImplicits.kt kotlinx-serialization-runtime-0.11.1

@laserg
Copy link
Author

laserg commented Aug 20, 2019

Another bad thing I discovered is that your API is non-consistent in terms of data types.
As I already said OutboundMessage#msg is a String, Not so bad on its own.
But in com.viartemev.thewhiterabbit.consumer.ConfirmConsumer all methods use the following lambda (com.rabbitmq.client.Delivery) -> kotlin.Unit
And com.rabbitmq.client.Delivery#_body is a byte[]

So we publish the String and consume byte[]
CASE1: if we publish JSON (for i.e.) we consume byte representation of JSON (looks simple)
CASE2: if we publish CBOR first of all we encode it as string and we consume byte representation of what we encode.
Now it our object that was bynary encoded then represenrted as string and this string represented as byte array (looks hard)

@laserg
Copy link
Author

laserg commented Aug 21, 2019

The PR with fix is #268
it was based on v.0.0.5 as the master branch is not in production-ready state

For master com.viartemev.thewhiterabbit.channel.TxChannel.Transaction line 37 fix needed.

@viartemev
Copy link
Owner

@laserg thank you for contribution.
I totally agree with you, that OutboundMessage should have ByteArray instead of String. As a part of work on RPC implementation, I added a new RabbitMqMessage type which has only basic properties and body.
In the future, all methods of the library should operate with this type. Feel free to contribute.

@laserg
Copy link
Author

laserg commented Aug 21, 2019

Give me a couple of minutes. I will fix PR.

@viartemev
Copy link
Owner

@laserg Moving to new RabbitMqMessage type required adding Envelop field to this type also

@viartemev
Copy link
Owner

@laserg I agree with you about inconsistency. But this problem should be solved in another issue. Before that, all required parameters should be collected and after we can move to one general type.

@laserg
Copy link
Author

laserg commented Aug 21, 2019

@viartemev Yep. Now #268
Contains backward compatible API fix,

@viartemev viartemev added the enhancement New feature or request label Aug 21, 2019
@viartemev viartemev added this to the 0.0.6 milestone Aug 21, 2019
@viartemev
Copy link
Owner

viartemev commented Aug 21, 2019

This feature available in version 0.0.6

@laserg
Copy link
Author

laserg commented Aug 21, 2019

@viartemev, please do not forget to close this issue.
Thank you for fast release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants