Make the slot as the SPI interface and add a @Order annotation for define slot order

[cdfive]

Describe what this PR does / why we need it

Make it easier to add custorm slot and arrange its order for users.

Does this pull request fix one issue?

Fixes #316

Describe how you did it

1.Add a @Order annotation and define the order of default slots from -9000 to -1000
NodeSelectorSlot: -9000
ClusterBuilderSlot: -8000
LogSlot: -7000
StatisticSlot: -6000
ParamFlowSlot: -5000
SystemSlot: -4000
AuthoritySlot: -3000
FlowSlot: -2000
DegradeSlot: -1000

2.Add a loadOrderedInstanceList(Class<T>) method in SpiLoader,
to Load the ordered instance list by SPI

3.Modify the DefaultSlotChainBuilder
new XxxSlot changed to using SpiLoader.loadOrderedInstanceList(ProcessorSlot.class);

Describe how to verify it

Add unit test

Special notes for reviews

Please review its feasibility in this way,
if it is okay, maybe the InitOrder can also use Order annotation instead, for similar logic,
the InitOrder and InitExecutor didn't modified at present.

[codecov-io]

Codecov Report

Merging #411 into master will increase coverage by 0.28%.
The diff coverage is 64.00%.

@@             Coverage Diff              @@
##             master     #411      +/-   ##
============================================
+ Coverage     43.24%   43.52%   +0.28%     
- Complexity     1579     1676      +97     
============================================
  Files           338      364      +26     
  Lines          9905    10492     +587     
  Branches       1337     1409      +72     
============================================
+ Hits           4283     4567     +284     
- Misses         5093     5365     +272     
- Partials        529      560      +31     

Impacted Files	Coverage Δ	Complexity Δ
...l/adapter/gateway/common/slot/GatewayFlowSlot.java	0.00% <ø> (ø)	0.00 <0.00> (ø)
...r/gateway/common/slot/GatewaySlotChainBuilder.java	0.00% <0.00%> (ø)	0.00 <0.00> (ø)
.../sentinel/slots/block/authority/AuthoritySlot.java	88.23% <ø> (ø)	7.00 <0.00> (ø)
.../csp/sentinel/slots/block/degrade/DegradeSlot.java	100.00% <ø> (ø)	3.00 <0.00> (ø)
...libaba/csp/sentinel/slots/block/flow/FlowSlot.java	100.00% <ø> (ø)	5.00 <0.00> (ø)
...tinel/slots/clusterbuilder/ClusterBuilderSlot.java	71.42% <ø> (ø)	8.00 <0.00> (ø)
...com/alibaba/csp/sentinel/slots/logger/LogSlot.java	73.33% <ø> (ø)	3.00 <0.00> (ø)
.../sentinel/slots/nodeselector/NodeSelectorSlot.java	94.73% <ø> (ø)	4.00 <0.00> (ø)
...ba/csp/sentinel/slots/statistic/StatisticSlot.java	67.18% <ø> (ø)	13.00 <0.00> (ø)
.../alibaba/csp/sentinel/slots/system/SystemSlot.java	100.00% <ø> (ø)	3.00 <0.00> (ø)
... and 60 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 c507155...68be55c. Read the comment docs.

[cdfive]

@sczyh30 Hi,
synchronize from master branch, I find @SpiOrder, so I use that instead of previous @Order.

Use SpiLoader.loadInstanceListSorted(Class<T> clazz) to get the sorted SPI instance list.

Since the ProcessorSlot need to create new instance in each DefaultSlotChainBuilder,
, so I modified the method, not use the SERVICE_LOADER_MAP but call ServiceLoader.load(clazz) directly, in order to get new instance each call.

Maybe the cached SERVICE_LOADER_MAP can be removed, I'm not sure about that.

Another concern is the performance of sort, if call the method frequently.
How about caching the sort of same SPI class?

[sczyh30]

Could you please fix the conflict?

[cdfive]

Sorry for late, the conflicts has fixed.

[sczyh30]

@cdfive Could you please resolve the conflict?

[cdfive]

@cdfive Could you please resolve the conflict?
I'll do it these days.

[cdfive]

@sczyh30 Sorry for late, The conflicts has been resolved.
I found that the ProcessorSlot(such as NodeSelectorSlot,ClusterBuilderSlot...) should create new instance in DefaultSlotChainBuilder, so add a method loadDifferentInstanceListSorted in SpiLoader class.

SpiLoaderTest#testLoadDifferentInstanceListSorted,DefaultSlotChainBuilderTest#testBuild
to verify it, please check.

[sczyh30]

👍Great, I'll review the PR soon.

[CLAassistant]

All committers have signed the CLA.

[cdfive]

Now GatewaySlotChainBuilder,HotParamSlotChainBuilder are removed, DefaultSlotChainBuilder is the only implement class of SlotChainBuilder,
which can sovle #1306, since the SlotChainProvider#newSlotChain will use DefaultSlotChainBuilder.

All slots are marked with @SpiOrder, from -10000 to -1000:

slot	@SpiOrder
NodeSelectorSlot	-10000
ClusterBuilderSlot	-9000
LogSlot	-8000
StatisticSlot	-7000
AuthoritySlot	-6000
SystemSlot	-5000
GatewayFlowSlot	-4000
ParamFlowSlot	-3000
FlowSlot	-2000
DegradeSlot	-1000

Test cases and comment has been improved, please take a review.

Is there any other suggestion? @sczyh30 @jasonjoo2010
Do we need to add the order values of Sentinel main slots into a Constant class, e.g. Constants?

like:

public static final String NODE_SELECTOR_SLOT_ORDER = -10000;
public static final String CLUSTER_BUILDER_SLOT_ORDER = NODE_SELECTOR_SLOT_ORDER + 1000;
...
public static final String SYSTEM_SLOT_ORDER = ...

and in extension GatewayFlowSlot:

@SpiOrder(Constants.SYSTEM_SLOT_ORDER + 1000)
public class GatewayFlowSlot {
}

Maybe it will be useful for user and developer to see the orders of slots more intuitively.

[jasonjoo2010]

I think maybe GatewaySlotChainBuilder and HotParamSlotChainBuilder should be preserved as legacy, declare them as deprecated, unlink their service declarations and rewrite them to inherit DefaultSlotChainBuilder to avoid that someone may reference or inherit them.

Thus we can allow users' customizations:

• Implement new ProcessorSlot by extends AbstractProcessorSlot and define it in their own META-INF
• Implement their own SlotChainBuilder and do all things in a different way (should be careful)

So provider of SlotChainBuilder maybe still need some adaption?

[cdfive]

GatewaySlotChainBuilder and HotParamSlotChainBuilder are reserved, marked as @Deprecated, the implement is not changed, and unlink their service declarations.
How about this? @jasonjoo2010 @sczyh30

[jasonjoo2010]

Oh if you take my advice keeping an empty legacy one you should make it a real EMPTY one.
According to the new DefaultSlotChainBuilder i think there is no need keeping overriding of build() here, right?

[cdfive]

I'm sorry for misunderstanding the point.It has been fixed, please check.

[jasonjoo2010]

Okay i have no further question now.

@sczyh30 It's suggested that you make a quick review.

[sczyh30]

Okay i have no further question now.

@sczyh30 It's suggested that you make a quick review.

I'll take a look.

[sczyh30]

LGTM. The name loadDifferentInstanceListSorted might be confusing, maybe we could improve it later :)

[sczyh30]

Nice work. Thanks!

[sczyh30]

Here we need to replace the old placeholder with "{}":
https://github.com/alibaba/Sentinel/blob/master/sentinel-core/src/main/java/com/alibaba/csp/sentinel/util/SpiLoader.java#L222

[sczyh30]

And we may need to improve the logs here because it will yield a large number of logs every time a new chain is built:

2020-03-26 16:21:14.948 INFO Add child </metric/queryTopResourceMetric.json> to node <sentinel_web_servlet_context>
2020-03-26 16:21:36.215 INFO [SpiLoader] Found {0} SPI: {1} with order -10000
2020-03-26 16:21:36.216 INFO [SpiLoader] Found {0} SPI: {1} with order -9000
2020-03-26 16:21:36.217 INFO [SpiLoader] Found {0} SPI: {1} with order -8000
2020-03-26 16:21:36.217 INFO [SpiLoader] Found {0} SPI: {1} with order -7000
2020-03-26 16:21:36.217 INFO [SpiLoader] Found {0} SPI: {1} with order -5000
2020-03-26 16:21:36.217 INFO [SpiLoader] Found {0} SPI: {1} with order -6000
2020-03-26 16:21:36.217 INFO [SpiLoader] Found {0} SPI: {1} with order -2000
2020-03-26 16:21:36.218 INFO [SpiLoader] Found {0} SPI: {1} with order -1000
2020-03-26 16:21:36.218 INFO [SpiLoader] Found {0} SPI: {1} with order -3000
2020-03-26 16:21:36.219 INFO [SpiLoader] Found {0} SPI: {1} with order -4000
2020-03-26 16:21:36.219 INFO Add child </resource/machineResource.json> to node <sentinel_web_servlet_context>
2020-03-26 16:21:38.956 INFO [SpiLoader] Found {0} SPI: {1} with order -10000
2020-03-26 16:21:38.956 INFO [SpiLoader] Found {0} SPI: {1} with order -9000
2020-03-26 16:21:38.956 INFO [SpiLoader] Found {0} SPI: {1} with order -8000
2020-03-26 16:21:38.957 INFO [SpiLoader] Found {0} SPI: {1} with order -7000
2020-03-26 16:21:38.957 INFO [SpiLoader] Found {0} SPI: {1} with order -5000
2020-03-26 16:21:38.957 INFO [SpiLoader] Found {0} SPI: {1} with order -6000
2020-03-26 16:21:38.957 INFO [SpiLoader] Found {0} SPI: {1} with order -2000
2020-03-26 16:21:38.957 INFO [SpiLoader] Found {0} SPI: {1} with order -1000
2020-03-26 16:21:38.958 INFO [SpiLoader] Found {0} SPI: {1} with order -3000
2020-03-26 16:21:38.958 INFO [SpiLoader] Found {0} SPI: {1} with order -4000

[cdfive]

Here we need to replace the old placeholder with "{}":
https://github.com/alibaba/Sentinel/blob/master/sentinel-core/src/main/java/com/alibaba/csp/sentinel/util/SpiLoader.java#L222

I'm sorry not merge master before commit. May I post new PR to fix it?

[cdfive]

LGTM. The name loadDifferentInstanceListSorted might be confusing, maybe we could improve it later :)

How about naming the method loadPrototypeInstanceListSorted? In @scope of Spring,
there are singleton and prototype for singleton or multiple instances.

And we may need to improve the logs here because it will yield a large number of logs every time a new chain is built:

Yes, since every new chain need create new slot instance, here is not suitable for too many logs.
How about using RecordLog.debug instead of RecordLog.info here? Now I couldn't thought out a better way,is there any suggestions? @sczyh30 @jasonjoo2010

[jasonjoo2010]

LGTM. The name loadDifferentInstanceListSorted might be confusing, maybe we could improve it later :)

How about naming the method loadPrototypeInstanceListSorted? In @scope of Spring,
there are singleton and prototype for singleton or multiple instances.

And we may need to improve the logs here because it will yield a large number of logs every time a new chain is built:

Yes, since every new chain need create new slot instance, here is not suitable for too many logs.
How about using RecordLog.debug instead of RecordLog.info here? Now I couldn't thought out a better way,is there any suggestions? @sczyh30 @jasonjoo2010

I think it can be in trace or debug level

[jasonjoo2010]

And coming to the logging please use placeholders {} instead of joining of strings

[jasonjoo2010]

And coming to the logging please use placeholders {} instead of joining of strings

... and i noticed you mentioned about new PR. Have you ever used rebase ? You can rebase your branch with an up to date master. It's suggested that making a backup before that. eg.

git checkout master
git pull github master     <<<<< make master branch up to date
git checkout slot-order   <<<<< backup it to another temporary branch first
git rebase master   <<<< maybe you should solve some conflicts

Where github is a remote name of alibaba/Sentinel

[cdfive]

@jasonjoo2010 Thanks for detailed guideline! Previously I just use rebase in local, push -f to my forked repository and then post a PR in the UI of github site. Now the previous PR has been merged, could use rebase master? I'm worry about the safey.

[jasonjoo2010]

@jasonjoo2010 Thanks for detailed guideline! Previously I just use rebase in local, push -f to my forked repository and then post a PR in the UI of github site. Now the previous PR has been merged, could use rebase master? I'm worry about the safey.

Yes rebase in your own repo. Is it dangerous? But it has been merged so the only thing you can do is to remove it :)

More polishings should lead to a new PR as you mentioned.

[cdfive]

Very clear! Got it, thanks! @jasonjoo2010