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

Support traffic routing capability. #3024

Merged
merged 4 commits into from
Feb 14, 2023

Conversation

panxiaojun233
Copy link
Collaborator

@panxiaojun233 panxiaojun233 commented Jan 30, 2023

Describe what this PR does / why we need it

Support traffic routing capability based on OpenSergo TrafficRouter

Does this pull request fix one issue?

Fixes #3023

Describe how you did it

Refer the design notes in #3023

Describe how to verify it

Run the test cases and demo.

A relevant Dubbo extension that integrates with the traffic router: apache/dubbo-spi-extensions#192

Special notes for reviews

IMPORTANT: we may need to discuss the module and package structure of the nouveau traffic routing code. See #3025

@CLAassistant
Copy link

CLAassistant commented Jan 30, 2023

CLA assistant check
All committers have signed the CLA.

@sczyh30 sczyh30 added kind/feature Category issues or prs related to feature request. area/traffic-governance Issues or PRs related to traffic governance labels Jan 30, 2023
@sczyh30 sczyh30 requested a review from a team January 30, 2023 03:12
@sczyh30 sczyh30 added to-review To review size/XXL Indicate a PR that changes 1000+ lines. labels Jan 30, 2023
/**
* @author panxiaojun233
*/
public class Instance {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a toString() for entities?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I will fix it

public class Route {
private String name;
private List<StringMatch> services;
private List<RouteDetail> routedetail;
Copy link
Member

Choose a reason for hiding this comment

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

camel case: routeDetail is better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I will fix it


@Override
public String toString() {
return "DubboRoute{" +
Copy link
Member

Choose a reason for hiding this comment

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

Please re-generate toString() for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.alibaba.csp.sentinel.traffic.rule.vritual.workload;
Copy link
Member

Choose a reason for hiding this comment

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

typo: vritual -> virtual
How about just com.alibaba.csp.sentinel.traffic.rule.workload?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

private StringMatch path;
private Integer argc;
private List<ArgumentMatch> args;
private List<StringMatch> argp;
Copy link
Member

Choose a reason for hiding this comment

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

This seems perplexing... Maybe we could improve the naming.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

* The subscriber of traffic rule.
* @author panxiaojun233
*/
public interface TrafficRuleSubscriber {
Copy link
Member

Choose a reason for hiding this comment

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

Here we actually observes the event of "remote provider app change", not the rule. So maybe RemoteAppObserver or other name is better?

The same for subscribe/unsubscribe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

LGTM

@sczyh30 sczyh30 merged commit f10aedd into alibaba:master Feb 14, 2023
@sczyh30
Copy link
Member

sczyh30 commented Feb 14, 2023

Nice work. Thanks for contributing!

@sczyh30 sczyh30 removed the to-review To review label Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/traffic-governance Issues or PRs related to traffic governance kind/feature Category issues or prs related to feature request. size/XXL Indicate a PR that changes 1000+ lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Support traffic routing capability | 流量路由模型及数据流设计
3 participants