Skip to content
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

Add arguments router to ConditionRouter #7378

Merged

Conversation

furaul
Copy link

@furaul furaul commented Mar 16, 2021

Add arguments router logic to ConditionRouter.

What is the purpose of the change

Add arguments router to ConditionRouter.
Compatible to current ConditionRouter logic.

Brief changelog

Add arguments router to ConditionRouter

Verifying this change

Add arguments router to ConditionRouter

Examples

In some cases, we need to choose the right provider while the arguments of the method is setted.
The condition rule is like

arguments[1].str!=b => host = 1.1.1.1

While the str property of the second argument is equal to b, the providers whose host is equal to 1.1.1.1 are to be choosed。
We could use this feature to route requests according to business params.
The feature is compatible to current ConditionRouter logic.

Follow this checklist to help us incorporate your contribution quickly and easily:

  • 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.
  • Format the pull request title like [Dubbo-XXX] Fix UnknownException when host config not exist #XXX. 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.
  • 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.
  • Run mvn clean install -DskipTests=false & mvn clean test-compile failsafe:integration-test to make sure unit-test and integration-test pass.
  • If this contribution is large, please follow the Software Donation Guide.

@furaul furaul changed the title Feature/20210316 arguments condition router Add arguments router to ConditionRouter Mar 16, 2021
@furaul
Copy link
Author

furaul commented Mar 16, 2021

#5649

@furaul
Copy link
Author

furaul commented Mar 16, 2021

@AlbumenJ

2. param support all type which implements toString.
3. add UT
@AlbumenJ AlbumenJ requested a review from chickenlj March 22, 2021 00:57
* Examples would be like this:
* "arguments[0]=1", whenCondition is that the first argument is equal to '1'.
* "arguments[1].param=a", whenCondition is that the 'param' attribute in the second argument is equal to 'a'.
* "arguments[2].inner.param=b", whenCondition is that the 'param' attribute in the 'inner' attribute of the third argument is equal to 'b'.
Copy link
Contributor

@chickenlj chickenlj Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good complement to have an argument matcher.

I think it's better if we can make the argument routing rules simpler while still covering more than 90% of the usage scenarios.

For example, we only support the matching of string type parameters and do not support t matching of complex objects like arguments[1].param. Or even our routing rule is like a switch, when the user tells us to turn it on, we will constantly match the first parameter of the method and require that this parameter be a string or primitive type.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good complement to have an argument matcher.

I think it's better if we can make the argument routing rules simpler while still covering more than 90% of the usage scenarios.

For example, we only support the matching of string type parameters and do not support t matching of complex objects like arguments[1].param. Or even our routing rule is like a switch, when the user tells us to turn it on, we will constantly match the first parameter of the method and require that this parameter be a string or primitive type.

I would like to simplify the arguments rule to make it easier to be understood and used.
For example, we only support "one flour param", like "arguments[0]=1", and only support java primary data type, like int, long, string and so on.
But I think it is not a good idea to make it like a switch, that would reduce flexibility.
And in the future, I would like to config arguments router in the configuration pages, we could use drop-down box to choose the right arguments and its value. And in that time, we could expand the explementation of arguments router.
@AlbumenJ

furao added 3 commits April 20, 2021 10:30
…e "arguments[0]=1".

2. to make it easier to use, the value only support java primary data type.
@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2021

Codecov Report

Merging #7378 (89328be) into master (d009d0c) will decrease coverage by 0.04%.
The diff coverage is 86.36%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7378      +/-   ##
============================================
- Coverage     58.72%   58.68%   -0.05%     
- Complexity      425      493      +68     
============================================
  Files          1043     1076      +33     
  Lines         42471    43439     +968     
  Branches       6212     6344     +132     
============================================
+ Hits          24942    25492     +550     
- Misses        14748    15111     +363     
- Partials       2781     2836      +55     
Impacted Files Coverage Δ Complexity Δ
.../rpc/cluster/router/condition/ConditionRouter.java 71.08% <86.36%> (+2.33%) 0.00 <0.00> (ø)
...he/dubbo/rpc/protocol/rmi/RmiRemoteInvocation.java 0.00% <0.00%> (-80.00%) 0.00% <0.00%> (-2.00%)
...org/apache/dubbo/rpc/protocol/rmi/RmiProtocol.java 0.00% <0.00%> (-61.67%) 0.00% <0.00%> (-10.00%)
...g/context/properties/DefaultDubboConfigBinder.java 42.85% <0.00%> (-57.15%) 0.00% <0.00%> (ø%)
.../java/org/apache/dubbo/qos/command/impl/Ready.java 3.33% <0.00%> (-46.67%) 0.00% <0.00%> (ø%)
.../cluster/configurator/parser/model/ConfigItem.java 48.00% <0.00%> (-24.00%) 0.00% <0.00%> (ø%)
...bbo/common/bytecode/CustomizedLoaderClassPath.java 28.00% <0.00%> (-16.00%) 0.00% <0.00%> (ø%)
...che/dubbo/remoting/transport/mina/MinaChannel.java 35.52% <0.00%> (-14.48%) 14.00% <0.00%> (-2.00%)
...t/migration/DefaultMigrationAddressComparator.java 15.38% <0.00%> (-11.54%) 0.00% <0.00%> (ø%)
...in/java/org/apache/dubbo/common/utils/JVMUtil.java 81.13% <0.00%> (-11.33%) 0.00% <0.00%> (ø%)
... and 135 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d009d0c...89328be. Read the comment docs.

Copy link
Member

@AlbumenJ AlbumenJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@AlbumenJ AlbumenJ merged commit 6e09966 into apache:master Apr 21, 2021
AlbumenJ added a commit to AlbumenJ/dubbo that referenced this pull request May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants