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

[core] Use boost iostream in pipe logger for cross-platform #50044

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Jan 23, 2025

This PR resolves a TODO item, which leverages boost iostreams to achieve cross-platform.

Signed-off-by: dentiny <[email protected]>
@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Jan 23, 2025
@dentiny dentiny force-pushed the hjiang/boost-windows-jan-23 branch from e72abed to c2c4418 Compare January 23, 2025 21:22
@dentiny dentiny force-pushed the hjiang/boost-windows-jan-23 branch from c2c4418 to 8bdd94a Compare January 23, 2025 21:25
src/ray/util/pipe_logger.cc Outdated Show resolved Hide resolved
src/ray/util/pipe_logger.cc Outdated Show resolved Hide resolved
src/ray/util/pipe_logger.cc Outdated Show resolved Hide resolved
src/ray/util/pipe_logger.h Show resolved Hide resolved
@dentiny dentiny requested a review from jjyao January 24, 2025 01:55
Signed-off-by: dentiny <[email protected]>
src/ray/util/pipe_logger.cc Show resolved Hide resolved
src/ray/util/pipe_logger.cc Outdated Show resolved Hide resolved
src/ray/util/pipe_logger.cc Outdated Show resolved Hide resolved
src/ray/util/pipe_logger.h Show resolved Hide resolved

namespace {

TEST_P(PipeLoggerTest, PipeWrite) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually why removing this test? Shouldn't we just make it run on all platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reason is, the test case is made for (1) pipe write, and check pipe size's effect; (2) log rotation.
Remove because (1) pipe is no longer used in the code; (2) log rotation has been tested elsewhere.

Signed-off-by: dentiny <[email protected]>
@dentiny dentiny requested a review from jjyao January 24, 2025 20:08
src/ray/util/pipe_logger.cc Outdated Show resolved Hide resolved
src/ray/util/pipe_logger.cc Outdated Show resolved Hide resolved
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
@dentiny dentiny requested a review from jjyao January 24, 2025 21:14
src/ray/util/pipe_logger.cc Outdated Show resolved Hide resolved
src/ray/util/pipe_logger.cc Outdated Show resolved Hide resolved
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
@dentiny dentiny force-pushed the hjiang/boost-windows-jan-23 branch from 6e0e559 to 5d74185 Compare January 25, 2025 01:43
@jjyao
Copy link
Collaborator

jjyao commented Jan 25, 2025

[2025-01-25T02:42:31Z] //src/ray/util/tests:stream_redirection_utils_test FAILED in 3 out of 3 in 2.2s

@dentiny dentiny force-pushed the hjiang/boost-windows-jan-23 branch from bc12f0b to e6c6ce1 Compare January 25, 2025 08:26
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants