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.

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

[ISSUES 111]Flow rule cache/check #253

Closed
wants to merge 10 commits into from

Conversation

binbin0325
Copy link
Collaborator

Describe what this PR does / why we need it

In order to avoid the meaningless updates to the property, downstream of property manager should
cache last update value to check consistency.

Does this pull request fix one issue?

Fix:#111

Describe how you did it

Using reflect.deepequal,maybe we need to think about performance

Describe how to verify it

ut

Special notes for reviews

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2020

Codecov Report

Merging #253 into master will increase coverage by 0.41%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #253      +/-   ##
==========================================
+ Coverage   43.06%   43.48%   +0.41%     
==========================================
  Files          79       79              
  Lines        3994     3997       +3     
==========================================
+ Hits         1720     1738      +18     
+ Misses       2009     1997      -12     
+ Partials      265      262       -3     
Impacted Files Coverage Δ
core/flow/rule_manager.go 61.50% <100.00%> (+0.44%) ⬆️
ext/datasource/datasource.go 88.88% <100.00%> (+33.33%) ⬆️
ext/datasource/helper.go 60.00% <0.00%> (+4.21%) ⬆️
ext/datasource/property.go 89.47% <0.00%> (+10.52%) ⬆️
ext/datasource/error.go 80.00% <0.00%> (+60.00%) ⬆️

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 4e8f5a0...6192303. Read the comment docs.

return nil
}

// LoadRules loads the given flow rules to the rule manager, while all previous rules will be replaced.
func LoadRules(rules []*Rule) (bool, error) {
// TODO: rethink the design
if isEqual := reflect.DeepEqual(currentRules, rules); isEqual {
return isEqual, errors.New("The current Rules is the same as the rules to be loaded")
Copy link
Collaborator

Choose a reason for hiding this comment

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

it should return false, nil

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

@louyuting
Copy link
Collaborator

@sczyh30 How do you think about this design?

@louyuting
Copy link
Collaborator

Could you please solve conflicts?

# Conflicts:
#	core/flow/rule_manager.go
#	core/flow/rule_manager_test.go
…ng into flow_rule_cache

# Conflicts:
#	core/config/config.go
#	core/flow/slot.go
#	core/flow/slot_test.go
#	core/flow/standalone_stat_slot.go
#	core/flow/tc_default.go
#	core/flow/traffic_shaping.go
#	core/isolation/slot.go
#	core/log/metric/writer.go
#	core/stat/stat_slot.go
#	example/isolation/concurrency_limitation_example.go
#	ext/datasource/file/refreshable_file.go
#	ext/datasource/helper_test.go
#	ext/datasource/nacos/nacos_test.go
#	logging/logging.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants