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

Add flatbuffer tonic demo #3

Merged
merged 21 commits into from
May 8, 2024
Merged

Add flatbuffer tonic demo #3

merged 21 commits into from
May 8, 2024

Conversation

zealchen
Copy link
Contributor

@zealchen zealchen commented May 2, 2024

Rationale

Add flatbuffer tonic demo.

Detailed Changes

As for the core flatbuffer codec implementation in common.rs, there may be some better solutions to manage the flatbuffer objects. AFAIK, the associated type Encode / Decode of Trait Codec has a 'static lifetime bound which means items been encoded or decoded shall not have any non static references. However flatbuffer related types always have a 'fbb lifetime bound, which I found no way to implement something like what serde do.

Test Plan

Manual test.

% cargo run --bin server
   Compiling fb_tonic_demo v0.1.0 
    Finished dev [unoptimized + debuginfo] target(s) in 2.33s
     Running `target/debug/server`
GreeterServer listening on [::1]:50051
Greetings from "Alice": "Hello~~"

% cargo run --bin client
   Compiling fb_tonic_demo v0.1.0
    Finished dev [unoptimized + debuginfo] target(s) in 0.75s
     Running `target/debug/client`
Greetings from "bob": "world~~"
'''

// items been encoded or decoded shall not have any non static references.
// However flatbuffer related types always have a 'fbb lifetime bound, I found no way to implement
// something like serde do.
pub struct FlatBufferBytes {
Copy link
Member

Choose a reason for hiding this comment

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

这里没必要单独定义这么一个结构吧?直接用 Vec<u8> 是不是也够了?拿到字节流后,上层自己去做反序列化

Copy link
Contributor Author

@zealchen zealchen May 6, 2024

Choose a reason for hiding this comment

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

Make sense. Done.

format!("Service was not ready: {}", e.into()),
)
})?;
let codec = crate::util::common::FlatBufferCodec::default();
Copy link
Member

Choose a reason for hiding this comment

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

这里这样的话,不就写死了吗?按说应该根据 content type 来决定用哪个 codec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

目前根据tonic的build逻辑看,其实生成的service代码在接口定义的部分就hardcode了和encode相关Input和Output等类型了,例如(https://github.com/zealchen/serialization-benchmark-rs/blob/main/fb_tonic_demo/src/util/fb.helloworld.Greeter.rs#L212).

感觉tonic支持的custom codec (hyperium/tonic#999) 只是编译可配置,而不是运行时可配置。

不知道这个理解对不对?

Copy link
Member

Choose a reason for hiding this comment

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

那这个我在确认下,目前这样应该也够用,但是觉得不太优雅。

.build();

tonic_build::manual::Builder::new()
.out_dir("src/util")
Copy link
Member

Choose a reason for hiding this comment

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

这种构建生成的一般不放到 git 里面,一般都是 target 里面,我们之前生成 fb 代码是这么写的:

    println!("cargo:rerun-if-changed=fbs/prom_write.fbs");
    flatc_rust::run(flatc_rust::Args {
        inputs: &[Path::new("fbs/prom_write.fbs")],
        out_dir: Path::new("target/flatbuffers/"),
        ..Default::default()
    })
    .expect("flatc");

然后在 lib.rs 里这样写

#[path = "../target/flatbuffers/prom_write_generated.rs"]
mod prom_write;

pub use prom_write::*;

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Best Practice. Done!

@jiacai2050 jiacai2050 merged commit 8e54e23 into CeresDB:main May 8, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants