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

[rosout] Proper logrotation for rosout #991

Closed
wants to merge 1 commit into from

Conversation

mgrrx
Copy link
Contributor

@mgrrx mgrrx commented Feb 16, 2017

This PR fixes #854 by rotating the log files as usual in Linux to guarantee the ordering of log files (https://en.wikipedia.org/wiki/Log_rotation).
As discussed previously, this PR does not touch the initializers of the class and targets the Luna release.

@mikepurvis
Copy link
Member

Summoning @gregwjacobs who is likely to have an opinion on this.

It'd be great to have some tests covering this behaviour but otherwise LGTM.

@mgrrx
Copy link
Contributor Author

mgrrx commented Feb 17, 2017

I also thought about writing a test for this functionality but with the current implementation we always need to write at least 100MB to the disk to see that the log rotates once - which I think is too much for a test. I'd therefor expose the max file size parameter as a ros parameter or command line argument and write the test with a reduced file size.
Do you prefer to have the test together with this PR or should I create a new ticket and PR the test?

@dirk-thomas
Copy link
Member

I have cherry-picked your patch to the newly created lunar-devel branch: ce4d726

In that process I have removed the unrelated whitespace change (https://github.com/ros/ros_comm/pull/991/files#diff-c21b75d2d000be7cceac00629a0a9e23R94) as well as readded the output to cout informing the user that the log is being rotated (ce4d726#diff-c21b75d2d000be7cceac00629a0a9e23R181).

Thank you!

@dirk-thomas
Copy link
Member

It would be great if you could create a separate PR for making the limit configurable as well as cover the feature with tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants