search

risingwavelabs / risingwave

Best-in-class stream processing, analytics, and management. Perform continuous analytics, or build event-driven applications, real-time ETL pipelines, and feature stores in minutes. Unified streaming and batch. PostgreSQL compatible.
https://go.risingwave.com/slack
Apache License 2.0
6.84k stars 568 forks source link

Replace serde-protobuf and rust-protobuf with prost? #4456

Closed ice1000 closed 1 year ago

ice1000 commented 2 years ago

We're using 3 different protobuf crates at the very same time (🤯). Maybe we want to only use prost, as it's supported by:

Plus, the code generated by prost is more friendly to move semantics.

P.S. PingCAP tried to migrate from rust-protobuf to prost in 2019, but the runtime performance didn't change much (*). However, there are still good reasons to migrate:

  1. Less dependencies (faster build time)
  2. Rust-protobuf generates bloated code that slows down compilation and takes lots of time to optimize (even faster build time)

*: I blame the migration strategy -- we wrapped the prost generated code with a rust-protobuf-like layer, and that's probably slowing down everything.

skyzh commented 2 years ago

I believe the dependency can be removed as long as we go through the protobuf decoding thing in connectors. cc @tabVersion may take a look if you have time :)

BugenZhao commented 2 years ago

Do we really use rust-protobuf in gRPC? I think it should have been replaced with prost long ago. 🥵

ice1000 commented 2 years ago

Do we really use rust-protobuf in gRPC? I think it should have been replaced with prost long ago. 🥵

No. The dependency is only introduced in tikv's rust-prometheus (as an optional feature, which we enabled) and risingwave-source. I think we can supersede the usage in risingwave-source.

github-actions[bot] commented 1 year ago

This issue has been open for 60 days with no activity. Could you please update the status? Feel free to continue discussion or close as not planned.

ice1000 commented 1 year ago

Probably impossible

skyzh commented 1 year ago

There's https://docs.rs/prost-reflect/latest/prost_reflect/index.html, probably someone is working on this.

tabVersion commented 1 year ago

There's https://docs.rs/prost-reflect/latest/prost_reflect/index.html, probably someone is working on this.

I am looking into this and I will take the issue

ice1000 commented 1 year ago

So "probably impossible" is probably impossible then