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

server: remove RandomGenerator from everywhere except Api #13360

Merged
merged 4 commits into from
Oct 6, 2020

Conversation

akonradi
Copy link
Contributor

@akonradi akonradi commented Oct 1, 2020

Commit Message: Remove RandomGenerator from everywhere except Api
Additional Description:
Remove the RandomGenerator accessor on Server and change the remaining call sites to
use Api::randomGenerator().

Risk Level: low
Testing: ran all affected tests
Docs Changes: n/a
Release Notes: n/a
Fixes: #13243

Replaces accesses to RandomGenerator with the equivalent calls through
Api::Api::randomGenerator().

Signed-off-by: Alex Konradi <[email protected]>
Remove the RandomGenerator accessor on Server and change the remaining
call sites to use Api::randomGenerator().

Signed-off-by: Alex Konradi <[email protected]>
@akonradi
Copy link
Contributor Author

akonradi commented Oct 2, 2020

This appears to be failing coverage because enough covered lines were removed to drop the source/server/config_validation directory below the threshold. Would it be okay to reduce the limit in this PR?

This PR makes no functional changes but removes some code that was
previously covered, which dropped the measured coverage fraction below
the previous limit. Decrease the limit to accommodate the refactoring.

Signed-off-by: Alex Konradi <[email protected]>
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

this is fine I think. I didn't think the previous state was that bad though. Taken too far toward bundling everything in one giant context collection and passing that everywhere results in arguably a lot of unnecessary dependencies across layers, but I didn't see that here.

Like, if I need to have some functionality that requires random numbers, I wouldn't want to have to construct a thread factory and whatever else API needs in order to get that tunneled down. So I'm a little leary of this but I'll pass this to @envoyproxy/senior-maintainers as it doesn't look too bad yet.

@akonradi
Copy link
Contributor Author

akonradi commented Oct 6, 2020

Agreed, and there are still places where RandomGenerator& is passed independent of Api::Api&. This refactoring was about making RandomGenerator accessible in more places without having to add additional constructor arguments everywhere.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Overall this seems like an improvement to me. Thanks!

@mattklein123 mattklein123 merged commit 22683a0 into envoyproxy:master Oct 6, 2020
@akonradi akonradi deleted the random-via-api-final branch October 6, 2020 15:40
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.

Expose RandomGenerator via Api interface
3 participants