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

Revert "Fix consumer side call DefaultExecutorRepository#getExecutor method cannot hit the cache when use the service discovery pattern " #7730

Closed
wants to merge 1 commit into from

Conversation

BurningCN
Copy link
Member

I'm sorry, There was a problem with #7717 PR before. There are some gaps in my understanding.

In consumer side,DefaultExecutorRepository#getExecutor in acquiring side parameter value is null because not RPC calls, but some of the other events, such as channelActive (use SHARED_EXECUTOR).

If an RPC call is made, InvokerInvocationHandler#invoke calls RpcContext.setRpcContext(invoker.getUrl()); So DefaultExecutorRepository#getExecutor ->InstanceAddressURL#getParameter will get the side parameter and its value is consumer

So the cached Executor can only be used if the consumer initiates the RPC call, and SHARED_EXECUTOR is used for other network events

// ====================================================================
Here is the screenshot

In consumer side, channelActive event use SHARED_EXECUTOR:
image

In consumer side, RPC call use previous cached executor:
image

…method cannot hit the cache when use the service discovery pattern "
@BurningCN
Copy link
Member Author

@AlbumenJ I am sorry for the previous PR submission, Please help to merge this revert PR In your spare time

@horizonzy thank you, Because of your reminder, this made me recheck it

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2021

Codecov Report

Merging #7730 (bb68151) into master (e845e16) will decrease coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7730      +/-   ##
============================================
- Coverage     59.28%   59.13%   -0.15%     
  Complexity      529      529              
============================================
  Files          1077     1077              
  Lines         43565    43563       -2     
  Branches       6368     6367       -1     
============================================
- Hits          25826    25760      -66     
- Misses        14879    14942      +63     
- Partials       2860     2861       +1     
Impacted Files Coverage Δ Complexity Δ
...ache/dubbo/registry/client/InstanceAddressURL.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...ng/transport/dispatcher/all/AllChannelHandler.java 62.06% <0.00%> (-27.59%) 0.00% <0.00%> (ø%)
...dubbo/common/status/support/LoadStatusChecker.java 46.15% <0.00%> (-15.39%) 0.00% <0.00%> (ø%)
...rg/apache/dubbo/remoting/utils/PayloadDropper.java 76.92% <0.00%> (-15.39%) 0.00% <0.00%> (ø%)
...ng/transport/dispatcher/WrappedChannelHandler.java 47.82% <0.00%> (-15.22%) 0.00% <0.00%> (ø%)
...bo/config/event/listener/LoggingEventListener.java 50.00% <0.00%> (-12.50%) 0.00% <0.00%> (ø%)
...in/java/org/apache/dubbo/common/utils/JVMUtil.java 81.13% <0.00%> (-11.33%) 0.00% <0.00%> (ø%)
...dubbo/remoting/exchange/support/DefaultFuture.java 78.63% <0.00%> (-8.55%) 0.00% <0.00%> (ø%)
.../remoting/transport/netty4/NettyClientHandler.java 79.66% <0.00%> (-6.78%) 0.00% <0.00%> (ø%)
...apache/dubbo/common/config/ConfigurationUtils.java 56.25% <0.00%> (-6.25%) 0.00% <0.00%> (ø%)
... and 16 more

Continue to review full report at Codecov.

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

@BurningCN
Copy link
Member Author

BurningCN commented May 11, 2021

The debug also found a problem. When we use registry:// pattern, Both RPC calls and network events such as channelActive use consumer previous cached Executors, while the service-discovery-registry:// pattern is the case I described above(RPC use cached Executors, network events use SHARED_EXECUTOR).

Which one should be the final criterion?

// ====================================================================

  • registry:// pattern use dubbo-demo-xml test case in dubbo project

  • service-discovery-registry:// pattern use dubbo-samples/dubbo-samples-cloud-native/dubbo-demo-servicediscovery-xml/ test case in dubbo-samples project

@BurningCN
Copy link
Member Author

The debug also found a problem. When we use registry:// pattern, Both RPC calls and network events such as channelActive use consumer previous cached Executors, while the service-discovery-registry:// pattern is the case I described above(RPC use cached Executors, network events use SHARED_EXECUTOR).

Which one should be the final criterion?

// ====================================================================

  • registry:// pattern use dubbo-demo-xml test case in dubbo project
  • service-discovery-registry:// pattern use dubbo-samples/dubbo-samples-cloud-native/dubbo-demo-servicediscovery-xml/ test case in dubbo-samples project

I'd like to open a issue to describe it, I temporarily close this PR

@BurningCN BurningCN closed this May 11, 2021
@BurningCN BurningCN deleted the revert_pr_#7717 branch May 14, 2021 04:11
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