-
Notifications
You must be signed in to change notification settings - Fork 238
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
apis/nfd: drop the private template caching fields #1477
apis/nfd: drop the private template caching fields #1477
Conversation
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
fca8ee8
to
dfc8bf2
Compare
/assign @ArangoGutierrez |
Drop the private fields – that were supposed to be used for caching parsed templates – from the Rule type. Keep the API typedefs cleaner and simpler. Moreover, the caching was not even used in practice, effectively complicating code without any benefit: the way the types are used in nfd-master creates a local copy of Rule type storing the cached template in the copy, wasting it from any future users. There are also other possible caveats in caching like we tried to do it. For example the objects returned by the api lister are supposed to be treated as read-only - in particular if we would be to modify them there should at least be proper locking in place as nfd-master potentially processes the same rule (the same Go object) in parallel for multiple nodes. If any optimization like this will be pursued it should be done properly, probably with private type(s) at the consumer's end, not contaminating the API types.
dfc8bf2
to
b2d9e15
Compare
Rebased |
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.
/lgtm
LGTM label has been added. Git tree hash: af19b85f6b1a1416a3f46ba140182973dcdd8dc4
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ArangoGutierrez, marquiz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1477 +/- ##
==========================================
+ Coverage 31.39% 32.53% +1.13%
==========================================
Files 59 59
Lines 7555 7810 +255
==========================================
+ Hits 2372 2541 +169
- Misses 4939 5014 +75
- Partials 244 255 +11
|
Drop the private fields – that were supposed to be used for caching parsed templates – from the Rule type. Keep the API typedefs cleaner and simpler. Moreover, the caching was not even used in practice, effectively complicating code without any benefit: the way the types are used in nfd-master creates a local copy of Rule type storing the cached template in the copy, wasting it from any future users.
There are also other possible caveats in caching like we tried to do it. For example the objects returned by the api lister are supposed to be treated as read-only - in particular if we would be to modify them there should at least be proper locking in place as nfd-master potentially processes the same rule (the same Go object) in parallel for multiple nodes. If any optimization like this will be pursued it should be done properly, probably with private type(s) at the consumer's end, not contaminating the API types.