Skip to content

Close the rally report file after parsing#3407

Open
buley wants to merge 2 commits into
elastic:mainfrom
buley:buley/fix-rally-report-close
Open

Close the rally report file after parsing#3407
buley wants to merge 2 commits into
elastic:mainfrom
buley:buley/fix-rally-report-close

Conversation

@buley

@buley buley commented Mar 24, 2026

Copy link
Copy Markdown

Summary

Close the CSV report file after parsing the Rally output instead of returning with the descriptor still open. This factors the parsing path through a small helper so the close happens on every path and adds unit coverage for both the happy path and close failures.

Testing

  • go test ./internal/benchrunner/runners/rally

@cla-checker-service

cla-checker-service Bot commented Mar 24, 2026

Copy link
Copy Markdown

💚 CLA has been signed

@buley

buley commented Mar 24, 2026

Copy link
Copy Markdown
Author

I have signed the CLA 👍🏻

@jsoriano jsoriano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey, thanks for fixing this! Added an small suggestion.

}

reader := csv.NewReader(reportCSV)
return parseRallyReport(reportCSV, r.svcInfo.Logs.Folder.Local, errOutput.String())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The defer should rather be placed in the same method that opens the file. This avoids introducing an unexpected side effect in the function that just parses what is in the reader.

Suggested change
return parseRallyReport(reportCSV, r.svcInfo.Logs.Folder.Local, errOutput.String())
defer reportCSV.Close()
return parseRallyReport(reportCSV, r.svcInfo.Logs.Folder.Local, errOutput.String())

@buley buley Mar 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion @jsoriano. I moved defer reportCSV.Close() back into runRally immediately after os.Open, so the function that opens the file also owns closing it. parseRallyReport now takes an io.Reader and only parses the CSV, which removes the side effect you called out. I also updated the test to verify parser output rather than close behavior.

@jsoriano

Copy link
Copy Markdown
Member

/test

@elasticmachine

elasticmachine commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

💔 Build Failed

Failed CI Steps

History

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