Thrift bool parameter gets into unknown volo_genbool type, methods started with '_' losts underscore symbol

kubroid commented 3 weeks ago

Bug Report

Encountered compilation errors when trying to compile a simple Thrift IDL using the volo crate. The errors include unknown types and incorrect handling of underscored method names, which is critical for backward compatibility with existing service definitions.

Version

cargo tree | grep -E "volo|pilota"
volo_test v0.1.0 (/home/alex/git/volo-test)
├── pilota v0.11.7
├── volo v0.10.3
├── volo-gen v0.1.0 (/home/alex/git/volo-test/volo-gen)
│   ├── pilota v0.11.7 (*)
│   ├── volo v0.10.3 (*)
│   └── volo-thrift v0.10.4
│       ├── pilota v0.11.7 (*)
│       └── volo v0.10.3 (*)
│   └── volo-build v0.10.15
│       ├── pilota-build v0.11.22
│       │   ├── pilota-thrift-parser v0.11.2
│       ├── volo v0.10.3 (*)
└── volo-thrift v0.10.4 (*)

Platform

uname -a
Linux P53 5.15.0-91-generic #101-Ubuntu SMP Tue Nov 14 13:30:08 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Crates

volo, pilota

Description

Steps to Reproduce:

Create a Sample IDL File:

~/git/volo-test> tree
.
└── rts
    └── test.thrift

~/git/volo-test> cat rts/test.thrift
service Test {
    string health(
        1: bool deep_check
    )

    string _underscored(
        1: string param
    )
}

Initialize a Volo Project:

~/git/volo-test> volo init volo-test rts/test.thrift`
cargo:rerun-if-changed=/home/alex/git/volo-test/rts/test.thrift

Resulting directory structure:

~/git/volo-test [master|…6]> tree
.
├── Cargo.lock
├── Cargo.toml
├── rts
│   └── test.thrift
├── src
│   ├── bin
│   │   └── server.rs
│   └── lib.rs
└── volo-gen
    ├── build.rs
    ├── Cargo.toml
    ├── src
    │   └── lib.rs
    └── volo.yml

Review Generated Code:

In src/lib.rs, observe issues such as:

Unknown type volo_genbool.

Incorrect method naming leading to _underscored issues.


~/git/volo-test [master|…5]> cat src/lib.rs
pub struct S;

impl volo_gen::test::Test for S { async fn health( &self, _deep_check: volo_genbool, // <<< this is unknown ) -> ::core::result::Result<::pilota::FastStr, ::volo_thrift::ServerError> { ::std::result::Result::Ok(Default::default()) } async fn underscored( // <<< underscore is missing &self, _param: volo_gen::pilota::FastStr, ) -> ::core::result::Result<::pilota::FastStr, ::volo_thrift::ServerError> { ::std::result::Result::Ok(Default::default()) } }


#### Build the Project:

cargo build ... error[E0422]: cannot find struct, variant or union type Test_UnderscoredArgsSend in this scope --> /home/alex/git/volo-test/target/debug/build/volo-gen-47336832199418a2/out/volo_gen.rs:1112:57 | 144 | pub struct TestUnderscoredArgsSend { | ---------------------------------- similarly named struct TestUnderscoredArgsSend defined here ... 1112 | let req = TestRequestSend::_Underscored(Test_UnderscoredArgsSend { param }); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: a struct with a similar name exists: TestUnderscoredArgsSend

error[E0422]: cannot find struct, variant or union type Test_UnderscoredArgsSend in this scope --> /home/alex/git/volo-test/target/debug/build/volo-gen-47336832199418a2/out/volo_gen.rs:1169:57 | 144 | pub struct TestUnderscoredArgsSend { | ---------------------------------- similarly named struct TestUnderscoredArgsSend defined here ... 1169 | let req = TestRequestSend::_Underscored(Test_UnderscoredArgsSend { param }); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: a struct with a similar name exists: TestUnderscoredArgsSend

error[E0412]: cannot find type Test_UnderscoredArgsRecv in this scope --> /home/alex/git/volo-test/target/debug/build/volo-gen-47336832199418a2/out/volo_gen.rs:1280:26 | 447 | pub struct TestUnderscoredArgsRecv { | ---------------------------------- similarly named struct TestUnderscoredArgsRecv defined here ... 1280 | _Underscored(Test_UnderscoredArgsRecv), | ^^^^^^^^^^^^^^^^^^^^^^^^ help: a struct with a similar name exists: TestUnderscoredArgsRecv

error[E0412]: cannot find type Test_UnderscoredArgsSend in this scope --> /home/alex/git/volo-test/target/debug/build/volo-gen-47336832199418a2/out/volo_gen.rs:1286:26 | 144 | pub struct TestUnderscoredArgsSend { | ---------------------------------- similarly named struct TestUnderscoredArgsSend defined here ... 1286 | _Underscored(Test_UnderscoredArgsSend), | ^^^^^^^^^^^^^^^^^^^^^^^^ help: a struct with a similar name exists: TestUnderscoredArgsSend


Decoder part from out/volo_gen.rs still has an underscore in name:

        fn decode<T: ::pilota::thrift::TInputProtocol>(
            __protocol: &mut T,
            msg_ident: &::pilota::thrift::TMessageIdentifier,
        ) -> ::core::result::Result<Self, ::pilota::thrift::ThriftException> {
            ::std::result::Result::Ok(match &*msg_ident.name {
                "health" => Self::Health(::pilota::thrift::Message::decode(__protocol)?),

                "_underscored" => { // <<< underscore is still here!

                    Self::_Underscored(::pilota::thrift::Message::decode(__protocol)?)
                }
                _ => {
                    return ::std::result::Result::Err(
                        ::pilota::thrift::new_application_exception(
                            ::pilota::thrift::ApplicationExceptionKind::UNKNOWN_METHOD,
                            format!("unknown method {}", msg_ident.name),
                        ),
                    );
                }
            })
        }


After removing `_` from IDL method definition we can see another error

~/git/volo-test [master	…6]> cargo build Compiling volo-gen v0.1.0 (/home/alex/git/volo-test/volo-gen) Compiling volo_test v0.1.0 (/home/alex/git/volo-test) error[E0433]: failed to resolve: could not find `pilota` in `volo_gen` --> src/lib.rs:12:27
12	_param: volo_gen::pilota::FastStr,
^^^^^^ could not find `pilota` in `volo_gen`

error[E0412]: cannot find type volo_genbool in this scope --> src/lib.rs:6:22 | 6 | _deep_check: volo_genbool, | ^^^^^^^^^^^^ not found in this scope



The ability to correctly parse and handle IDLs with underscored method names is crucial for maintaining compatibility with existing production services. Adjustments to Thrift IDLs for naming conventions without underscores are not feasible due to service dependencies.

If further context or any specific file changes are required, please let me know so I can assist with providing them.

PureWhiteWu commented 3 weeks ago

PTAL @Ggiggle @Millione

kubroid commented 2 weeks ago

Guys, you are the best!! Thanks for such fast fix.

kubroid commented 2 weeks ago

There is one more issue i found - pilota-build crashes in case meet semicolon after typedef:

typedef string ScoringMetadataLabel;

service Test {
    string health(
        1: bool deep_check;
    );
    string _underscored(
        1: string param;
    );
}

leads to

  thread 'main' panicked at /home/alex/.cargo/registry/src/index.crates.io-6f17d22bba15001f/pilota-build-0.11.23/src/parser/thrift/mod.rs:34:53:
  called `Result::unwrap()` on an `Err` value: Error(Error { input: ";\n\nservice Test {\n    string health(\n        1: bool deep_check;\n    );\n    string _underscored(\n        1: string param;\n    );\n}\n", code: Satisfy })

And after service definition:

  service Test {
    string health(
        1: bool deep_check;
    );
    string _underscored(
        1: string param;
    );
};

  thread 'main' panicked at /home/alex/.cargo/registry/src/index.crates.io-6f17d22bba15001f/pilota-build-0.11.23/src/parser/thrift/mod.rs:34:53:
  called `Result::unwrap()` on an `Err` value: Error(Error { input: ";\n", code: Satisfy })

Ggiggle commented 2 weeks ago

Currently there is indeed a lack of handling of semicolons or commas at the end when parsing typedef or struct, thanks for discovering, we will fix it later. But I only found the typedef example using the semicolon or comma at the end, can you guide where to confirm there are similar syntaxes for other types, such as struct and service.

PureWhiteWu commented 2 weeks ago

Thanks very much for your report!

https://thrift.apache.org/docs/idl

According to the thrift idl spec, semicolon (ListSeparator) is not allowed after typedef and service.

But I think we can also support such usage later since it's more flexible for users and I think it will not lead to confusions.

For now, maybe you can just remove the semicolon and try again?

kubroid commented 2 weeks ago

Thanks! We just has semicolons in that places and thrift and thriftpy2 have no issues to compile it. Of course i can remove it.

kubroid commented 2 weeks ago

Sorry to bother you again, I tried to compile our company thrift IDLs and now see other issues:

# some subset of definitions
struct TSchemaField {
    1: required string name;
    6: optional map<string, list<string>> selectors;
}

service Test {
    string health(
        1: bool deep_check
    )
    string test_map(
        1: required string name;
        6: optional set<TSchemaField> fields;
    )
}

cargo tree | grep -E "volo|pilota"
volo_test v0.1.0 (/home/alex/git/volo-test)
├── pilota v0.11.7
├── volo v0.10.3
├── volo-gen v0.1.0 (/home/alex/git/volo-test/volo-gen)
│   ├── pilota v0.11.7 (*)
│   ├── volo v0.10.3 (*)
│   └── volo-thrift v0.10.4
│       ├── pilota v0.11.7 (*)
│       └── volo v0.10.3 (*)
│   └── volo-build v0.10.16
│       ├── pilota-build v0.11.23
│       │   ├── pilota-thrift-parser v0.11.3
│       ├── volo v0.10.3 (*)
└── volo-thrift v0.10.4 (*)

    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1077:13
     |
1073 |         #[derive(Debug, Default, Clone, PartialEq)]
     |                                         --------- in this derive macro expansion
...
1077 |             pub fields: ::pilota::AHashSet<TSchemaField>,
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
note: the following type would have to `impl` its required traits for this operation to be valid
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:6:9
     |
6    |         pub struct TSchemaField {
     |         ^^^^^^^^^^^^^^^^^^^^^^^
     |         |
     |         must implement `Hash`
     |         must implement `std::cmp::Eq`
     = note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider annotating `TSchemaField` with `#[derive(Eq, Hash, PartialEq)]`
     |
6    +         #[derive(Eq, Hash, PartialEq)]
7    |         pub struct TSchemaField {
     |

error[E0277]: the trait bound `TSchemaField: std::cmp::Eq` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1140:45
     |
1140 | ...                   val.insert(::pilota::thrift::Message::decode(__protocol)?);
     |                           ^^^^^^ the trait `std::cmp::Eq` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:8
     |
429  |     T: Eq + Hash,
     |        ^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function
help: consider annotating `TSchemaField` with `#[derive(Eq)]`
     |
6    +         #[derive(Eq)]
7    |         pub struct TSchemaField {
     |

error[E0277]: the trait bound `TSchemaField: Hash` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1140:45
     |
1140 | ...                   val.insert(::pilota::thrift::Message::decode(__protocol)?);
     |                           ^^^^^^ the trait `Hash` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:13
     |
429  |     T: Eq + Hash,
     |             ^^^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function

error[E0277]: the trait bound `TSchemaField: std::cmp::Eq` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1223:29
     |
1223 |                         val.insert(<TSchemaField as ::pilota::thrift::Message>::decode_async(__protocol).await?);
     |                             ^^^^^^ the trait `std::cmp::Eq` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:8
     |
429  |     T: Eq + Hash,
     |        ^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function
help: consider annotating `TSchemaField` with `#[derive(Eq)]`
     |
6    +         #[derive(Eq)]
7    |         pub struct TSchemaField {
     |

error[E0277]: the trait bound `TSchemaField: Hash` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1223:29
     |
1223 |                         val.insert(<TSchemaField as ::pilota::thrift::Message>::decode_async(__protocol).await?);
     |                             ^^^^^^ the trait `Hash` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:13
     |
429  |     T: Eq + Hash,
     |             ^^^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function

error[E0369]: binary operation `==` cannot be applied to type `AHashSet<TSchemaField>`
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1446:13
     |
1442 |         #[derive(Debug, Default, Clone, PartialEq)]
     |                                         --------- in this derive macro expansion
...
1446 |             pub fields: ::pilota::AHashSet<TSchemaField>,
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
note: the following type would have to `impl` its required traits for this operation to be valid
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:6:9
     |
6    |         pub struct TSchemaField {
     |         ^^^^^^^^^^^^^^^^^^^^^^^
     |         |
     |         must implement `Hash`
     |         must implement `std::cmp::Eq`
     = note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider annotating `TSchemaField` with `#[derive(Eq, Hash, PartialEq)]`
     |
6    +         #[derive(Eq, Hash, PartialEq)]
7    |         pub struct TSchemaField {
     |

error[E0277]: the trait bound `TSchemaField: std::cmp::Eq` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1509:45
     |
1509 | ...                   val.insert(::pilota::thrift::Message::decode(__protocol)?);
     |                           ^^^^^^ the trait `std::cmp::Eq` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:8
     |
429  |     T: Eq + Hash,
     |        ^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function
help: consider annotating `TSchemaField` with `#[derive(Eq)]`
     |
6    +         #[derive(Eq)]
7    |         pub struct TSchemaField {
     |

error[E0277]: the trait bound `TSchemaField: Hash` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1509:45
     |
1509 | ...                   val.insert(::pilota::thrift::Message::decode(__protocol)?);
     |                           ^^^^^^ the trait `Hash` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:13
     |
429  |     T: Eq + Hash,
     |             ^^^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function

error[E0277]: the trait bound `TSchemaField: std::cmp::Eq` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1592:29
     |
1592 |                         val.insert(<TSchemaField as ::pilota::thrift::Message>::decode_async(__protocol).await?);
     |                             ^^^^^^ the trait `std::cmp::Eq` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:8
     |
429  |     T: Eq + Hash,
     |        ^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function
help: consider annotating `TSchemaField` with `#[derive(Eq)]`
     |
6    +         #[derive(Eq)]
7    |         pub struct TSchemaField {
     |

error[E0277]: the trait bound `TSchemaField: Hash` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1592:29
     |
1592 |                         val.insert(<TSchemaField as ::pilota::thrift::Message>::decode_async(__protocol).await?);
     |                             ^^^^^^ the trait `Hash` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:13
     |
429  |     T: Eq + Hash,
     |             ^^^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function

Commenting 6: optional map<string, list<string>> selectors; line "solves" the issue.

error[E0433]: failed to resolve: could not find `pilota` in `volo_gen`
  --> src/lib.rs:12:26
   |
12 |         _name: volo_gen::pilota::FastStr,
   |                          ^^^^^^ could not find `pilota` in `volo_gen`

error[E0433]: failed to resolve: could not find `pilota` in `volo_gen`
  --> src/lib.rs:13:28
   |
13 |         _fields: volo_gen::pilota::AHashSet<::test::TSchemaField>,
   |                            ^^^^^^ could not find `pilota` in `volo_gen`

error[E0412]: cannot find type `volo_genbool` in this scope
 --> src/lib.rs:6:22
  |
6 |         _deep_check: volo_genbool,
  |                      ^^^^^^^^^^^^ not found in this scope

error[E0433]: failed to resolve: could not find `test` in the list of imported crates
  --> src/lib.rs:13:47
   |
13 |         _fields: volo_gen::pilota::AHashSet<::test::TSchemaField>,
   |                                               ^^^^ could not find `test` in the list of imported crates
   |
help: consider importing this module
   |
1  + use volo_gen::test;
   |
help: if you import `test`, refer to it directly
   |
13 -         _fields: volo_gen::pilota::AHashSet<::test::TSchemaField>,
13 +         _fields: volo_gen::pilota::AHashSet<test::TSchemaField>,
   |

PureWhiteWu commented 2 weeks ago

Thanks very much for your report, we will investigate this asap.

kubroid commented 2 weeks ago

Currently there is indeed a lack of handling of semicolons or commas at the end when parsing typedef or struct, thanks for discovering, we will fix it later. But I only found the typedef example using the semicolon or comma at the end, can you guide where to confirm there are similar syntaxes for other types, such as struct and service.

i have a feeling that semicolon could be almost on each line, especially if author is c++ guy :) Found it after typedefs, struct/unions fields, methods with and without throws:

    TReindexStatus get_reindex_status(
        1: common.SiteId site_id,
    ) throws (1: common.TServiceException e);

never after struct definitions (closing '}')

PureWhiteWu commented 2 weeks ago

The semicolon(ListSeparator) has been supported in https://github.com/cloudwego/pilota/pull/284. We are now investigating the new issue.

PureWhiteWu commented 2 weeks ago

Hi, sorry for the late reply, I found that the issue is because there's a map<string, list<string>> that is not comparable in TSchemaField, which is used in the param set<TSchemaField> that requires the inner element comparable.

Can this idl be compiled successfully in other languages such as go/cpp?

kubroid commented 2 weeks ago

Hi, sorry for the late reply, I found that the issue is because there's a map<string, list<string>> that is not comparable in TSchemaField, which is used in the param set<TSchemaField> that requires the inner element comparable.

Can this idl be compiled successfully in other languages such as go/cpp?

yes, thrift c++ and rs compile it well. also thift async is good.

PureWhiteWu commented 2 weeks ago

let me try using thrift rs

kubroid commented 2 weeks ago

It could be even worse, like:

    7: optional map<string, list< map<string, list<string>>>> transforms;

PureWhiteWu commented 2 weeks ago

I've found that, in thrift-rs, they use BTreeMap and BTreeSet which implements PartialEq, Eq, PartialOrd, Ord that makes it able to be used in set/map. But in fact, the implementation is very inefficient, as it will iterator all the elements to calc the hash and compare the value.

For now, pilota doesn't support using BTreeMap and BTreeSet for map and set, we uses AHashMap/AHashSet for much better performance.

Supporting BTreeMap and BTreeSet in those cases are not impossible in pilota, but may take some time.

Could you modify your idl to avoid this limitation for now?

kubroid commented 2 weeks ago

Thanks for your investigation. Unfortunately this is a part of production IDL currently in use by multiple services, so i cant change it. I'll try to remove it for my test app just to see if it fly.

kubroid commented 2 weeks ago

Still there are few erros after parsing:

_deep_check: volo_genbool,
_exclude: volo_gen::pilota::AHashSet<::resources::Id>, instead of _exclude: ::pilota::AHashSet<volo_gen::resources::Id>,
_ids: volo_gen::std::vec::Vec<::resources::Id>, instead of _ids: ::std::vec::Vec<volo_gen::resources::Id>,


async fn get_candidates(
    &self,
    ...
    ) -> ::core::result::Result<
    ::volo_thrift::MaybeException<
        volo_gen::candidates::TCandidateList,
        CandidatesGetCandidatesException, <<< volo_gen::candidates:: is missing
    >,
    ::volo_thrift::ServerError,
> {
    ::std::result::Result::Ok(Default::default())
}```

Default is not implemented for MaybeException:

error[E0277]: the trait bound `MaybeException<Vec<TJobInfo>, CandidatesJobInfoException>: Default` is not satisfied
--> src/lib.rs:298:35
|
298 |         ::std::result::Result::Ok(Default::default())
|                                   ^^^^^^^^^^^^^^^^^^ the trait `Default` is not implemented for `MaybeException<Vec<TJobInfo>, CandidatesJobInfoException>`

I'll prepare a test IDLs for you.

PureWhiteWu commented 2 weeks ago

Hi, thanks very much for your quick response! We will investigate those problems asap. Are you planning to use volo in your production environment?

If so, we can try to support your usage faster.

kubroid commented 2 weeks ago

It's still a bit early to discuss production plans. Currently, I'm working on migrating one of our Python projects to Rust. At this stage, I’m gathering information on libraries, frameworks, usability, support, and other relevant factors.

I have one implementation in place that uses an async Thrift library, and so far, it’s showing promising results. However, when I combine it with another library (async Aerospike), the Aerospike library’s performance is unexpectedly poor—even slower than Python! I haven't yet identified the root cause of this performance issue.

Since I’m not yet an expert in Rust, there’s a chance I may have overlooked something in my code here. To address this, I plan to test Volo, as it’s written purely in Rust, uses more advanced techniques, and in preliminary tests, it appears to offer better performance and lower CPU usage.

PureWhiteWu commented 2 weeks ago

Hi, the two issues you mentioned before should be fixed now, could you please cargo update and try again?

PureWhiteWu commented 2 weeks ago

For the BTreeMap and BTreeSet issue, do you have time to support this? @Millione I think maybe we can support users to switch to these two types by pilota annotation?

Millione commented 2 weeks ago

@PureWhiteWu Yhea, I will implement it as soon as possible. Maybe just introduce a feature to get this done?

kubroid commented 2 weeks ago

cat rts/test.thrift
service Test {
    string health(
        1: bool deep_check
    )

    string _underscored(
        1: string param
    )
}

volo init volo-test rts/test.thrift
cargo:rerun-if-changed=/home/alex/git/volo-test/rts/test.thrift

cargo update
    Updating crates.io index
     Locking 0 packages to latest compatible versions
note: pass `--verbose` to see 36 unchanged dependencies behind latest

cargo tree | grep -E "volo|pilota"
test v0.1.0 (/home/alex/git/volo-test)
├── pilota v0.11.7
├── volo v0.10.3
├── volo-gen v0.1.0 (/home/alex/git/volo-test/volo-gen)
│   ├── pilota v0.11.7 (*)
│   ├── volo v0.10.3 (*)
│   └── volo-thrift v0.10.5
│       ├── pilota v0.11.7 (*)
│       └── volo v0.10.3 (*)
│   └── volo-build v0.10.17
│       ├── pilota-build v0.11.25
│       │   ├── pilota-thrift-parser v0.11.5
│       ├── volo v0.10.3 (*)
└── volo-thrift v0.10.5 (*)

cargo --version
cargo 1.81.0 (2dbb1af80 2024-08-20)
rustc --version
rustc 1.81.0 (eeb90cda1 2024-09-04)

cargo build
...
error[E0407]: method `underscored` is not a member of trait `volo_gen::test::Test`
  --> src/lib.rs:10:5
   |
10 |       async fn underscored(
   |       ^        ----------- help: there is an associated function with a similar name: `_underscored`
   |  _____|
   | |
11 | |         &self,
12 | |         _param: volo_gen::pilota::FastStr,
13 | |     ) -> ::core::result::Result<::pilota::FastStr, ::volo_thrift::ServerError> {
14 | |         ::std::result::Result::Ok(Default::default())
15 | |     }
   | |_____^ not a member of trait `volo_gen::test::Test`

error[E0433]: failed to resolve: could not find `pilota` in `volo_gen`
  --> src/lib.rs:12:27
   |
12 |         _param: volo_gen::pilota::FastStr,
   |                           ^^^^^^ could not find `pilota` in `volo_gen`

error[E0412]: cannot find type `volo_genbool` in this scope
 --> src/lib.rs:6:22
  |
6 |         _deep_check: volo_genbool,
  |                      ^^^^^^^^^^^^ not found in this scope

error[E0046]: not all trait items implemented, missing: `_underscored`
 --> src/lib.rs:3:1
  |
3 | impl volo_gen::test::Test for S {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `_underscored` in implementation
  |
  = help: implement the missing item: `fn _underscored(&self, _: FastStr) -> impl Future<Output = Result<FastStr, ServerError>> + Send { todo!() }`

PureWhiteWu commented 2 weeks ago

@PureWhiteWu Yhea, I will implement it as soon as possible. Maybe just introduce a feature to get this done?

I don't think a feature is enough, because the codec part in pilota also needs to add extra methods such as write_btree_set

PureWhiteWu commented 2 weeks ago

Oh you need to re-install your volo-cli @kubroid

kubroid commented 2 weeks ago

Oh you need to re-install your volo-cli @kubroid

Sorry, after update all looks good!! The only thing missing is a volo_gen namespace for custom(?) exceptions

common.rs
exception TServiceException {
...
}

include "common.thrift"
service Candidates {
...
  list<common.SiteId> cleanup_indices() throws (1: common.TServiceException e)
}

    async fn cleanup_indices(
        &self,
    ) -> ::core::result::Result<
        ::volo_thrift::MaybeException<
            ::std::vec::Vec<volo_gen::common::SiteId>,
            CandidatesCleanupIndicesException,     <<<<<<<<<<<<
        >,
        ::volo_thrift::ServerError,
    > {
        ::std::result::Result::Ok(Default::default())
    }

PureWhiteWu commented 2 weeks ago

@kubroid Yep, this is also a bug of the generated template, could you manually import the Exception type in your code for now? We will try to fix this later.

kubroid commented 2 weeks ago

Sure, i've already did. Thanks for such an agile problem fixing!!!

PureWhiteWu commented 2 weeks ago

Are you able to run your application without any problems now?

kubroid commented 2 weeks ago

Yes, its runs now. I need to implement all internal things. Thanks you!!!

I have one, maybe silly question related to thrift client.

At this moment i have something like this for tests:

pub struct S {
    pub client: volo_gen::candidates::CandidatesClient,
}
...
    volo_gen::candidates::CandidatesServer::new(S {
        client: {
            let addr = "127.0.0.1:7070".parse::<std::net::SocketAddr>().unwrap();

            volo_gen::candidates::CandidatesClientBuilder::new("service_name")
                .make_codec(volo_thrift::codec::DefaultMakeCodec::framed())
                .address(addr)
                .build()
        },
    })
    .run(addr)
    .await
    .unwrap();

but in real life 127.0.0.1:7070 value will come from a config as some.dns.name:port, so i need to resolve it, which gives me a list of endpoints for load sharing.

Make client for each call seems expensive.

What you think is a best way to handle this?

Sorry, this is completely out of the scope of current issue.

PureWhiteWu commented 2 weeks ago

Hi, for your use case, you can implement a discover according to this doc: https://www.cloudwego.io/docs/volo/guide/discovery_lb/

Do we have a dns resolver out of box now? @Millione

Feel free to raise any questions.

kubroid commented 2 weeks ago

@PureWhiteWu thank you for your previous assistance! I have another request regarding the following Thrift definition:

    void filters_changed(
        1: common.SiteId site_id,
        2: optional list<string> filter_names,
       99: optional common.TraceParent traceparent,
    )

produces

    async fn filters_changed(
        &self,
        _site_id: volo_gen::common::SiteId,
        _filter_names: ::std::vec::Vec<::pilota::FastStr>,
        _traceparent: volo_gen::common::TraceParent,
    ) -> ::core::result::Result<(), ::volo_thrift::ServerError> {
        ::std::result::Result::Ok(Default::default())
    }

In this case, traceparent is treated as a non-optional value. However, we have logic that heavily relies on the presence, absence, or specific value of this parameter.

Unfortunately, due to the behavior of Python (specifically ThriftPy2), required or default fields can be omitted in the protocol if they are None. This leads to some complications in our call logic, as traceparent can be None, an empty string (""), or contain specific values (e.g., "some value").

Is it possible to implement support for optional method parameters? Perhaps this could be controlled with an additional flag in the generator to avoid interfering with the standard implementation.

Thank you once again for your excellent support!

PureWhiteWu commented 2 weeks ago

Thanks for your feedback, you are right I think changing this to use Option is more suitable.

We will implement this asap.

Millione commented 2 weeks ago

I don't think a feature is enough, because the codec part in pilota also needs to add extra methods such as write_btree_set

Get it.

Do we have a dns resolver out of box now? @Millione

Currently I think the answer is no, using https://docs.rs/trust-dns-resolver/latest/trust_dns_resolver/ maybe helpful, and @AH-dark has mentioned that he would like to help providing something like Consul、Etcd in the future.

kubroid commented 1 week ago

Thanks @PureWhiteWu @Millione Discover + trust_dns_resolver works fine!

PureWhiteWu commented 3 days ago

Reopened since there's still an issue that needs to be solved.

kubroid commented 3 days ago

@PureWhiteWu if you relay on this, maybe its better to create a separate issue for better tracking?

Millione commented 9 hours ago

It could be even worse, like:

    7: optional map<string, list< map<string, list<string>>>> transforms;

@kubroid Hi, this get supported now by annotations in IDL, you can refer to https://github.com/cloudwego/pilota/pull/289 to see how it works.

cloudwego / volo