Refactor log-validator to break up the big main function. #7170
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The main function in log-validator is overly big, so this refactors it in preparation for adding support for file globs. Largely it's functionally identical, except for one change: The 1-per-second ratelimiter is moved to be per-file, so it's 1-per-second-per-file. I think this more closely aligns with what we'd want, as we could potentially miss that a file had bad lines in it if it was overwhelmed by another log file at the same time.
beautifulentropy
requested changes
Nov 22, 2023
|
|
||
| // ValidateFile validates a single file and returns | ||
| func ValidateFile(filename string) error { | ||
| file, err := os.ReadFile(filename) |
There was a problem hiding this comment.
Consider reading the file line-by-line instead of loading it all into memory:
file, err := os.Open(filename)
if err != nil {
return err
}
defer file.Close()
scanner := bufio.NewScanner(file)
badFile := false
lineNum := 0
for scanner.Scan() {
lineNum++
line := scanner.Text()
...There was a problem hiding this comment.
Oh, I almost forgot, the scanner has one part that's a little wonky:
// Check for any errors that occurred during the scan.
if err := scanner.Err()
if err != nil {
return fmt.Errorf("error reading file: %w", err)
}You'll want to do this after you've finished scanning a file, not inside of the loop.
There was a problem hiding this comment.
I actually have an idea to just get rid of this function completely: it’s not used at all during the regular operation of log-validator so is extra code duplication, but I didn’t want to make too many changes while moving things around. This function is currently unmodified from the source. So I’d like to just leave it as-is for now and eliminate it in the next patch in this series.
jsha
previously approved these changes
Nov 22, 2023
jsha
approved these changes
Nov 23, 2023
beautifulentropy
approved these changes
Nov 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main function in log-validator is overly big, so this refactors it in preparation for adding support for file globs, which is started in the (currently draft) #7134
This PR should be functionally a no-op, except for one change:
The 1-per-second ratelimiter is moved to be per-file, so it's 1-per-second-per-file. I think this more closely aligns with what we'd want, as we could potentially miss that a file had bad lines in it if it was overwhelmed by another log file at the same time.