Skip to content

Add ProtoBuf I/O Stream support on jvm (#2075) #2082

New issue

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

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

Already on GitHub? # to your account

Closed

Conversation

GeorgePap-719
Copy link
Contributor

Add implementations for streaming support in Protobuf format supporting simple and delimited messages.

Add implementations for streaming support in Protobuf format supporting
simple and delimited messages.

Signed-off-by: George Papadopoulos <george.719pap@gmail.com>
@qwwdfsad qwwdfsad requested review from shanshin and qwwdfsad and removed request for shanshin October 31, 2022 11:21
Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for the effort!

Unfortunately, we cannot accept your contribution in its current form (you can use https://github.com/Kotlin/kotlinx.serialization/blob/master/CONTRIBUTING.md for a more predictable result in the future).

In short, the first glance found the following issues:

  • At the moment, when there is more than a single way to encode/decode data, we should test both of them in each test
  • There is a new concept, decode/encode "delimited", but in JSON we already have a different approach. We prefer to be consistent within our library, so such API definitely should be discussed beforehand.
  • The API that fails at the moment when the message is greater than some pre-defined constant is just a time bomb waiting to explode in other people's production, which is a no-go
  • Streaming API implies that both encoding and decoding of data are streamed. An implementation that reads/writes to ByteArray whole message and then passes it as is to the underlying IO stream does not pass this criterion

@qwwdfsad qwwdfsad closed this Oct 31, 2022
@GeorgePap-719
Copy link
Contributor Author

Hello, thanks for the feedback much appreciate it!

yes i used the Contributing.md as base line for the pr, but since i had a draft in mind i figured i would open a pr for faster feedback not sure if it's ok or not :)

Is it alright if i continue to work this contribution based on your feedback or should i just also close the related issue?

@qwwdfsad
Copy link
Collaborator

The issue is fine -- let's gather some feedback on whether there is a demand for it.

I suggest addressing the most important point, streaming, in the issue, though it's unclear whether it is possible to do so at all due to weird protobuf nature when it's impossible to write a message unless knowing its varint-encoded size in advance

@GeorgePap-719
Copy link
Contributor Author

May i ask a stupid question? How we gather feedback? :P

Ok, i will work on streaming.

About demand not sure if this counts but:

My use case was to provide support for protobuf with kotlin's impl in spring, for example to be able to impl something like this:
https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/http/codec/protobuf/ProtobufEncoder.java

The encoder when the stream is through flux is using delimited messages. So in order to provide a compatible solution i guess the kotlin's one would need to have delimited messages (i think).

Also, recently they provided a simple integration kotlin's protobuf impl here:
https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/http/codec/protobuf/KotlinSerializationProtobufEncoder.java

But they state the encoding stream is not yet supported and they have linked the #1073

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Nov 1, 2022

How we gather feedback? :P

Mostly wait for people to pass by and leave their comments about the feature request. Common sense also plays its role here :)

The use case is perfectly valid; what puzzles me is how it is possible to make an actual streaming protobuf without reading all to byte array first

@GeorgePap-719
Copy link
Contributor Author

GeorgePap-719 commented Nov 7, 2022

After some research it seems that there is a way calculate the serialized size without encoding the message. Java's implementation does it, the only "trick" to it is that the function is generated by the compiler plugin.

The actual implementation seems simple (not sure from compiler's pov tho), at first at glance at least. Not sure if it requires code from compiler's plugin or it can be done at library lvl.

Concept:

expose a variable: val serializedSize: Int (java's implementation memoizes it) probably we can either also memoize it or implement it like lazy ?

The serialized size calculates the serialized size separately for each field and then sums it up. All methods for calculating the size are similar to computeUInt32SizeNoTag() in my branch.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants