Design of SlotChainBuilder

[jasonjoo2010]

Issue Description

SlotChainBuilder is used to decide the collection of modules with orders when doing a check.
And it's loaded and initialized in SlotChainProvider(which is called by CtSph) through SPI.
sentinel-core offers a default implementation DefaultSlotChainBuilder for fallback if no implementation found while both sentinel-parameter-flow-control and sentinel-api-gateway-adapter-common offer its own implementation(HotParamSlotChainBuilder and GatewaySlotChainBuilder).

If both of them are introduced which one would be the lucky one is undetermined.
So some of users may trigger the bug(refer to #1306 ).

Describe what you expected to happen

Following aspects should be considered:

• Orders of slots
• Multiple implementations
• Minimal dependency
• Enough capacity for user defined implementation
• Duplicated implementations (Maybe through user's specifying)

Possible designs

Generally speaking, considering backward compatibility, function and capacity of customizing, there are mainly three directions:

1. Make Slot as a service.

We could abandon SlotBuilder and turn to slots loading through SPI and introduce kind of @SpiOrder to it to make them sortable.

And existing attempts please refer to #316 #411

Which will not be supported or should be careful

Order value should be carefully assigned because new priority sensitive slots need available space of adjacent ones.
User-defined implementation can't resort slots.
It cannot deal with conflicts slots.

2. Enhance SlotBuilder supporting chaining

Make the builders support something like filters or listeners and maintain their own slots and orders like:

DefaultSlotBuilder introduces NodeSelecter -> ClusterBuilder -> Log -> Flow -> Degrade.
GatewaySlotBuilder introduces GatewayFlow after Log.
ParamFlowSlotBuilder introduces ParamFlow after Log.

Sorts can be implemented as

• getOrder in ProcessorSlot or AbstractProcessorSlot
• Annotation for each slot
• Introduce addSlotAt() to ProcessorSlotChain

For more capacity in user's implementations getSlots() / setSlots() can be introduced into ProcessorSlotChain.

Which should be careful

The scenario updated and legacy sentinel-* are mixed.
Maybe make it unworkable or compatible is acceptable.

3. Create NewSlotBuilder supporting chaining

No change on legacy definitions but the scenario mixed mentioned above will become harder to deal with.

Proposal

I am mainly on design 2.
Further discussion is welcomed.

[jasonjoo2010]

And i think it'd better to be enhanced or fixed in 1.7.2 if possible because it has already affected some of users.

[jasonjoo2010]

For backward compatibility a new keyword default in interface is introduced in java 8.
And spring-cloud-alibaba also declares the compiler working under java 8 at least.
We could use default to make it simpler if we update the configuration from java 7 to java 8.

Certainly we have other solutions if not.

[cdfive]

Very detailed description!
IMO, design 1 is suitable for most scenes, as it's same as Dubbo Filter.
Using @SpiOrder to it to make them sortable, since the number of slot is limited, usually the available space is ample.
If only one system SlotChainBuilder(DefaultSlotChainBuilder) exists, when user add one custom SlotChainBuilder, the SpiLoader.loadFirstInstanceOrDefault(SlotChainBuilder.class, DefaultSlotChainBuilder.class); method works well.

Introduce addSlotAt() to ProcessorSlotChain is a good idea, it will be useful when user custom the SlotChainBuilder with programing.
The order of default Sentinel slots is very important and should be careful when custom with programing.

[jasonjoo2010]

Very detailed description!
IMO, design 1 is suitable for most scenes, as it's same as Dubbo Filter.
Using @SpiOrder to it to make them sortable, since the number of slot is limited, usually the available space is ample.
If only one system SlotChainBuilder(DefaultSlotChainBuilder) exists, when user add one custom SlotChainBuilder, the SpiLoader.loadFirstInstanceOrDefault(SlotChainBuilder.class, DefaultSlotChainBuilder.class); method works well.

Introduce addSlotAt() to ProcessorSlotChain is a good idea, it will be useful when user custom the SlotChainBuilder with programing.
The order of default Sentinel slots is very important and should be careful when custom with programing.

Sorry for latency.

I have read your design and summarize it follows:

• Make ProcessSlot a new SPI interface
• Preserve SlotChainBuilder as an SPI interface but remove all implementations except DefaultSlotChainBuilder
• Define SlotChainBuilder for customizing purpose totally and will not introduce more official implementation

Is that right?

It's a good practice i think if we don't have the historical debt.
Several issue we should dig further:

• Has anyone implemented his own Builder in project?
• Has anyone defined builder SPI declaration? Especially use the legacy ClusterChainSlotBuilder. How to solve it or decide?
• What will happen if different versions of sentinel-* packages are mixed in dependency tree? Will they work correctly, wrong, or refuse to work together and throw an exception?

Questions above is that we should carefully deal with.

[jasonjoo2010]

... And based on your proposal i have several improvement to make more scenarios met:

• Builders also need SpiOrder
• Remove old SPI exporting but preserve old implementations. We could make them inheriting DefaultChainSlotBuilder. Deprecating them is also necessary maybe. Don't forget instructions in descriptions.
• Take builders without @SpiOrder with high priority to use maybe
• Ban builder classes of HotParam and Cluster in BuilderProvider. If we found any of them a critical log or enforced exception should be given.

[jasonjoo2010]

... And there is another PR(#1226) that is relevant to this. So i suggest we should finish and close this issue fast.

@sczyh30 @cdfive

[jasonjoo2010]

@cdfive Could we make any conclusion or do you have better design?

Because there is a PR(#1226 1226) blocked by this and it has already make some changes on chain provider in sentinel-core.

@sczyh30 And could you review the final design based on @cdfive 's PR?

[sczyh30]

Resolved via #411. We may improve the design of slots (like #1363) in next versions.