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

Log file cleanup: Only delete temporary file if it was created #103

Merged
merged 1 commit into from Nov 4, 2020

Conversation

lfittl
Copy link
Member

@lfittl lfittl commented Nov 3, 2020

Due to the change in dc4a656 we may
enter cleanup in cases where the TmpFile wasn't actually initialized.

In such cases, we should just do nothing in cleanup.

Due to the change in dc4a656 we may
enter cleanup in cases where the TmpFile wasn't actually initialized.

In such cases, we should just do nothing in cleanup.
@lfittl lfittl requested a review from msakrejda November 3, 2020 17:51
@msakrejda
Copy link
Contributor

CI failure is the stream_test.go issue we've seen before; restarted the build for now.

@msakrejda
Copy link
Contributor

Restarted for the CI failure a few times now--I think this might be because in handleLogAnalysis we arrange the log lines by backend in a map, and then traverse that (unordered) map to build the result. If I understand this correctly, this is correct but not deterministic. I'm not sure how to solve this, though: attempting to sort in handleLogAnalysis just for determinism seems like overkill, and I don't know if there's a reasonable way to sort this in the test for verifying test behavior. Thoughts?

@lfittl lfittl merged commit 15660f2 into master Nov 4, 2020
@lfittl
Copy link
Member Author

lfittl commented Nov 4, 2020

@uhoh-itsmaciek I think we need to add an explicit sort in handleLogAnalysis, can't think of a better way to solve this unfortunately. I can look into that.

(merged now since CI did end up being green after a while)

@lfittl lfittl deleted the logs-processing-fix-nil-handling branch November 4, 2020 16:50
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.

None yet

2 participants