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

fix: only load config and rules once #1470

Merged
merged 6 commits into from
Jan 23, 2025
Merged

Conversation

TylerHelmuth
Copy link
Contributor

Which problem is this PR solving?

We found an issue where refinery, as part of its config load process on startup or reload, would load the config/rules (whether from file or http or any), multiple times. This was happening because we the config data was passed around as a reader and if we ever needed the data a second time (which we do for validation vs unmarshaling into config) we had to go get another reader.

Short description of the changes

  • updates newFileConfig to first grab the data of all config locations and the use this data for all validation and unmarshalling. This prevents newFileConfig from needing to grab the data multiple times
  • updated some unit tests, but most needed unchanged.

@TylerHelmuth TylerHelmuth requested a review from a team as a code owner January 15, 2025 22:58
Copy link
Contributor

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

Personally, I think I would have done it differently by keeping the io.ReadCloser abstraction but changing it to a io.ReadSeekerCloser so it could be reset to the beginning to re-read it. I believe this would have been a smaller change, but also, I think what you did here is fine and don't need to force you to change it.

However, because Reader is a well-known abstraction in Go with a specific meaning, and this is no longer one of those, if you prefer to do it this way, I'd like to change the name used in all these functions. Instead of getReaderFor, maybe getDataBufferFor? or getContentsFor? And similarly for other functions and the new type you created.

Also, there are a couple of wrong comments now. (Like, "we read the file twice").

@TylerHelmuth
Copy link
Contributor Author

TylerHelmuth commented Jan 16, 2025

I didn't know about ReadSeekerCloser, thats exactly what I wanted to do but couldnt find a way to reset a reader. I'll switch to that

@TylerHelmuth
Copy link
Contributor Author

Reopenning because I would've had to make my own implementation of io.ReadSeekCloser for the resp.Body case since bytes.NewReader is only an io.Reader.

config/configLoadHelpers.go Outdated Show resolved Hide resolved
config/file_config.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

Thank you! I'm happy now.

@VinozzZ VinozzZ merged commit 570b6c2 into main Jan 23, 2025
5 checks passed
@VinozzZ VinozzZ deleted the tyler.reduce-config-loads branch January 23, 2025 15:52
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