-
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(triple): Dubbo triple&rest protocol convergense #13607
feat(triple): Dubbo triple&rest protocol convergense #13607
Conversation
566f6db
to
00679b1
Compare
4494714
to
56c928a
Compare
</dependency> | ||
<dependency> | ||
<groupId>junit</groupId> | ||
<artifactId>junit</artifactId> |
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 test scope
@@ -1,63 +0,0 @@ | |||
/* |
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.
Why remove these classes?
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.
When analyzing the code, it was found that org.apache.dubbo.rpc.protocol.tri.transport.TripleHttp2FrameServerHandler
is no longer in use, so I cleaned up the related classes to facilitate code reference analysis. The test class org.apache.dubbo.reactive.CreateObserverAdapter
referred to the unnecessary ServerCallToObserverAdapter.
When it's time for the final merge, I will split the cleanup work into a separate PR.
56c928a
to
4a5d381
Compare
} | ||
|
||
@Override | ||
public boolean IsHttp2() { |
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.
lower first letter
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.
😂, confusing to write both java and csharp, I'll check them.
import org.apache.dubbo.remoting.http12.HttpMetadata; | ||
import org.apache.dubbo.remoting.http12.HttpResponse; | ||
|
||
@Activate |
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.
Would scope bean be better?
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.
Would scope bean be better?
Consider adapting to the servlet model when the user includes the servlet API, enabling support for scenarios requiring the servlet system.
fdf421d
to
60848cf
Compare
* When the data accessed for the first time, add it to history list. If the size of history list reaches max capacity, eliminate the earliest data (first in first out). | ||
* When the data already exists in the history list, and be accessed for the second time, then it will be put into cache. | ||
* | ||
* </p> | ||
* TODO, consider replacing with ConcurrentHashMap to improve performance under concurrency | ||
*/ | ||
public class LRU2Cache<K, V> extends LinkedHashMap<K, V> { |
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.
LRU2Cache 和 LRUCache 历史上存在过并发或者是内存泄漏问题,如果需要使用的话这部分需要验证下
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.
Ok,只是加了一个computeIfAbsent,这个之前没重写缺了锁
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.
这个变动如果跟这个PR没有关联,可以单独提个PR。
import org.apache.dubbo.remoting.http12.HttpChannel; | ||
import org.apache.dubbo.remoting.http12.RequestMetadata; | ||
|
||
@SPI(value = CommonConstants.TRIPLE, scope = ExtensionScope.FRAMEWORK) |
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.
对于只有一个实现的 SPI,直接使用 ScopeBean 的模式去处理
import org.apache.dubbo.remoting.http12.HttpResponse; | ||
import org.apache.dubbo.remoting.http12.message.HttpMessageAdapterFactory; | ||
|
||
@Activate(order = -100, onClass = "javax.servlet.http.HttpServletRequest") |
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.
- HttpServletRequest 目前是不是还没有实现
- 这里使用 Activate 机制的模式有点奇怪,这里应该是需要基于数据源是哪种类型动态切换的吧,最好是用 Adaptive 的模式基于参数动态路由?
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.
这个主要是希望能够支持servlet生态,在应用依赖里面有servlet包的时候,直接适配成HttpServletRequest,这样在扩展filter里面就可以支持很多spring web和servlet的能力
}); | ||
} | ||
|
||
private void register(RequestMapping mapping, HandlerMeta handler) { |
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.
Rename to register0
would be better
|
||
import java.util.function.Supplier; | ||
|
||
public interface FluentLogger { |
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.
This change should not be in this PR.
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.
The current logger parameters are too lengthy and prone to misuse; in almost every instance, two empty strings are being passed. using fluent style api is more user-friendly. However, there are only two classes involved. While it's possible to split them, it doesn't seem particularly necessary.
} else { | ||
return values.iterator().next(); | ||
} | ||
} | ||
|
||
public static <T> T first(List<T> values) { |
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.
This can be replaced by first(Collection<T> values)
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.
It will eliminate one class type check and as an overloaded method, it has no significant usage cost.
public static <T> Set<T> newHashSet(int expectedSize) { | ||
return new HashSet<>(capacity(expectedSize)); | ||
} | ||
|
||
public static <T> Set<T> newLinkedHashSet(int expectedSize) { | ||
return new LinkedHashSet<>(capacity(expectedSize)); | ||
} | ||
|
||
public static <T, K> Map<K, T> newHashMap(int expectedSize) { | ||
return new HashMap<>(capacity(expectedSize)); | ||
} | ||
|
||
public static <T, K> Map<K, T> newLinkedHashMap(int expectedSize) { | ||
return new LinkedHashMap<>(capacity(expectedSize)); | ||
} | ||
|
||
public static int capacity(int expectedSize) { | ||
if (expectedSize < 3) { | ||
if (expectedSize < 0) { | ||
throw new IllegalArgumentException("expectedSize cannot be negative but was: " + expectedSize); | ||
} | ||
return expectedSize + 1; | ||
} | ||
if (expectedSize < 1 << (Integer.SIZE - 2)) { | ||
return (int) (expectedSize / 0.75F + 1.0F); | ||
} | ||
return Integer.MAX_VALUE; | ||
} |
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 are the benefits of this part of the code? If not, I think we can focus on the important content of the PR itself first, and this part can be deleted first.
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.
Copy from guava, https://stackoverflow.com/questions/30220820/difference-between-new-hashmapint-and-guava-maps-newhashmapwithexpectedsizein
Will add some comments.
* When the data accessed for the first time, add it to history list. If the size of history list reaches max capacity, eliminate the earliest data (first in first out). | ||
* When the data already exists in the history list, and be accessed for the second time, then it will be put into cache. | ||
* | ||
* </p> | ||
* TODO, consider replacing with ConcurrentHashMap to improve performance under concurrency | ||
*/ | ||
public class LRU2Cache<K, V> extends LinkedHashMap<K, V> { |
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.
这个变动如果跟这个PR没有关联,可以单独提个PR。
@@ -73,4 +73,29 @@ private static synchronized void startTicker() { | |||
isTickerAlive = true; | |||
} | |||
} | |||
|
|||
public static Long parseTimeoutToMills(String timeoutVal) { |
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.
This section is recommended to be put back into the corresponding module first, it does not seem to be general enough.
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.
LRU2Cache has been updated with overwrite 'computeIfAbsent' method. This wasn't overridden before, and using it could lead to concurrency issues due to the lack of locking.
public class DefaultHttpResult<T> implements HttpResult<T> { | ||
|
||
private int status; | ||
private Map<String, List<String>> headers; |
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.
Why is the value of the header an array?
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.
For example, with something like 'set-cookie', there can be multiple instances, so it should be a list.
|
||
public class DefaultHttpResult<T> implements HttpResult<T> { | ||
|
||
private int status; |
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.
statusCode?
941ff49
to
67ce0fa
Compare
0ec19b1
to
e453da0
Compare
e453da0
to
a0263d9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.3 #13607 +/- ##
==========================================
- Coverage 41.41% 38.51% -2.90%
==========================================
Files 1750 1895 +145
Lines 73370 79258 +5888
Branches 10371 11532 +1161
==========================================
+ Hits 30388 30529 +141
- Misses 38691 44435 +5744
- Partials 4291 4294 +3 ☔ View full report in Codecov by Sentry. |
d31286d
to
3c7432d
Compare
7a19a9d
to
3816b3c
Compare
0161ebc
to
efe149f
Compare
efe149f
to
f66fed1
Compare
918cba1
to
a56c34d
Compare
|
What is the purpose of the change
See #13427,only for code review.
Brief changelog
Verifying this change
Checklist