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

[issue #10148]Replace sync forward request with async request #10158

Merged
merged 20 commits into from
Apr 20, 2023

Conversation

yuyijq
Copy link
Contributor

@yuyijq yuyijq commented Mar 21, 2023

For fix issue #10148.
Use async request in DistroFilter other than sync request.

@CLAassistant
Copy link

CLAassistant commented Mar 21, 2023

CLA assistant check
All committers have signed the CLA.

@yuyijq yuyijq changed the title Replace sync forward request with async request [issue #10148]Replace sync forward request with async request Mar 21, 2023
@KomachiSion KomachiSion added the status/wontfix This will not be worked on label Mar 24, 2023
@yuyijq
Copy link
Contributor Author

yuyijq commented Mar 24, 2023

@KomachiSion 请问为啥wontfix呢?
你就说说这个问题是不是真实存在的吧?

@xuechaos
Copy link
Member

@yuyijq 感谢贡献哈,这个点我们在issues里边再讨论一下,需要再考虑一下边界场景。

@xuechaos xuechaos reopened this Mar 27, 2023
@xuechaos xuechaos removed the status/wontfix This will not be worked on label Mar 27, 2023
1. Move async forward switch from sys module to naming module.
2. use nacos code style to format code.
@yuyijq yuyijq requested a review from KomachiSion March 28, 2023 04:36
1. one import per Class
2. add javadoc
@yuyijq yuyijq requested a review from KomachiSion March 29, 2023 08:45
@yuyijq
Copy link
Contributor Author

yuyijq commented Mar 31, 2023

@KomachiSion 能再看看吗

Copy link
Collaborator

@KomachiSion KomachiSion left a comment

Choose a reason for hiding this comment

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

改动点应该只剩上面那个地方了。

完了麻烦加一下UT

@yuyijq yuyijq requested a review from KomachiSion April 4, 2023 04:46
@yuyijq
Copy link
Contributor Author

yuyijq commented Apr 4, 2023

我看现在的UT可以覆盖这个改动把,我其实没有改变具体的行为

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2023

Codecov Report

Merging #10158 (050338f) into develop (953fa9e) will increase coverage by 1.97%.
The diff coverage is 63.49%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             develop   #10158      +/-   ##
=============================================
+ Coverage      51.86%   53.84%   +1.97%     
- Complexity      5098     5357     +259     
=============================================
  Files            875      879       +4     
  Lines          27791    27877      +86     
  Branches        3077     3086       +9     
=============================================
+ Hits           14414    15010     +596     
+ Misses         12043    11530     -513     
- Partials        1334     1337       +3     
Impacted Files Coverage Δ
...cos/common/http/client/NacosAsyncRestTemplate.java 0.00% <0.00%> (ø)
...nacos/core/distributed/distro/DistroConstants.java 0.00% <ø> (ø)
.../com/alibaba/nacos/naming/constants/Constants.java 50.00% <ø> (ø)
...ava/com/alibaba/nacos/naming/web/NamingConfig.java 0.00% <0.00%> (ø)
...c/main/java/com/alibaba/nacos/sys/env/EnvUtil.java 23.37% <0.00%> (+4.42%) ⬆️
...ba/nacos/core/distributed/distro/DistroConfig.java 81.81% <50.00%> (-3.90%) ⬇️
...ava/com/alibaba/nacos/naming/web/DistroFilter.java 45.56% <64.70%> (+45.56%) ⬆️
...java/com/alibaba/nacos/naming/misc/HttpClient.java 23.72% <88.23%> (+23.72%) ⬆️

... and 91 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 953fa9e...050338f. Read the comment docs.

@KomachiSion
Copy link
Collaborator

以前只有同步的方式,现在有加入了异步方式, 如果以前有测试,只是测试了同步的, 你需要加一定的UT来测试异步模式。

@KomachiSion
Copy link
Collaborator

@yuyijq 麻烦加一下异步相关代码的ut

@yuyijq
Copy link
Contributor Author

yuyijq commented Apr 16, 2023

@yuyijq 麻烦加一下异步相关代码的ut

@KomachiSion async 的ut加上了,这个DistroConfig的静态加载有点坑,搞了很久才发现是这个问题导致一直不走async forward,本地单跑一直没发现。
你再review一下吧,就是加了个测试和一个setter。

@yuyijq
Copy link
Contributor Author

yuyijq commented Apr 18, 2023

@KomachiSion 这个还有什么问题么?

@KomachiSion
Copy link
Collaborator

Sorry, I make a mistake, I found add new dependency

        <dependency>
            <groupId>org.mock-server</groupId>
            <artifactId>mockserver-netty</artifactId>
        </dependency>
        <dependency>
            <groupId>org.mock-server</groupId>
            <artifactId>mockserver-client-java</artifactId>
        </dependency>

@KomachiSion
Copy link
Collaborator

Why add these new dependencies?

@KomachiSion KomachiSion removed this from the 2.3.0 milestone Oct 19, 2023
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.

5 participants