feat: Support remote engine server with flatbuffer write #1539

zealchen · 2024-06-29T23:12:16Z

Rationale

Our benchmarks show flatbuffers has the best perf, thus we replace the remote server write operation from protobuf to flatbuffers protocol.

#1515

Detailed Changes

Add a new flatbuffer(remote_engine.fbs) protocol schema.
Implement RemoteEngineFbService trait for RemoteEngineServiceImpl as the new flatbuffer-based service.
Make the write& batch_write interface backward compatible through the macros TonicWriteRequestExt and TonicWriteBatchRequestExt.
Add Client/Server side implementation for flatbuffer write.

Test Plan

Manual test as follows:

run horaemeta-server and two hoaredb-server on my Mac(M1) local machine.
create a partition table:

curl --location --request POST 'http://127.0.0.1:5440/sql' \
-d '
CREATE TABLE `demo`(
    `name`string TAG,
    `id` int TAG,
    `value` double NOT NULL,
    `t` timestamp NOT NULL,
    TIMESTAMP KEY(t)
    ) PARTITION BY KEY(name) PARTITIONS 2 ENGINE = Analytic
'

send write request:

curl --location --request POST 'http://127.0.0.1:15440/sql' \
-d '
INSERT INTO demo (t, name, value, id)
    VALUES (1702224000000, "horaedb", 100, 1)
'

Write request serialization benchmark test:

ANALYTIC_BENCH_CONFIG_PATH=bench.toml cargo bench -p benchmarks

ContiguousRowWriter inner type build with ByteVec:
pb encode: 131.916µs, pb decode: 63µs, fb encode: 174.459µs, fb decode: 83ns
ContiguousRowWriter inner type build with FlatbufferBuilder:
pb encode: 131.583µs, pb decode: 71.292µs, fb encode: 376.5µs, fb decode: 42ns

src/components/fb_util/src/common.rs

jiacai2050 · 2024-08-07T05:18:56Z

src/remote_engine_client/src/client.rs

-                    code: header.code,
-                    msg: header.error.clone(),
+                    code: header.code(),
+                    msg: header.error().unwrap().to_string(),


The unwrap is safe here?

Change to unwrap_or() and also update server side to response None if error message if empty.

src/server/src/grpc/remote_engine_service/mod.rs

jiacai2050 · 2024-08-07T06:27:50Z

src/server/src/grpc/remote_engine_service/mod.rs

+            .await?;
+        match result {
+            TonicWriteResponseExt::Flatbuffer(v) => Ok(v),
+            TonicWriteResponseExt::Proto(_) => Err(Status::new(Code::Internal, "logic error")),


Suggested change

TonicWriteResponseExt::Proto(_) => Err(Status::new(Code::Internal, "logic error")),

TonicWriteResponseExt::Proto(_) => Err(Status::new(Code::Internal, "only support flatbuffer payload")),

src/server/src/grpc/remote_engine_service/mod.rs

jiacai2050 · 2024-08-07T06:32:53Z

src/components/fb_util/src/common.rs

common.rs is not suitable here, the file is only designed for remote_service, so remote_service.rs maybe better name.

jiacai2050 · 2024-08-07T06:34:40Z

src/server/src/grpc/mod.rs

@@ -165,6 +166,7 @@ pub struct RpcServices {
    rpc_server: InterceptedService<StorageServiceServer<StorageServiceImpl>, AuthWithFile>,
    meta_rpc_server: Option<MetaEventServiceServer<MetaServiceImpl>>,
    remote_engine_server: RemoteEngineServiceServer<RemoteEngineServiceImpl>,
+    remote_engine_server_ext: RemoteEngineFbServiceServer<RemoteEngineServiceImpl>,


remote_engine_fb_server ?

jiacai2050 · 2024-08-07T06:36:23Z

src/remote_engine_client/src/client.rs

        })?;
-        let mut rpc_client = RemoteEngineServiceClient::<Channel>::new(route_context.channel);
+
+        let mut rpc_client = RemoteEngineFbServiceClient::<Channel>::new(route_context.channel);


Does this mean remote server will only have request with fb payload?

Adding a REQUEST_TYPE environment variable to support both protobuf and flatbuffer protocol.

src/remote_engine_client/src/client.rs

jiacai2050 · 2024-08-09T02:52:47Z

src/server/src/grpc/remote_engine_service/mod.rs

+            }
+            TonicWriteBatchRequestExt::Flatbuffer(v) => {
+                let request = Arc::new(v.into_inner());
+                let req = request.deserialize::<FBWriteBatchRequest>().unwrap();


There are several unwrap in this block, are they all safe?

FlatBufferBytes::deserialize is changed to unsafe function call as internal request routing is supposed to be safe among servers.

Other unwraps are all based on batch() which cannot be None in internal procedure.

Always follow the backwards compatibility solution for the schema design (https://flatbuffers.dev/flatbuffers_guide_tutorial.html)

jiacai2050 · 2024-08-09T02:57:25Z

src/server/src/grpc/remote_engine_service/mod.rs

+
+fn build_fb_write_response(resp: WriteResponse) -> FlatBufferBytes {
+    let mut builder = flatbuffers::FlatBufferBuilder::with_capacity(1024);
+    let header = resp.header.unwrap();


Remove this unwrap?

Change it to unwrap_or_default?

jiacai2050 · 2024-08-09T02:58:37Z

src/table_engine/src/remote/model.rs

@@ -109,6 +130,16 @@ pub struct TableIdentifier {
    pub table: String,
 }

+impl From<FBTableIdentifier<'_>> for TableIdentifier {


Impl TryFrom to remove those unwrap?

jiacai2050 · 2024-08-20T02:46:18Z

As said in description, the improvement after introduce Flatbuffer is not what we originally expected, the reason is that this PR only refactor upper layer of the put request, the ContiguousRow's encoding is unchanged, however it's where cost mainly come from, so we cannot get much perf improvement without refactor it.

But ContiguousRow is a heavily used structure in write path, refactor it would introduce much more changes, so I suggest we stop working on this, sorry for not find this earlier.

Really appreciated your work on this.

support remote engine server with flatbuffer write

b1c5869

zealchen changed the title ~~support remote engine server with flatbuffer write~~ [feat] Support remote engine server with flatbuffer write Jun 29, 2024

zealchen changed the title ~~[feat] Support remote engine server with flatbuffer write~~ feat: Support remote engine server with flatbuffer write Jun 29, 2024

github-actions bot added the feature New feature or request label Jun 29, 2024

zealchen added 2 commits June 29, 2024 21:44

add

ea7e8e7

add

8c6c62f

jiacai2050 self-requested a review July 1, 2024 09:53

zealchen added 4 commits July 2, 2024 23:32

fix ci issues

5d180b5

fix ci issues

668718f

fix ci issues

6a2716e

fix ci issues

eea24f8

jiacai2050 requested changes Aug 7, 2024

View reviewed changes

jiacai2050 reviewed Aug 7, 2024

View reviewed changes

zealchen force-pushed the refactor_fb_codec branch from fe5b4e8 to eea24f8 Compare August 8, 2024 03:27

zealchen added 4 commits August 7, 2024 22:35

Merge branch 'main' into refactor_fb_codec

6edff22

cr fix

2edb871

refactor client to support both protobuf and flatbuffer protocol

69672c4

cr fix

8c9ed4b

jiacai2050 reviewed Aug 9, 2024

View reviewed changes

src/remote_engine_client/src/client.rs Outdated Show resolved Hide resolved

jiacai2050 and others added 4 commits August 9, 2024 10:50

Update src/remote_engine_client/src/client.rs

ba4e78a

ci fix

d71618b

refactor write and add benchmark

7164e19

ci fix

9b267b4

jiacai2050 reviewed Aug 19, 2024

View reviewed changes

cr fix

99837c2

jiacai2050 closed this Aug 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support remote engine server with flatbuffer write #1539

feat: Support remote engine server with flatbuffer write #1539

zealchen commented Jun 29, 2024 •

edited

Loading

jiacai2050 Aug 7, 2024

zealchen Aug 8, 2024

jiacai2050 Aug 7, 2024

zealchen Aug 8, 2024

jiacai2050 Aug 7, 2024

zealchen Aug 8, 2024

jiacai2050 Aug 7, 2024

zealchen Aug 8, 2024

jiacai2050 Aug 7, 2024

zealchen Aug 8, 2024

jiacai2050 Aug 9, 2024

zealchen Aug 19, 2024

jiacai2050 Aug 9, 2024

zealchen Aug 19, 2024

jiacai2050 Aug 9, 2024

zealchen Aug 19, 2024

jiacai2050 commented Aug 20, 2024

	TonicWriteResponseExt::Proto(_) => Err(Status::new(Code::Internal, "logic error")),
	TonicWriteResponseExt::Proto(_) => Err(Status::new(Code::Internal, "only support flatbuffer payload")),

feat: Support remote engine server with flatbuffer write #1539

feat: Support remote engine server with flatbuffer write #1539

Conversation

zealchen commented Jun 29, 2024 • edited Loading

Rationale

Detailed Changes

Test Plan

Manual test as follows:

Write request serialization benchmark test:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiacai2050 commented Aug 20, 2024

zealchen commented Jun 29, 2024 •

edited

Loading