modify triple unique id logic #1006
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @hui-cha, Please sign Contributor License Agreement! After you signed CLA, we will automatically sync the status of this pull request in 3 minutes. |
OrezzerO
requested changes
Jan 11, 2021
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/server/triple/TripleServer.java
Outdated
Show resolved
Hide resolved
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/server/triple/TripleServer.java
Outdated
Show resolved
Hide resolved
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/server/triple/UniqueIdInvoker.java
Outdated
Show resolved
Hide resolved
950cc39 to
d417e62
Compare
d417e62 to
a25c78b
Compare
3e55d20 to
7bd2796
Compare
Codecov Report
@@ Coverage Diff @@
## master #1006 +/- ##
============================================
+ Coverage 71.06% 71.08% +0.02%
+ Complexity 828 826 -2
============================================
Files 406 407 +1
Lines 17150 17180 +30
Branches 2671 2679 +8
============================================
+ Hits 12187 12213 +26
Misses 3591 3591
- Partials 1372 1376 +4
Continue to review full report at Codecov.
|
88f8215 to
c960d16
Compare
OrezzerO
requested changes
Jan 20, 2022
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/server/triple/TripleServer.java
Outdated
Show resolved
Hide resolved
test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/TripleServerTest.java
Outdated
Show resolved
Hide resolved
75b8705 to
823f755
Compare
EvenLjj
reviewed
Jan 24, 2022
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/server/triple/TripleServer.java
Outdated
Show resolved
Hide resolved
823f755 to
e70bc30
Compare
OrezzerO
requested changes
Jan 24, 2022
...remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapter.java
Outdated
Show resolved
Hide resolved
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/server/triple/UniqueIdInvoker.java
Outdated
Show resolved
Hide resolved
e70bc30 to
9cb497e
Compare
9cb497e to
bef836f
Compare
bef836f to
e924f72
Compare
e924f72 to
6b0fe53
Compare
JervyShi
requested changes
Feb 8, 2022
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/server/triple/UniqueIdInvoker.java
Show resolved
Hide resolved
| appResponse = (String) sofaResponse.getAppResponse(); | ||
| Assert.assertEquals(appResponse, UNIQUE_ID_TWO); | ||
|
|
||
| // Case 3: There was only one invoker in UniqueIdInvoker, can not find invoker with unique id two, |
There was a problem hiding this comment.
We need to discuss the reasonableness of case3, I think the case of uniqueId mismatch should not fallback to other uniqueId`s invoker.
There was a problem hiding this comment.
In the last implement of uniqueId for triple, a server can serve request with path /{interfaceId}/{method} or '/{interfaceId}.{uniqueId}/{method}'. A client with very old version of SOFARPC may send a request with path /{interfaceId}/{method} and without uniqueId info in header. So if there is only one invoker and uniqueId is empty, we should fallback to the only invoker.
There was a problem hiding this comment.
I think a check for if uniqueid is empty should added here.
if(StringUtils.isEmpty(uniqueId)){
// falback
}else{
// throw exception
}There was a problem hiding this comment.
OK, i will fix this
6b0fe53 to
c2e8864
Compare
c2e8864 to
77cc488
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
根据讨论，triple 协议的 unique id 需要通过请求头传递，RPC 框架根据请求头中的 uniquid id 路由到对应的业务服务，调用其实现。原有的将 unique id 拼接到请求 path 中的做法需要回退掉。
Modification:
针对 triple 请求作出如下修改