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

Provider supports thread pool isolation between services #10658

Merged

Conversation

BurningCN
Copy link
Member

@BurningCN BurningCN commented Sep 22, 2022

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2022

Codecov Report

Merging #10658 (b64aba6) into 3.2 (f5c8f59) will increase coverage by 0.13%.
The diff coverage is 81.81%.

@@             Coverage Diff              @@
##                3.2   #10658      +/-   ##
============================================
+ Coverage     65.30%   65.43%   +0.13%     
+ Complexity      416      392      -24     
============================================
  Files          1338     1347       +9     
  Lines         57058    57189     +131     
  Branches       8452     8464      +12     
============================================
+ Hits          37262    37422     +160     
+ Misses        15829    15806      -23     
+ Partials       3967     3961       -6     
Impacted Files Coverage Δ
...apache/dubbo/common/constants/CommonConstants.java 100.00% <ø> (ø)
...a/org/apache/dubbo/rpc/model/ApplicationModel.java 81.69% <0.00%> (ø)
...fig/bootstrap/builders/AbstractServiceBuilder.java 88.76% <0.00%> (-4.18%) ⬇️
.../protocol/dubbo/DubboIsolationExecutorSupport.java 58.82% <58.82%> (ø)
...in/java/org/apache/dubbo/config/ServiceConfig.java 70.09% <66.66%> (-0.16%) ⬇️
...he/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java 89.83% <66.66%> (-1.84%) ⬇️
.../tri/transport/TripleIsolationExecutorSupport.java 70.58% <70.58%> (ø)
.../common/threadpool/manager/ExecutorRepository.java 80.00% <80.00%> (ø)
...rpc/executor/AbstractIsolationExecutorSupport.java 82.75% <82.75%> (ø)
...ng/transport/dispatcher/WrappedChannelHandler.java 70.58% <83.33%> (+13.44%) ⬆️
... and 67 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…emoved)

2.Consumers should not involved in service thread pool isolation
@AlbumenJ AlbumenJ changed the base branch from 3.1 to 3.2 September 26, 2022 07:05
@BurningCN BurningCN force-pushed the Supports_thread_pool_isolation_between_services branch from e141895 to f15d954 Compare October 9, 2022 10:23
# Conflicts:
#	dubbo-common/src/main/java/org/apache/dubbo/common/constants/CommonConstants.java
#	dubbo-common/src/main/java/org/apache/dubbo/common/threadpool/manager/DefaultExecutorRepository.java
#	dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/bootstrap/builders/AbstractServiceBuilder.java
#	dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/protocol/AbstractInvoker.java
# Conflicts:
#	dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java
#	dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/transport/TripleHttp2FrameServerHandler.java
Copy link
Member

@AlbumenJ AlbumenJ left a comment

Choose a reason for hiding this comment

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

LGTM

}

@Override
protected ServiceKey getServiceKey(Object data) {
Copy link
Member

Choose a reason for hiding this comment

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

The servicekey seems to have been obtained in onheader, can it be reused? But there is a problem here, onheader is already in the business thread pool, you may need to pay attention

@@ -109,6 +107,8 @@ public void onDataRead(ChannelHandlerContext ctx, Http2DataFrame msg) throws Exc
}

public void onHeadersRead(ChannelHandlerContext ctx, Http2HeadersFrame msg) throws Exception {
Executor executor = executorSupport.getExecutor(msg);
Copy link
Member

Choose a reason for hiding this comment

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

I think that the calculation of the thread pool can be put into the onheader method, and the previous part of the logic can be executed in the eventloop. (Only basic judgment, no complex logic)

@AlbumenJ AlbumenJ requested a review from EarthChen October 27, 2022 03:22
Copy link
Member

@EarthChen EarthChen left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants