feat(triple): Dubbo triple&rest protocol convergense #13607

oxsean · 2024-01-03T15:00:35Z

What is the purpose of the change

See #13427,only for code review.

Brief changelog

Refine Triple Transport layer code for the REST protocol support.
Added initial support for the REST protocol.

Verifying this change

Checklist

Make sure there is a GitHub_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GitHub issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
Each commit in the pull request should have a meaningful subject line and body.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Check if is necessary to patch to Dubbo 3 if you are work on Dubbo 2.7
Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
Add some description to dubbo-website project if you are requesting to add a feature.
GitHub Actions works fine on your own branch.
If this contribution is large, please follow the Software Donation Guide.

AlbumenJ · 2024-01-04T13:35:41Z

Please these compiling issues

oxsean · 2024-01-04T13:57:17Z

Please these compiling issues

Done, but just for design review at this time.

AlbumenJ · 2024-01-04T16:02:40Z

dubbo-plugin/dubbo-plugin-rest-spring/pom.xml

+    </dependency>
+    <dependency>
+      <groupId>junit</groupId>
+      <artifactId>junit</artifactId>


Add test scope

AlbumenJ · 2024-01-04T16:04:29Z

dubbo-plugin/dubbo-reactive/src/test/java/org/apache/dubbo/reactive/CreateObserverAdapter.java

@@ -1,63 +0,0 @@
-/*


Why remove these classes?

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.

AlbumenJ · 2024-01-04T16:18:24Z

...moting-http12/src/main/java/org/apache/dubbo/remoting/http12/message/DefaultHttpRequest.java

+    }
+
+    @Override
+    public boolean IsHttp2() {


lower first letter

😂, confusing to write both java and csharp, I'll check them.

AlbumenJ · 2024-01-04T16:19:25Z

...src/main/java/org/apache/dubbo/remoting/http12/message/DefaultHttpMessageAdapterFactory.java

+import org.apache.dubbo.remoting.http12.HttpMetadata;
+import org.apache.dubbo.remoting.http12.HttpResponse;
+
+@Activate


Would scope bean be better?

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.

AlbumenJ · 2024-01-13T02:50:57Z

dubbo-common/src/main/java/org/apache/dubbo/common/utils/LRU2Cache.java

 * 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> {


LRU2Cache 和 LRUCache 历史上存在过并发或者是内存泄漏问题，如果需要使用的话这部分需要验证下

Ok，只是加了一个computeIfAbsent，这个之前没重写缺了锁

这个变动如果跟这个PR没有关联，可以单独提个PR。

AlbumenJ · 2024-01-13T03:17:52Z

...pc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/route/RequestRouter.java

+import org.apache.dubbo.remoting.http12.HttpChannel;
+import org.apache.dubbo.remoting.http12.RequestMetadata;
+
+@SPI(value = CommonConstants.TRIPLE, scope = ExtensionScope.FRAMEWORK)


对于只有一个实现的 SPI，直接使用 ScopeBean 的模式去处理

AlbumenJ · 2024-01-13T03:23:44Z

...org/apache/dubbo/rpc/protocol/tri/rest/support/servlet/ServletHttpMessageAdapterFactory.java

+import org.apache.dubbo.remoting.http12.HttpResponse;
+import org.apache.dubbo.remoting.http12.message.HttpMessageAdapterFactory;
+
+@Activate(order = -100, onClass = "javax.servlet.http.HttpServletRequest")


HttpServletRequest 目前是不是还没有实现

这里使用 Activate 机制的模式有点奇怪，这里应该是需要基于数据源是哪种类型动态切换的吧，最好是用 Adaptive 的模式基于参数动态路由？

这个主要是希望能够支持servlet生态，在应用依赖里面有servlet包的时候，直接适配成HttpServletRequest，这样在扩展filter里面就可以支持很多spring web和servlet的能力

AlbumenJ · 2024-01-13T04:00:42Z

.../main/java/org/apache/dubbo/rpc/protocol/tri/rest/mapping/DefaultRequestMappingRegistry.java

+        });
+    }
+
+    private void register(RequestMapping mapping, HandlerMeta handler) {


Rename to register0 would be better

CrazyHZM · 2024-01-18T03:12:32Z

dubbo-common/src/main/java/org/apache/dubbo/common/logger/FluentLogger.java

+
+import java.util.function.Supplier;
+
+public interface FluentLogger {


This change should not be in this PR.

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.

CrazyHZM · 2024-01-18T03:16:51Z

dubbo-common/src/main/java/org/apache/dubbo/common/utils/CollectionUtils.java

        } else {
            return values.iterator().next();
        }
    }

+    public static <T> T first(List<T> values) {


This can be replaced by first(Collection<T> values)

It will eliminate one class type check and as an overloaded method, it has no significant usage cost.

CrazyHZM · 2024-01-18T03:24:37Z

dubbo-common/src/main/java/org/apache/dubbo/common/utils/CollectionUtils.java

+    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;
+    }


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.

Copy from guava, https://stackoverflow.com/questions/30220820/difference-between-new-hashmapint-and-guava-maps-newhashmapwithexpectedsizein
Will add some comments.

CrazyHZM · 2024-01-18T03:27:03Z

dubbo-common/src/main/java/org/apache/dubbo/common/utils/LRU2Cache.java

 * 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> {


这个变动如果跟这个PR没有关联，可以单独提个PR。

CrazyHZM · 2024-01-18T03:33:25Z

dubbo-common/src/main/java/org/apache/dubbo/common/utils/TimeUtils.java

@@ -73,4 +73,29 @@ private static synchronized void startTicker() {
            isTickerAlive = true;
        }
    }
+
+    public static Long parseTimeoutToMills(String timeoutVal) {


This section is recommended to be put back into the corresponding module first, it does not seem to be general enough.

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.

CrazyHZM · 2024-01-18T06:36:24Z

...emoting-http12/src/main/java/org/apache/dubbo/remoting/http12/message/DefaultHttpResult.java

+public class DefaultHttpResult<T> implements HttpResult<T> {
+
+    private int status;
+    private Map<String, List<String>> headers;


Why is the value of the header an array?

For example, with something like 'set-cookie', there can be multiple instances, so it should be a list.

CrazyHZM · 2024-01-18T06:37:45Z

...emoting-http12/src/main/java/org/apache/dubbo/remoting/http12/message/DefaultHttpResult.java

+
+public class DefaultHttpResult<T> implements HttpResult<T> {
+
+    private int status;


statusCode?

codecov-commenter · 2024-01-23T17:21:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (46af145) 41.41% compared to head (a56c34d) 38.51%.
Report is 44 commits behind head on 3.3.

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.
📢 Have feedback on the report? Share it here.

sonarcloud · 2024-01-31T10:12:54Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

317 New issues
0 Security Hotspots
0.0% Coverage on New Code
1.5% Duplication on New Code

See analysis details on SonarCloud

oxsean force-pushed the feature-13427-triple-convergence-rest branch from 566f6db to 00679b1 Compare January 4, 2024 13:41

oxsean force-pushed the feature-13427-triple-convergence-rest branch 2 times, most recently from 4494714 to 56c928a Compare January 4, 2024 14:48

AlbumenJ requested changes Jan 4, 2024

View reviewed changes

oxsean force-pushed the feature-13427-triple-convergence-rest branch from 56c928a to 4a5d381 Compare January 5, 2024 01:17

AlbumenJ requested changes Jan 5, 2024

View reviewed changes

oxsean force-pushed the feature-13427-triple-convergence-rest branch 11 times, most recently from fdf421d to 60848cf Compare January 6, 2024 14:26

AlbumenJ requested changes Jan 13, 2024

View reviewed changes

CrazyHZM requested changes Jan 18, 2024

View reviewed changes

oxsean force-pushed the feature-13427-triple-convergence-rest branch 4 times, most recently from 941ff49 to 67ce0fa Compare January 19, 2024 05:39

oxsean added 2 commits January 20, 2024 17:07

feat(triple ): First version of dubbo triple&rest protocol convergence

1fb4d46

feat(rest): argument resolve & split into separate modules

ebab805

oxsean force-pushed the feature-13427-triple-convergence-rest branch from 0ec19b1 to e453da0 Compare January 22, 2024 14:47

feat(rest): Add unit tests

a0263d9

oxsean force-pushed the feature-13427-triple-convergence-rest branch from e453da0 to a0263d9 Compare January 22, 2024 15:55

oxsean added 2 commits January 23, 2024 09:28

feat(rest): Trigger ci.

e1380d8

fix(rest): Avoid accessing applicationModel in constructor

2be93af

feat(rest): refine & response support

3c7432d

oxsean force-pushed the feature-13427-triple-convergence-rest branch from d31286d to 3c7432d Compare January 26, 2024 06:45

fix(rest) Bugfix in http2

3816b3c

oxsean force-pushed the feature-13427-triple-convergence-rest branch from 7a19a9d to 3816b3c Compare January 26, 2024 12:11

oxsean added 5 commits January 26, 2024 21:32

fix(rest) Resolve sonar issue

b216002

feat(rest): Refine exception usage

7c4759f

fix(rest): Bugfix in obtaining beanFactory in SpringRestToolKit

309f3b7

fix(rest): Bugfix of parameter name not found

2fb1900

feat(codecs): Enhance security and parsing in XML and YAML codecs

95b9aaf

oxsean force-pushed the feature-13427-triple-convergence-rest branch 4 times, most recently from 0161ebc to efe149f Compare January 29, 2024 16:07

fix(triple): Exclude standard http header in attachment

f66fed1

oxsean force-pushed the feature-13427-triple-convergence-rest branch from efe149f to f66fed1 Compare January 29, 2024 16:08

oxsean added 2 commits January 31, 2024 03:48

fix(rest): Refactor method names and modify codec activation order

24ac6ee

Merge branch '3.3' into feature-13427-triple-convergence-rest

6abeb11

oxsean mentioned this pull request Jan 31, 2024

Fix when the Accept header is "*/*" or null, adapt the Content-type to application/json #13698

Closed

refactor(rest): Refine content-type negotiation and clean up code

a56c34d

oxsean force-pushed the feature-13427-triple-convergence-rest branch from 918cba1 to a56c34d Compare January 31, 2024 09:21

AlbumenJ approved these changes Feb 1, 2024

View reviewed changes

AlbumenJ merged commit 2dde7ac into apache:3.3 Feb 1, 2024
19 checks passed

oxsean mentioned this pull request Feb 1, 2024

JsonPbCodecFactory needs to check if the protobuf class exists #13650

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(triple): Dubbo triple&rest protocol convergense #13607

feat(triple): Dubbo triple&rest protocol convergense #13607

oxsean commented Jan 3, 2024 •

edited

AlbumenJ commented Jan 4, 2024

oxsean commented Jan 4, 2024

AlbumenJ Jan 4, 2024

AlbumenJ Jan 4, 2024

oxsean Jan 5, 2024

AlbumenJ Jan 4, 2024

oxsean Jan 5, 2024 •

edited

AlbumenJ Jan 4, 2024

oxsean Jan 5, 2024

AlbumenJ Jan 13, 2024

oxsean Jan 13, 2024

CrazyHZM Jan 18, 2024

AlbumenJ Jan 13, 2024

AlbumenJ Jan 13, 2024

oxsean Jan 13, 2024

AlbumenJ Jan 13, 2024

CrazyHZM Jan 18, 2024

oxsean Jan 18, 2024

CrazyHZM Jan 18, 2024

oxsean Jan 18, 2024

CrazyHZM Jan 18, 2024

oxsean Jan 18, 2024 •

edited

CrazyHZM Jan 18, 2024

CrazyHZM Jan 18, 2024

oxsean Jan 18, 2024

CrazyHZM Jan 18, 2024

oxsean Jan 18, 2024

CrazyHZM Jan 18, 2024

codecov-commenter commented Jan 23, 2024 •

edited

sonarcloud bot commented Jan 31, 2024


		import java.util.function.Supplier;

		public interface FluentLogger {


		public class DefaultHttpResult<T> implements HttpResult<T> {

		private int status;

feat(triple): Dubbo triple&rest protocol convergense #13607

feat(triple): Dubbo triple&rest protocol convergense #13607

Conversation

oxsean commented Jan 3, 2024 • edited

What is the purpose of the change

Brief changelog

Verifying this change

Checklist

AlbumenJ commented Jan 4, 2024

oxsean commented Jan 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxsean Jan 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxsean Jan 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 23, 2024 • edited

Codecov Report

sonarcloud bot commented Jan 31, 2024

Quality Gate passed

oxsean commented Jan 3, 2024 •

edited

oxsean Jan 5, 2024 •

edited

oxsean Jan 18, 2024 •

edited

codecov-commenter commented Jan 23, 2024 •

edited