-
Notifications
You must be signed in to change notification settings - Fork 14
[Framework] Add ingress cache #70
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
Changes from 12 commits
dd7328f
34ebe66
a356561
59daf75
530f90d
e68a6d0
11beb6d
53c4afa
b4eaa16
a0488ec
5100bc2
d1741bc
5f6ef19
e18346a
3722cc5
d8d3fac
6b78f0d
3199365
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,124 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Copyright 2019 Iguazio Systems Ltd. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Copyright 2019 Iguazio Systems Ltd. | |
| Copyright 2025 Iguazio Systems Ltd. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here - CR comment - Copyright
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe remove name from SetFunctionName and others? So when we move to typed trie values (not [string]), it will return either canaryFunction or singleFunction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should keep using functionName — it's a commonly used convention and clearly describes the value the trie holds.
But as @TomerShor recommended here, I will change the GetFunctionName specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weilerN I think we will need to rename when code is moved from using []string to structs, which will be with function prefix in the name (e.g canaryFunction, singleFunction). Though naming can be reconsidered in the following PR
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top lvl entity up
| type IngressHostsTree interface { | |
| SetFunctionName(path string, function string) error // will overwrite existing values if exists | |
| DeleteFunctionName(path string, function string) error | |
| GetFunctionName(path string) ([]string, error) | |
| IsEmpty() bool | |
| } | |
| type IngressHostCache interface { | |
| Set(host string, path string, function string) error // will overwrite existing values if exists | |
| Delete(host string, path string, function string) error | |
| Get(host string, path string) ([]string, error) | |
| } | |
| type IngressHostCache interface { | |
| Set(host string, path string, function string) error // will overwrite existing values if exists | |
| Delete(host string, path string, function string) error | |
| Get(host string, path string) ([]string, error) | |
| } | |
| type IngressHostsTree interface { | |
| SetFunctionName(path string, function string) error // will overwrite existing values if exists | |
| DeleteFunctionName(path string, function string) error | |
| GetFunctionName(path string) ([]string, error) | |
| IsEmpty() bool | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TomerShor marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
TomerShor marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use ic.syncMap.LoadOrStore() instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid using syncMap.Delete on every failure, I chose to keep the result from syncMap.Load and only store it at the end of the positive flow.
Fixed here - CR comment - improve usage of syncMap and slice
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even more explicit:
| return errors.Wrap(err, "cache set failed") | |
| return errors.Wrap(err, "Failed to set function name in the ingress host tree") |
The caller of Set will return an error Failed to set function in cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ic.logger.DebugWith("cache delete: host removed as it is empty", | |
| "host", host) | |
| ic.syncMap.Delete(host) | |
| ic.syncMap.Delete(host) | |
| ic.logger.DebugWith("cache delete: host removed as it is empty", | |
| "host", host) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here
Uh oh!
There was an error while loading. Please reload this page.