-
Notifications
You must be signed in to change notification settings - Fork 26.4k
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
feat: split rpcContext #10798
feat: split rpcContext #10798
Conversation
Codecov Report
@@ Coverage Diff @@
## 3.2 #10798 +/- ##
============================================
- Coverage 64.82% 64.76% -0.06%
+ Complexity 392 391 -1
============================================
Files 1355 1356 +1
Lines 57595 57699 +104
Branches 8491 8506 +15
============================================
+ Hits 37336 37371 +35
- Misses 16272 16326 +54
- Partials 3987 4002 +15
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between #10793
0332eae
to
6c98f8e
Compare
...cluster/src/main/java/org/apache/dubbo/rpc/cluster/filter/support/ConsumerContextFilter.java
Outdated
Show resolved
Hide resolved
dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/RpcContext.java
Outdated
Show resolved
Hide resolved
dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/PenetrateAttachmentSelector.java
Outdated
Show resolved
Hide resolved
dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/RpcContext.java
Outdated
Show resolved
Hide resolved
@Override | ||
public RpcContextAttachment setObjectAttachment(String key, Object value) { | ||
if (value == null) { | ||
attachments.remove(key); | ||
} else { | ||
RpcContext.getServerResponseContext().setAttachment(key, value); | ||
attachments.put(key, value); | ||
} | ||
return this; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
override all methods in RpcContextAttachment, and proxy to serverResponse / clientResponse
|
||
@Override | ||
public String getAttachment(String key) { | ||
return RpcContext.getClientResponseContext().getAttachment(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get from serverResponse
first and then fall back to clientResponse
|
||
@Override | ||
public Object getObjectAttachment(String key) { | ||
return RpcContext.getClientResponseContext().getObjectAttachment(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get from serverResponse
first and then fall back to clientResponse
|
||
@Override | ||
public RpcContextAttachment removeAttachment(String key) { | ||
return RpcContext.getServerResponseContext().removeAttachment(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove in both serverResponse
and clientResponse
|
||
@Override | ||
public Map<String, String> getAttachments() { | ||
return RpcContext.getServerResponseContext().getAttachments(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a new map, add all attachments from clientResponse
and serverResponse
. serverResponse
should override clientResponse
.
|
||
@Override | ||
public Map<String, Object> getObjectAttachments() { | ||
return RpcContext.getServerResponseContext().getObjectAttachments(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a new map, add all attachments from clientResponse
and serverResponse
. serverResponse
should override clientResponse
.
|
||
@Override | ||
public Object get(String key) { | ||
return RpcContext.getServerResponseContext().get(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecated method. Proxy it to getAttachment
|
||
@Override | ||
public RpcContextAttachment copyOf(boolean needCopy) { | ||
return RpcContext.getServerResponseContext().copyOf(needCopy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internal method, should not be called. Throw exception directly. BTW, check the usage.
|
||
@Override | ||
protected boolean isValid() { | ||
return RpcContext.getServerResponseContext().isValid(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internal method, should not be called. Throw exception directly. BTW, check the usage.
public ContextFilter() { | ||
ApplicationModel applicationModel = ApplicationModel.defaultModel(); | ||
ExtensionLoader<PenetrateAttachmentSelector> selectorExtensionLoader = applicationModel.getExtensionLoader(PenetrateAttachmentSelector.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public ContextFilter() { | |
ApplicationModel applicationModel = ApplicationModel.defaultModel(); | |
ExtensionLoader<PenetrateAttachmentSelector> selectorExtensionLoader = applicationModel.getExtensionLoader(PenetrateAttachmentSelector.class); | |
public ContextFilter(ApplicationModel applicationModel) { | |
ExtensionLoader<PenetrateAttachmentSelector> selectorExtensionLoader = applicationModel.getExtensionLoader(PenetrateAttachmentSelector.class); |
@Override | ||
public Map<String, Object> selectReverse(Invocation invocation, RpcContextAttachment clientResponseContext, RpcContextAttachment serverResponseContext) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some test cases for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please enhance the description in https://dubbo.apache.org/zh/docs3-v2/java-sdk/advanced-features-and-usage/service/attachment
Map<String, String> attachmentMap = this; | ||
attachmentMap.putAll(getAllAttachmentMap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the purpose of these two lines
RpcContext.getServerResponseContext().setRemoteAddress(remoteAddress); | ||
RpcContext.getServerContext().setRemoteAddress(remoteAddress); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RpcContext.getServerResponseContext().setRemoteAddress(remoteAddress); | |
RpcContext.getServerContext().setRemoteAddress(remoteAddress); | |
RpcContext.getServiceContext().setRemoteAddress(remoteAddress); |
RpcContext.getServerResponseContext().setRemoteApplicationName(remoteApp); | ||
RpcContext.getServerContext().setRemoteApplicationName(remoteApp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RpcContext.getServerResponseContext().setRemoteApplicationName(remoteApp); | |
RpcContext.getServerContext().setRemoteApplicationName(remoteApp); | |
RpcContext.getServiceContext().setRemoteApplicationName(remoteApp); |
RpcContext.getServerContext().setRemoteApplicationName(remoteApp); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is the pure version