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

mobile: allow overriding dns cache key value save interval #25325

Merged
merged 13 commits into from
Feb 3, 2023
Merged

mobile: allow overriding dns cache key value save interval #25325

merged 13 commits into from
Feb 3, 2023

Conversation

jpsim
Copy link
Contributor

@jpsim jpsim commented Feb 2, 2023

In Lyft's usage, we have up to 14 calls per second to save DNS results to the key value store. Buffering these to save at most once per second should be sufficient.

Commit Message: mobile: set dns cache key value save interval to 1s
Additional Description: In Lyft's usage, we have up to 14 calls per second to save DNS results to the key value store. Buffering these to save at most once per second should be sufficient.
Risk Level: Low
Testing: In Lyft's apps
Docs Changes: Added
Release Notes: Added
Platform Specific Features: N/A

In Lyft's usage, we have up to 14 calls per second to save DNS results
to the key value store. Buffering these to save at most once per second
should be sufficient.

Signed-off-by: JP Simard <jp@jpsim.com>
@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #25325 was opened by jpsim.

see: more, trace.

@jpsim jpsim marked this pull request as ready for review February 2, 2023 21:16
RyanTheOptimist
RyanTheOptimist previously approved these changes Feb 2, 2023
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

LGTM, though Chrome's analogous timer is 60s[1] so we could probably use a larger value if we wanted to limit IO.

  1. https://source.chromium.org/chromium/chromium/src/+/main:net/http/http_server_properties.cc;l=34;drc=2a7fa9b6065bba9cbe273ddc4f5e5a4a51bb450d;bpv=1;bpt=1

@jpsim
Copy link
Contributor Author

jpsim commented Feb 2, 2023

Oops, marked as ready to review too quickly, tests are failing. I'll update that.

I'll expose a builder parameter to set the interval since different apps might want different values. For us, 1s is fine and 60s would be too long, since storing the results in a sqlite db should be very efficient with our kinds of data, and we have app sessions shorter than 60s.

@alyssawilk
Copy link
Contributor

@RyanTheOptimist are we using the DNS cache yet for YT? If not I'd be inclined to suggest we switch the time interval now, and TODO the builder change for 2 weeks just because there's a lot of builder changes in flight
if it'd affect us now, I'll deal with the merge conflicts :-P

@jpsim jpsim marked this pull request as draft February 2, 2023 21:27
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim added 6 commits February 2, 2023 17:50
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
@jpsim jpsim changed the title mobile: set dns cache key value save interval to 1s mobile: allow overriding dns cache key value save interval Feb 3, 2023
jpsim added 3 commits February 3, 2023 09:21
Signed-off-by: JP Simard <jp@jpsim.com>
This reverts commit 43ff04c.

I pushed this to the wrong branch by accident.

Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
@jpsim
Copy link
Contributor Author

jpsim commented Feb 3, 2023

I've wired this through all the builder paths to keep things in sync as much as possible and minimize thrash with the proto builder changes.

@jpsim jpsim marked this pull request as ready for review February 3, 2023 14:34
@jpsim jpsim requested a review from RyanTheOptimist February 3, 2023 14:34
RyanTheOptimist
RyanTheOptimist previously approved these changes Feb 3, 2023
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Looks great! Should we have a test for the C++ builder too?

@jpsim
Copy link
Contributor Author

jpsim commented Feb 3, 2023

Looks great! Should we have a test for the C++ builder too?

Yes, I'll add that.

and fix type from `bool` to `int`.

Signed-off-by: JP Simard <jp@jpsim.com>
RyanTheOptimist
RyanTheOptimist previously approved these changes Feb 3, 2023
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

Signed-off-by: JP Simard <jp@jpsim.com>
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Woo hoo!

@jpsim jpsim enabled auto-merge (squash) February 3, 2023 16:36
@jpsim jpsim merged commit 754aa5b into envoyproxy:main Feb 3, 2023
@jpsim jpsim deleted the mobile-set-dns-cache-key-value-save-interval-to-1s branch February 3, 2023 18:24
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