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.

Sign up for GitHub

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

Jump to bottom

Refactor slot chain mechanism and introduce slot order to rank #318

Merged
merged 7 commits into from
Nov 15, 2020
Merged

Refactor slot chain mechanism and introduce slot order to rank #318

merged 7 commits into from
Nov 15, 2020

Conversation

luckyxiaoqiang
Copy link
Collaborator

@luckyxiaoqiang luckyxiaoqiang commented Nov 8, 2020 •
edited
Loading

Describe what this PR does / why we need it

Support slot order.

Slots in a slot chain are sorted by order value of slots in increasing order. Default slot chain has a fixed default order and have no relation to the add sequence. User extended slot can set the order to make it be in front of or at the back of a sentinel build-in slot easily.

Does this pull request fix one issue?

#317

Describe how you did it

Describe how to verify it

Special notes for reviews

@louyuting louyuting self-requested a review November 9, 2020 01:49
@louyuting louyuting added this to the 1.0.0 milestone Nov 9, 2020
@louyuting louyuting added the kind/enhancement Category issues or PRs related to enhancement label Nov 9, 2020
@sczyh30 sczyh30 added kind/refactor Issue related to functional refactoring. to-review PRs to review labels Nov 9, 2020
@sczyh30 sczyh30 self-requested a review November 9, 2020 03:42
louyuting
louyuting reviewed Nov 9, 2020
View reviewed changes
core/base/slot_chain.go Outdated Show resolved Hide resolved
louyuting
louyuting reviewed Nov 9, 2020
View reviewed changes
core/base/slot_chain.go Outdated Show resolved Hide resolved
louyuting
louyuting reviewed Nov 10, 2020
View reviewed changes
core/base/slot_chain.go Outdated Show resolved Hide resolved
@louyuting
Copy link
Collaborator

Some suggestion about default order of embedded slot.

  1. In each bucket(PrepareStatSlot Bucket、CheckSlot Bucket、StatSlot Bucket), The default order should from local to external。Such as CheckSlot, it should be system、flow、isolation、hotspot、circuitbreaker;
  2. The StatSlot order should keep consistent with CheckSlot bucket。 Such as: flowStat、hotspotStat、circuitbreakerStat.

@codecov-io
Copy link

Codecov Report

Merging #318 (7a17278) into master (14ddaf9) will increase coverage by 0.02%.
The diff coverage is 72.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
+ Coverage   50.36%   50.39%   +0.02%     
==========================================
  Files          68       68              
  Lines        3709     3717       +8     
==========================================
+ Hits         1868     1873       +5     
- Misses       1578     1582       +4     
+ Partials      263      262       -1
Impacted Files Coverage Δ
core/circuitbreaker/slot.go 94.11% <0.00%> (-5.89%) ⬇️
core/flow/slot.go 43.33% <0.00%> (-1.50%) ⬇️
core/hotspot/slot.go 25.00% <0.00%> (-0.72%) ⬇️
core/isolation/slot.go 0.00% <0.00%> (ø)
core/system/slot.go 53.06% <0.00%> (-1.11%) ⬇️
core/misc/resource_slot_chain.go 37.73% <44.44%> (ø)
api/slot_chain.go 76.47% <100.00%> (ø)
core/base/slot_chain.go 97.70% <100.00%> (ø)
core/circuitbreaker/stat_slot.go 25.00% <100.00%> (+10.71%) ⬆️
core/flow/standalone_stat_slot.go 75.00% <100.00%> (+3.57%) ⬆️
... and 2 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 14ddaf9...7a17278. Read the comment docs.

@luckyxiaoqiang
Copy link
Collaborator Author

Some suggestion about default order of embedded slot.

  1. In each bucket(PrepareStatSlot Bucket、CheckSlot Bucket、StatSlot Bucket), The default order should from local to external。Such as CheckSlot, it should be system、flow、isolation、hotspot、circuitbreaker;
  2. The StatSlot order should keep consistent with CheckSlot bucket。 Such as: flowStat、hotspotStat、circuitbreakerStat.

Agree. Refined.

@louyuting
Copy link
Collaborator

@liqiangz @sanxun0325 Could you please take a look on this PR ?

binbin0325
binbin0325 reviewed Nov 12, 2020
View reviewed changes
core/circuitbreaker/slot.go
@@ -5,7 +5,8 @@ import (
)

const (
RuleCheckSlotName = "sentinel-core-circuit-breaker-rule-check-slot"
RuleCheckSlotName = "sentinel-core-circuit-breaker-rule-check-slot"
RuleCheckSlotOrder = 5000
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is better to define the order of slots in a uniform location, which may seem more intuitive。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My first version is like what you recommended, current version is the result of discussion with @louyuting .

core/stat/stat_prepare_slot.go
@@ -5,7 +5,8 @@ import (
)

const (
PrepareSlotName = "sentinel-core-stat-resource-node-prepare-slot"
PrepareSlotName = "sentinel-core-stat-resource-node-prepare-slot"
PrepareSlotOrder = 1000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the definition of a slot order, starting at 1000, have any particular meaning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Leave a gap for the user to add extended slots before it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Leave a gap for the user to add extended slots before it.

Yes,The main thing I'm trying to say is the order of slots 1000, why isn't it 2000, or 10000.There should be a specification for it. I don't know if there are any other better ideas for defining the slot order that Sentinel provides by default。

sczyh30
sczyh30 approved these changes Nov 15, 2020
View reviewed changes
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

@louyuting louyuting merged commit b98d989 into alibaba:master Nov 15, 2020
@louyuting louyuting changed the title Support slot order Refactor slot chain mechanism and introduce slot order to rank Nov 15, 2020
@louyuting louyuting linked an issue Nov 15, 2020 that may be closed by this pull request
Make Slot implementation with Order semantic #317
Closed
@louyuting louyuting mentioned this pull request Nov 15, 2020
Closed
@sczyh30 sczyh30 removed the to-review PRs to review label Nov 23, 2020
@luckyxiaoqiang luckyxiaoqiang deleted the slot_order branch December 27, 2020 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@binbin0325 binbin0325 binbin0325 left review comments

@louyuting louyuting louyuting approved these changes

@sczyh30 sczyh30 sczyh30 approved these changes

Assignees
No one assigned
Labels
kind/enhancement Category issues or PRs related to enhancement kind/refactor Issue related to functional refactoring.
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.

Make Slot implementation with Order semantic
5 participants
@luckyxiaoqiang @louyuting @codecov-io @sczyh30 @binbin0325