add feature , request can be handle by self-defined class by gudelian941 · Pull Request #3377 · apache/servicecomb-java-chassis

gudelian941 · 2022-09-30T06:23:52Z

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

Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [SCB-XXX] Fixes bug in ApproximateQuantiles, where you replace SCB-XXX with the appropriate JIRA issue.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean install -Pit to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

liubao68 · 2022-09-30T06:37:51Z

governance/src/main/java/org/apache/servicecomb/governance/marker/TrafficMarker.java

+  private String profileValues;
+
+  private String profileExtractClass;


I think it is not a good idea to create these two properties here. We need to keep the specification clear and clarity, see https://github.com/huaweicloud/spring-cloud-huawei/wiki/using-governance

First of all, we should keep the specification language independent. For your requirement, I think you can add this logic when you create your governance request, and set the header you need.

liubao68 · 2022-09-30T06:39:13Z

governance/src/main/java/org/apache/servicecomb/governance/marker/GovernanceRequest.java

+  public Object getSourceRequest() {
+    return sourceRequest;
+  }
+
+  public void setSourceRequest(Object sourceRequest) {
+    this.sourceRequest = sourceRequest;
+  }


This property will make design not specific and not descriptive, I think this is not good extension.

I don't think it's reasonable to treat the sourceRequest as a Object.
If it just a URI, we can change the Object to String.

liubao68 · 2022-09-30T06:40:29Z

Can you create a issue on why you add this feature? and maybe we have other ways to implement your feature. Your PR seems not very good for a framework extension.

governance/src/main/java/org/apache/servicecomb/governance/marker/RequestProcessor.java

governance/src/main/java/org/apache/servicecomb/governance/utils/CustomMatch.java

governance/src/main/java/org/apache/servicecomb/governance/marker/CustomMatcher.java

WillemJiang · 2022-10-11T01:17:32Z

governance/src/main/java/org/apache/servicecomb/governance/marker/GovernanceRequest.java

+  public Object getSourceRequest() {
+    return sourceRequest;
+  }
+
+  public void setSourceRequest(Object sourceRequest) {
+    this.sourceRequest = sourceRequest;
+  }


I don't think it's reasonable to treat the sourceRequest as a Object.
If it just a URI, we can change the Object to String.

governance/src/main/java/org/apache/servicecomb/governance/marker/RequestProcessor.java

governance/src/test/java/org/apache/servicecomb/governance/CustomMatchTest.java

governance/src/test/java/org/apache/servicecomb/governance/mockclasses/ClassNotImplments.java

governance/src/test/java/org/apache/servicecomb/governance/mockclasses/ClassWithAnnotation.java

governance/src/test/java/org/apache/servicecomb/governance/mockclasses/CustomMatchDemo.java

...org/apache/servicecomb/governance/mockclasses/service/MockConfigurationForCustomMatcher.java

.../java/org/apache/servicecomb/governance/mockclasses/service/MockProfileClassUserService.java

WillemJiang · 2022-10-11T02:53:41Z

governance/src/main/java/org/apache/servicecomb/governance/utils/CustomMatch.java

 public interface CustomMatch {
    String errorMessageForNotImplements = " didn't implement interface org.apache.servicecomb.governance.utils.CustomMatch";
    String errorMessageForAbstractClass = " should be a instantiable class rather than abstract class or other else";
+    String infoMessageForCreatingClass = "is not in spring container, create one and register it to spring container";


If we don't let the user to use this constant string, it's better just put them into the log method.

codecov-commenter · 2022-10-14T04:34:04Z

Codecov Report

Merging #3377 (1a470e6) into master (6d20460) will decrease coverage by 0.01%.
The diff coverage is 70.90%.

@@             Coverage Diff              @@
##             master    #3377      +/-   ##
============================================
- Coverage     77.57%   77.55%   -0.02%     
- Complexity     1462     1463       +1     
============================================
  Files          1650     1651       +1     
  Lines         43772    43860      +88     
  Branches       3685     3693       +8     
============================================
+ Hits          33955    34016      +61     
- Misses         8280     8306      +26     
- Partials       1537     1538       +1

Impacted Files	Coverage Δ
...rvicecomb/governance/marker/GovernanceRequest.java	`81.81% <0.00%> (-12.92%)`	⬇️
...ervicecomb/governance/marker/RequestProcessor.java	`81.25% <69.04%> (-13.63%)`	⬇️
...e/servicecomb/governance/marker/CustomMatcher.java	`100.00% <100.00%> (ø)`
.../apache/servicecomb/governance/marker/Matcher.java	`100.00% <100.00%> (ø)`
...thentication/consumer/RSAConsumerTokenManager.java	`70.96% <0.00%> (-6.46%)`	⬇️
...ervicecomb/common/rest/AbstractRestInvocation.java	`87.91% <0.00%> (-4.69%)`	⬇️
...mb/config/ConfigCenterConfigurationSourceImpl.java	`9.57% <0.00%> (-1.07%)`	⬇️
...cecomb/serviceregistry/auth/TokenCacheManager.java	`13.04% <0.00%> (-1.02%)`	⬇️
...g/apache/servicecomb/edge/core/EdgeInvocation.java	`5.12% <0.00%> (-0.14%)`	⬇️
...ache/servicecomb/router/cache/RouterRuleCache.java	`58.13% <0.00%> (ø)`
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

* add feature , request can be handle by self-defined class * refactor for advice * refactor for advice * refactor for advice * refactor for advice * add apache license information for file header * fix issue * rename function * modify for suggestion * modify for suggession * modify for suggestion * resolv checkstyle issue * fix issue

add feature , request can be handle by self-defined class

5d92e43

liubao68 reviewed Sep 30, 2022

View reviewed changes

gudelian941 added 4 commits October 8, 2022 14:33

refactor for advice

8961250

refactor for advice

1e67f91

refactor for advice

10ae788

refactor for advice

f3fbf80

little-cui requested changes Oct 10, 2022

View reviewed changes

governance/src/main/java/org/apache/servicecomb/governance/marker/RequestProcessor.java Show resolved Hide resolved

yhs0092 reviewed Oct 10, 2022

View reviewed changes

governance/src/main/java/org/apache/servicecomb/governance/utils/CustomMatch.java Show resolved Hide resolved

yhs0092 reviewed Oct 10, 2022

View reviewed changes

governance/src/main/java/org/apache/servicecomb/governance/marker/CustomMatcher.java Show resolved Hide resolved

gudelian941 added 3 commits October 10, 2022 15:55

add apache license information for file header

121cbf3

fix issue

124b6da

rename function

b9ca7c9

little-cui approved these changes Oct 11, 2022

View reviewed changes

WillemJiang requested changes Oct 11, 2022

View reviewed changes

gudelian941 added 2 commits October 11, 2022 09:44

modify for suggestion

37659d0

modify for suggession

2b94d92

WillemJiang reviewed Oct 11, 2022

View reviewed changes

gudelian941 added 2 commits October 11, 2022 14:17

modify for suggestion

466cf2d

resolv checkstyle issue

44d7368

gudelian941 requested review from WillemJiang, liubao68 and yhs0092 October 12, 2022 01:02

fix issue

1a470e6

WillemJiang approved these changes Oct 14, 2022

View reviewed changes

little-cui merged commit 7e661e3 into apache:master Oct 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add feature , request can be handle by self-defined class#3377

add feature , request can be handle by self-defined class#3377
little-cui merged 13 commits intoapache:masterfrom
gudelian941:governance_enhancement

gudelian941 commented Sep 30, 2022

Uh oh!

liubao68 Sep 30, 2022

Uh oh!

liubao68 Sep 30, 2022

Uh oh!

WillemJiang Oct 11, 2022

Uh oh!

liubao68 commented Sep 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WillemJiang Oct 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WillemJiang Oct 11, 2022

Uh oh!

codecov-commenter commented Oct 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

		private String profileValues;

		private String profileExtractClass;

Conversation

gudelian941 commented Sep 30, 2022

Uh oh!

liubao68 Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

liubao68 Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

WillemJiang Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

liubao68 commented Sep 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WillemJiang Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WillemJiang Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 14, 2022

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants