Re: [PR] fix diff method can not be added to RadixTree [dubbo]
oxsean merged PR #15445: URL: https://github.com/apache/dubbo/pull/15445 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
Stellar1999 commented on PR #15445: URL: https://github.com/apache/dubbo/pull/15445#issuecomment-2960882762 @oxsean Please take a look again.I changed Overlap method. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
oxsean commented on code in PR #15445:
URL: https://github.com/apache/dubbo/pull/15445#discussion_r2137858746
##
dubbo-rpc/dubbo-rpc-triple/src/test/groovy/org/apache/dubbo/rpc/protocol/tri/rest/mapping/RadixTreeTest.groovy:
##
@@ -59,6 +61,29 @@ class RadixTreeTest extends Specification {
'/a/b/c/d' | 0
}
+def "test repeat add,no equal function"() {
Review Comment:
Should test with class `Registration`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
oxsean commented on code in PR #15445:
URL: https://github.com/apache/dubbo/pull/15445#discussion_r2138072584
##
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/rest/mapping/RadixTree.java:
##
@@ -63,13 +65,25 @@ public RadixTree() {
this(true, '/');
}
+/**
+ * This is a default implementation that does not check for equality.
+ */
public T addPath(PathExpression path, T value) {
+return addPath(path, value, (t, t2) -> Boolean.TRUE);
+}
+
+/**
+ * When predicate is not null, if a node is found at the path, it will not
be returned immediately;
Review Comment:
Remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
oxsean commented on code in PR #15445:
URL: https://github.com/apache/dubbo/pull/15445#discussion_r2137876444
##
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/rest/mapping/RadixTree.java:
##
@@ -83,9 +95,16 @@ public T addPath(PathExpression path, T value) {
Node child = getChild(current, segments[i]);
if (i == len - 1) {
List> values = child.values;
-for (int j = 0, size = values.size(); j < size; j++) {
-if (values.get(j).getLeft().equals(path)) {
-return values.get(j).getRight();
+for (Pair pathExpressionTPair : values) {
Review Comment:
Minor: Why using an index-based loop because it can avoid create an iterator
object to reduce object allocation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
oxsean commented on code in PR #15445:
URL: https://github.com/apache/dubbo/pull/15445#discussion_r2138072584
##
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/rest/mapping/RadixTree.java:
##
@@ -63,13 +65,25 @@ public RadixTree() {
this(true, '/');
}
+/**
+ * This is a default implementation that does not check for equality.
+ */
public T addPath(PathExpression path, T value) {
+return addPath(path, value, (t, t2) -> Boolean.TRUE);
+}
+
+/**
+ * When predicate is not null, if a node is found at the path, it will not
be returned immediately;
Review Comment:
Rewrite it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
Stellar1999 commented on PR #15445: URL: https://github.com/apache/dubbo/pull/15445#issuecomment-2959415328 > Please fix the comments. done. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
oxsean commented on code in PR #15445:
URL: https://github.com/apache/dubbo/pull/15445#discussion_r2137884012
##
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/rest/mapping/RadixTree.java:
##
@@ -83,9 +95,16 @@ public T addPath(PathExpression path, T value) {
Node child = getChild(current, segments[i]);
if (i == len - 1) {
List> values = child.values;
-for (int j = 0, size = values.size(); j < size; j++) {
-if (values.get(j).getLeft().equals(path)) {
-return values.get(j).getRight();
+for (Pair pathExpressionTPair : values) {
Review Comment:
Minor: When there is no ambiguity, shorter variable names are easier to read
Rename pathExpressionTPair to pair
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
oxsean commented on code in PR #15445:
URL: https://github.com/apache/dubbo/pull/15445#discussion_r2137901408
##
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/rest/mapping/RadixTree.java:
##
@@ -83,9 +95,16 @@ public T addPath(PathExpression path, T value) {
Node child = getChild(current, segments[i]);
if (i == len - 1) {
List> values = child.values;
-for (int j = 0, size = values.size(); j < size; j++) {
-if (values.get(j).getLeft().equals(path)) {
-return values.get(j).getRight();
+for (Pair pathExpressionTPair : values) {
+if (pathExpressionTPair.getLeft().equals(path)) {
+T rightVal = pathExpressionTPair.getRight();
Review Comment:
rightVal to existingValue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
oxsean commented on code in PR #15445:
URL: https://github.com/apache/dubbo/pull/15445#discussion_r2137866723
##
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/rest/mapping/RadixTree.java:
##
@@ -63,7 +64,18 @@ public RadixTree() {
this(true, '/');
}
+/**
+ * This is a default implementation that does not check for equality.
+ */
public T addPath(PathExpression path, T value) {
+return addPath(path, value, (t, t2) -> Boolean.TRUE);
+}
+
+/**
+ * When equalFunction is not null, if a node is found at the path, it will
not be returned immediately;
+ * instead, whether to return it depends on the result of the function.
+ */
+public T addPath(PathExpression path, T value, BiFunction
equalFunction) {
Review Comment:
equalFunction rename to predicate or overlapChecker
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
oxsean commented on code in PR #15445:
URL: https://github.com/apache/dubbo/pull/15445#discussion_r2137864027
##
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/rest/mapping/RadixTree.java:
##
@@ -63,7 +64,18 @@ public RadixTree() {
this(true, '/');
}
+/**
+ * This is a default implementation that does not check for equality.
+ */
public T addPath(PathExpression path, T value) {
+return addPath(path, value, (t, t2) -> Boolean.TRUE);
+}
+
+/**
+ * When equalFunction is not null, if a node is found at the path, it will
not be returned immediately;
+ * instead, whether to return it depends on the result of the function.
+ */
+public T addPath(PathExpression path, T value, BiFunction
equalFunction) {
Review Comment:
Support null is not a good design, as users may not understand its meaning.
It is recommended to use `Objects#requireNonNull(T)` for null checks instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
oxsean commented on code in PR #15445:
URL: https://github.com/apache/dubbo/pull/15445#discussion_r2137856814
##
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/rest/mapping/DefaultRequestMappingRegistry.java:
##
@@ -169,7 +169,7 @@ private void register0(RequestMapping mapping, HandlerMeta
handler, AtomicIntege
try {
Registration registration = new Registration(mapping, handler);
for (PathExpression path :
mapping.getPathCondition().getExpressions()) {
-Registration exists = tree.addPath(path, registration);
+Registration exists = tree.addPath(path, registration,
Registration::equals);
Review Comment:
You cannot use `equals`, you need to check whether the method lists
intersect, try add the following method to Registration:
```
public boolean isMethodOverlap(Registration registration) {
MethodsCondition self = mapping.getMethodsCondition();
MethodsCondition other =
registration.getMapping().getMethodsCondition();
return self == null || other == null ||
!Collections.disjoint(self.getMethods(), other.getMethods());
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
Stellar1999 commented on PR #15445: URL: https://github.com/apache/dubbo/pull/15445#issuecomment-2957750693 @oxsean PTAL -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
Stellar1999 commented on PR #15445: URL: https://github.com/apache/dubbo/pull/15445#issuecomment-2956241864 > add unit tests from #15433 ? Sure,I'll do it later. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
zrlw commented on PR #15445: URL: https://github.com/apache/dubbo/pull/15445#issuecomment-2956211915 add unit tests from #15433 ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] fix diff method can not be added to RadixTree [dubbo]
codecov-commenter commented on PR #15445: URL: https://github.com/apache/dubbo/pull/15445#issuecomment-2956006556 ## [Codecov](https://app.codecov.io/gh/apache/dubbo/pull/15445?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report Attention: Patch coverage is `0%` with `5 lines` in your changes missing coverage. Please review. > Project coverage is 29.37%. Comparing base [(`c31164a`)](https://app.codecov.io/gh/apache/dubbo/commit/c31164ab323e7b22c3c8088e5ab596374fddbd1c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`a5af65b`)](https://app.codecov.io/gh/apache/dubbo/commit/a5af65b962cd8edce2d44f5393e286bdfa42a3b1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). | [Files with missing lines](https://app.codecov.io/gh/apache/dubbo/pull/15445?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [...dubbo/rpc/protocol/tri/rest/mapping/RadixTree.java](https://app.codecov.io/gh/apache/dubbo/pull/15445?src=pr&el=tree&filepath=dubbo-rpc%2Fdubbo-rpc-triple%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fdubbo%2Frpc%2Fprotocol%2Ftri%2Frest%2Fmapping%2FRadixTree.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy10cmlwbGUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9wcm90b2NvbC90cmkvcmVzdC9tYXBwaW5nL1JhZGl4VHJlZS5qYXZh) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/dubbo/pull/15445?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | > :exclamation: There is a different number of reports uploaded between BASE (c31164a) and HEAD (a5af65b). Click for more details. > > HEAD has 4 uploads less than BASE > >| Flag | BASE (c31164a) | HEAD (a5af65b) | >|--|--|--| >|samples-tests-java17|1|0| >|integration-tests-java17|1|0| >|integration-tests-java8|1|0| >|unit-tests|1|0| > Additional details and impacted files ```diff @@ Coverage Diff @@ ##3.3 #15445 +/- ## = - Coverage 60.90% 29.37% -31.53% + Complexity11455 8958 -2497 = Files 1888 1888 Lines 8630986312+3 Branches 1293312934+1 = - Hits 5257025358-27212 - Misses2830256960+28658 + Partials 5437 3994 -1443 ``` | [Flag](https://app.codecov.io/gh/apache/dubbo/pull/15445/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [integration-tests-java17](https://app.codecov.io/gh/apache/dubbo/pull/15445/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | | [integration-tests-java8](https://app.codecov.io/gh/apache/dubbo/pull/15445/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | | [samples-tests-java17](https://app.codecov.io/gh/apache/dubbo/pull/15445/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | | [samples-tests-java8](https://app.codecov.io/gh/apache/dubbo/pull/15445/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `29.37% <0.00%> (+0.10%)` | :arrow_up: | | [unit-tests](https://app.codecov.io/gh/apache/dubbo/pull/15445/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/dubbo/pull/15445?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm
