Skip to content

[To rel/1.0] Change read dispatch from async to sync#8651

Closed
JackieTien97 wants to merge 1 commit intorel/1.0from
DispatchRead
Closed

[To rel/1.0] Change read dispatch from async to sync#8651
JackieTien97 wants to merge 1 commit intorel/1.0from
DispatchRead

Conversation

@JackieTien97
Copy link
Contributor

Even for query, we still blocked to wait for dispatch result, so there is no need to use sperated thread pool to do dispatching.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug B 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 9 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum left a comment

Choose a reason for hiding this comment

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

The thread executing this function is ClientRPC-Processor, and the maximum number of ClientRPC-Processor threads in threadPool is 65535. However, The coreSize of executor threadPool is 20.

Prior to this change, if the concurrency was high enough, a maximum of 20 RPCs can be sent in parallel, and the remaining requests are blocked, and the higher the network latency, the more obvious this problem is. However, with this change, you can only send RPCS synchronously. If many instances are involved, it may take a long time to send RPCS multiple times.

For these reasons, I recommend using Thrift AsyncClient. It can both send multiple RPCs in parallel within the current thread without introducing any thread pool, and it can lay the foundation for more refined thread resource control in the future

Although cumbersome, the significant performance gains on this critical path are well worth it.

@HTHou HTHou deleted the DispatchRead branch April 8, 2025 11:07
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