-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
feat: add azure zone list cache #4811
feat: add azure zone list cache #4811
Conversation
Signed-off-by: tanujd11 <[email protected]>
Hi @tanujd11. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
provider/azure/cache.go
Outdated
} | ||
} | ||
|
||
// Get method to retrieve the cached zones |
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.
comments should add more value than writing what you can read.
In this example you can add if it's a shallow copy or not for example.
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.
Have added more concrete lines. Thanks
provider/azure/cache.go
Outdated
return z.zones | ||
} | ||
|
||
// Expired method to check if the cache has expired based on duration or if zones are empty |
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.
Why do we check for len(zones) would be interesting for the doc.
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.
Done, PTAL
|
||
// Expired method to check if the cache has expired based on duration or if zones are empty | ||
func (z *zonesCache[T]) Expired() bool { | ||
return len(z.zones) < 1 || time.Since(z.age) > z.duration |
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.
Why z.age and not time.Now() ?
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.
z.age is the time of last update. Basically the time passed since last update, if it is greater than the allowed duration, then we invalidate the cache. I am not sure what you mean how time.Now() will work here.
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.
Yeah sorry makes sense
/ok-to-test |
Signed-off-by: tanujd11 <[email protected]>
Signed-off-by: tanujd11 <[email protected]>
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: szuecs 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 |
Description
zoneListCache is introduced for Azure so that the azure ratelimiting can be countered with for this particular call. Although the ratelimit is higher for this particular call but in Azure there are some ratelimiters which are shared between various services for example this particular one comes in microsoft.network API provider. This has resulted in some API ratelimiting for this particular call for me even with a relatively lesser number of external DNS running(8). This particular call need not run on all the external DNS reconciliation anyways, it will improve the speed of the reconciliation as well.
Error is mentioned below:
Fixes #ISSUE
Checklist