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

Supports grpc+json and the curl tool #148

Merged
merged 11 commits into from
Aug 18, 2023
Merged

Supports grpc+json and the curl tool #148

merged 11 commits into from
Aug 18, 2023

Conversation

AdachiAndShimamura
Copy link
Contributor

Implemented support for grpc+Json, and enabled direct communication between curl tool and unary services

pub struct TripleServer<T> {
codec: T,
pub struct TripleServer<T, V> {
_pd: PhantomData<(T, V)>,
Copy link
Member

Choose a reason for hiding this comment

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

将TripleServer中 T, V 这两个泛型 改成 M1, M2,和TripleClient保持一致。注意区分哪个是request和response

{
let config =
ConsumerConfig::get_global_consumer_config(invocation.get_target_service_unique_name());
Copy link
Member

@yang20150702 yang20150702 Aug 16, 2023

Choose a reason for hiding this comment

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

这种实现方式有些不太好:配置逻辑侵入到核心代码中,并且这是一段隐藏逻辑。
根据service_name来动态地从配置文件中获取codec、compress 这个功能可以先去掉。这个功能应该在一个单独的feature里来做。
我建议:这个pr可以先支持基于ClientBuilder的静态配置的方式。将codec和compress配置项加在ClientBuilder里。另外,可以通过加载配置文件来初始化ClientBuilder,进而初始化TripleClient。

@@ -330,7 +328,7 @@ fn generate_unary<T: Method>(
method_ident: Ident,
server_trait: Ident,
) -> TokenStream {
let codec_name = syn::parse_str::<syn::Path>(CODEC_PATH).unwrap();
// let codec_name = syn::parse_str::<syn::Path>(CODEC_PATH).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

删除注释掉的代码

@yang20150702
Copy link
Member

Good work. Thanks for your contribution to dubbo-rust.

@yang20150702 yang20150702 merged commit 74ff5fa into apache:main Aug 18, 2023
4 checks passed
@yang20150702
Copy link
Member

#145

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.

3 participants