[SCB-706]change filters to discovery filters and support invocation based filters #803
Conversation
liubao68
requested review from
wujimin and
WillemJiang
and removed request for
wujimin
|
related docs PR： apache/servicecomb-docs#17 |
| public boolean equals(Object o) { | ||
| if (o instanceof CseServer) { | ||
| return this.getHost().equals(((CseServer) o).getHost()); | ||
| if (o instanceof ServiceCombServer) { |
There was a problem hiding this comment.
rest endpoint equals highway endpoint?
why?
There was a problem hiding this comment.
DiscoveryFilter sees MicroserviceInstance, not Endpoint. And the health check is also to MicroserviceInstance, not Endpoint. I modified server same when MicroserviceInstance is same to make all these place consistence to each other. (like when invocation failed, we mark instance status to failure for one time, but not endpoint failed.)
| } | ||
|
|
||
| @Override | ||
| public List<Server> getFilteredListOfServers(List<Server> servers, Invocation invocation) { | ||
| List<Server> filteredServers = new ArrayList<>(); | ||
| Map<String, String> filterOptions = |
There was a problem hiding this comment.
seems this is about configuration and instances
better to move to DiscoveryFilter?
so that we can use cache ability?
There was a problem hiding this comment.
This filter leaves for compatible, maybe we need provide a new one in future. So I leave it here but not remove it. We can think about your idea in future when needed.
| @@ -30,6 +30,9 @@ public DiscoveryTreeNode discovery(DiscoveryContext context, DiscoveryTreeNode p | |||
| } | |||
|
|
|||
| String childName = findChildName(context, parent); | |||
| if (childName == null) { | |||
| return null; | |||
There was a problem hiding this comment.
when will this happened?
after return null, DiscoveryTree will throw exception:
// impossible, throw exception to help fix bug
throw new ServiceCombException(filter.getClass().getName() + " discovery return null.");
|
Since 3 days passed, no more reviews, I am going to merge this PR and doing further integration tests. |
| @@ -173,7 +173,7 @@ public int getRetryOnSame(String microservice) { | |||
| } | |||
|
|
|||
| public boolean isIsolationFilterOpen(String microservice) { | |||
| String p = getStringProperty("false", | |||
| String p = getStringProperty("true", | |||
There was a problem hiding this comment.
There are lots of default value changed, not sure if it break the old behavior.
There was a problem hiding this comment.
Isolation is not enabled by old version, now we enable it by default and change it's default values. Documents here: https://huaweicse.github.io/servicecomb-java-chassis-doc/zh_CN/references-handlers/loadbalance.html
| throw new Error(errMsg, e); | ||
| if (!policyClsName.isEmpty()) { | ||
| LOGGER.error(Configuration.TRANSACTIONCONTROL_POLICY_KEY_PATTERN + " is not supported anymore." + | ||
| "You can change this class to SPI, and filters will be loaded by SPI."); |
There was a problem hiding this comment.
We need to provide a document for the user to upgrade the code the new SPI.
There was a problem hiding this comment.
Follow this checklist to help us incorporate your contribution quickly and easily:
[SCB-XXX] Fixes bug in ApproximateQuantiles, where you replaceSCB-XXXwith the appropriate JIRA issue.mvn clean installto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.