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

Add a cache for decrypted values #240

Merged
merged 2 commits into from
Jul 14, 2017
Merged

Conversation

stlava
Copy link

@stlava stlava commented Jul 10, 2017

Addresses #239

@rnelson0
Copy link
Member

@stlava I've fixed the tests, if you can rebase this to make sure it passes.

I do like the idea in #239 of an option to control whether caching is enabled. Do you think you could add that to this PR?

@stlava
Copy link
Author

stlava commented Jul 12, 2017

@rnelson0 I added an option. It didn't seem very necessary to include this part of the sub-command options but let me know.

Copy link
Member

@rnelson0 rnelson0 left a comment

Choose a reason for hiding this comment

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

This looks great! I'm hoping @diranged can give us some feedback shortly so we can see how well this works. If there's no response within a week, I think we then merge it and revisit performance later if it's suffering. Thanks!

@stlava
Copy link
Author

stlava commented Jul 13, 2017

@rnelson0 I work with @diranged and took over the ticket. I deployed this yesterday at 22:00 utc. Here's the trend:
average config retrieval times

@rnelson0
Copy link
Member

Oh, cool! That's noticeable, but not that drastic of a change. Is your compilation time coming from something else or does the caching need more work?

Happy to merge it, thanks for the contribution!

@rnelson0 rnelson0 merged commit 3067cc6 into voxpupuli:master Jul 14, 2017
@stlava
Copy link
Author

stlava commented Jul 14, 2017

Adding the caching was a step forward for us to cut down the compilation time but as you pointed out the rest of our time does come from us hitting other external resources. Thanks for getting this in!

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.

2 participants