From f51d82274a838842c0b4c9163841f5e4ce312285 Mon Sep 17 00:00:00 2001 From: dentiny Date: Thu, 23 Jan 2025 04:58:25 +0000 Subject: [PATCH] Use statusor for filesystem and resolve dependency Signed-off-by: dentiny --- src/ray/common/BUILD | 6 +++- src/ray/common/buffer.h | 1 + src/ray/common/status.h | 34 +++++------------- src/ray/common/status_or.h | 19 ---------- src/ray/common/test/BUILD | 1 + src/ray/util/BUILD | 22 +++++++----- src/ray/util/filesystem.cc | 13 +++---- src/ray/util/filesystem.h | 6 ++-- src/ray/util/logging.h | 36 +++++++++++++++++++ src/ray/util/tests/BUILD | 2 ++ src/ray/util/tests/pipe_logger_test.cc | 26 ++++++++++---- .../tests/stream_redirection_utils_test.cc | 9 +++-- 12 files changed, 103 insertions(+), 72 deletions(-) diff --git a/src/ray/common/BUILD b/src/ray/common/BUILD index f5e6b9d8b9cd1..06638d2f014a6 100644 --- a/src/ray/common/BUILD +++ b/src/ray/common/BUILD @@ -283,7 +283,11 @@ ray_cc_library( hdrs = ["status.h"], deps = [ ":source_location", - "//src/ray/util", + "//src/ray/util:macros", + "//src/ray/util:visibility", + "@boost//:system", + "@com_google_absl//absl/container:flat_hash_map", + "@com_google_absl//absl/strings", ], ) diff --git a/src/ray/common/buffer.h b/src/ray/common/buffer.h index 0ce0829d0af73..a4607e60ce292 100644 --- a/src/ray/common/buffer.h +++ b/src/ray/common/buffer.h @@ -21,6 +21,7 @@ #include "ray/common/status.h" #include "ray/thirdparty/aligned_alloc.h" +#include "ray/util/logging.h" #define BUFFER_ALIGNMENT 64 diff --git a/src/ray/common/status.h b/src/ray/common/status.h index e8206ba90aaf6..920c6c49468fe 100644 --- a/src/ray/common/status.h +++ b/src/ray/common/status.h @@ -32,36 +32,14 @@ #include #include +#include "absl/strings/str_cat.h" #include "ray/common/source_location.h" -#include "ray/util/logging.h" #include "ray/util/macros.h" #include "ray/util/visibility.h" -namespace boost { - -namespace system { - +namespace boost::system { class error_code; - -} // namespace system - -} // namespace boost - -// Return the given status if it is not OK. -#define RAY_RETURN_NOT_OK(s) \ - do { \ - const ::ray::Status &_s = (s); \ - if (RAY_PREDICT_FALSE(!_s.ok())) { \ - return _s; \ - } \ - } while (0) - -// If the status is not OK, CHECK-fail immediately, appending the status to the -// logged message. The message can be appended with <<. -#define RAY_CHECK_OK(s) \ - if (const ::ray::Status &_status_ = (s); true) \ - RAY_CHECK_WITH_DISPLAY(_status_.ok(), #s) \ - << "Status not OK: " << _status_.ToString() << " " +} // namespace boost::system namespace ray { @@ -328,6 +306,12 @@ class RAY_EXPORT Status { std::string message() const { return ok() ? "" : state_->msg; } + template + Status &operator<<(T &&...msg) { + absl::StrAppend(&state_->msg, std::forward(msg)...); + return *this; + } + private: struct State { StatusCode code; diff --git a/src/ray/common/status_or.h b/src/ray/common/status_or.h index 7b0cc0d65a769..29b8976f0fc98 100644 --- a/src/ray/common/status_or.h +++ b/src/ray/common/status_or.h @@ -20,27 +20,8 @@ #include #include "absl/base/attributes.h" -#include "ray/common/macros.h" #include "ray/common/status.h" -#define __RAY_ASSIGN_OR_RETURN_IMPL(var, expr, statusor_name) \ - auto statusor_name = (expr); \ - RAY_RETURN_NOT_OK(statusor_name.status()); \ - var = std::move(statusor_name).value() - -// Use `RAY_UNIQUE_VARIABLE` to add line number into variable name, since we could have -// multiple macros used in one code block. -#define RAY_ASSIGN_OR_RETURN(var, expr) \ - __RAY_ASSIGN_OR_RETURN_IMPL(var, expr, RAY_UNIQUE_VARIABLE(statusor)) - -#define __RAY_ASSIGN_OR_CHECK_IMPL(var, expr, statusor_name) \ - auto statusor_name = (expr); \ - RAY_CHECK_OK(statusor_name.status()); \ - var = std::move(statusor_name).value() - -#define RAY_ASSIGN_OR_CHECK(var, expr) \ - __RAY_ASSIGN_OR_CHECK_IMPL(var, expr, RAY_UNIQUE_VARIABLE(statusor)) - namespace ray { template diff --git a/src/ray/common/test/BUILD b/src/ray/common/test/BUILD index 75f15f741c6ba..08bf2a2695238 100644 --- a/src/ray/common/test/BUILD +++ b/src/ray/common/test/BUILD @@ -160,6 +160,7 @@ ray_cc_test( ray_cc_library( name = "testing", hdrs = ["testing.h"], + deps = ["//src/ray/util:macros"], testonly = True, ) diff --git a/src/ray/util/BUILD b/src/ray/util/BUILD index 2afc20d3fa84b..b2c388475a93e 100644 --- a/src/ray/util/BUILD +++ b/src/ray/util/BUILD @@ -40,26 +40,30 @@ ray_cc_library( # TODO(hjiang): filesystem and logging has interdependency, we should split them into three targets: filesystem, logging, ray_check_macros. ray_cc_library( name = "logging", - hdrs = [ - "filesystem.h", - "logging.h", - ], - srcs = [ - "filesystem.cc", - "logging.cc", - ], + hdrs = ["logging.h"], + srcs = ["logging.cc"], deps = [ ":event_label", + ":filesystem", ":macros", ":thread_utils", "@com_github_spdlog//:spdlog", "@com_google_absl//absl/debugging:failure_signal_handler", "@com_google_absl//absl/strings:str_format", - "@com_google_googletest//:gtest_main", + "@com_google_googletest//:gtest_prod", "@nlohmann_json", ], ) +ray_cc_library( + name = "filesystem", + hdrs = ["filesystem.h"], + srcs = ["filesystem.cc"], + deps = [ + "//src/ray/common:status_or", + ], +) + ray_cc_library( name = "container_util", hdrs = ["container_util.h"], diff --git a/src/ray/util/filesystem.cc b/src/ray/util/filesystem.cc index 9d4e2fd163d90..b96d6b0c35a1b 100644 --- a/src/ray/util/filesystem.cc +++ b/src/ray/util/filesystem.cc @@ -17,8 +17,6 @@ #include #include -#include "ray/util/logging.h" - #ifdef _WIN32 #include #endif @@ -57,17 +55,20 @@ std::string GetUserTempDir() { while (!result.empty() && IsDirSep(result.back())) { result.pop_back(); } - RAY_CHECK(!result.empty()); return result; } -std::string CompleteReadFile(const std::string &fname) { +StatusOr CompleteReadFile(const std::string &fname) { std::ifstream file(fname); - RAY_CHECK(file.good()) << "Fails to open file " << fname; + if (!file.good()) { + return Status::IOError("") << "Fails to open file " << fname; + } std::ostringstream buffer; buffer << file.rdbuf(); - RAY_CHECK(file.good()) << "Fails to read from file " << fname; + if (!file.good()) { + return Status::IOError("") << "Fails to read from file " << fname; + } std::string content = buffer.str(); file.close(); diff --git a/src/ray/util/filesystem.h b/src/ray/util/filesystem.h index beed7387b102a..b1968fb2a254e 100644 --- a/src/ray/util/filesystem.h +++ b/src/ray/util/filesystem.h @@ -19,6 +19,8 @@ #include #include +#include "ray/common/status_or.h" + // Filesystem and path manipulation APIs. // (NTFS stream & attribute paths are not supported.) @@ -63,8 +65,6 @@ std::string JoinPaths(std::string base, const Paths &...components) { // Read the whole content for the given [fname], and return as string. // If any error happens, error message will be logged and the program will exit // immediately. -// -// TODO(hjiang): Use StatusOr as return type in the followup PR. -std::string CompleteReadFile(const std::string &fname); +StatusOr CompleteReadFile(const std::string &fname); } // namespace ray diff --git a/src/ray/util/logging.h b/src/ray/util/logging.h index 8b6401f99be2a..d2d6968d842b3 100644 --- a/src/ray/util/logging.h +++ b/src/ray/util/logging.h @@ -233,6 +233,42 @@ enum class RayLogLevel { ? ray::RayLogLevel::level \ : ray::RayLogLevel::otherLevel) +// Ray status macros. +// Return the given status if it is not OK. +#define RAY_RETURN_NOT_OK(s) \ + do { \ + const ::ray::Status &_s = (s); \ + if (RAY_PREDICT_FALSE(!_s.ok())) { \ + return _s; \ + } \ + } while (0) + +// If the status is not OK, CHECK-fail immediately, appending the status to the +// logged message. The message can be appended with <<. +#define RAY_CHECK_OK(s) \ + if (const ::ray::Status &_status_ = (s); true) \ + RAY_CHECK_WITH_DISPLAY(_status_.ok(), #s) \ + << "Status not OK: " << _status_.ToString() << " " + +// Ray StatusOr macros. +#define __RAY_ASSIGN_OR_RETURN_IMPL(var, expr, statusor_name) \ + auto statusor_name = (expr); \ + RAY_RETURN_NOT_OK(statusor_name.status()); \ + var = std::move(statusor_name).value() + +// Use `RAY_UNIQUE_VARIABLE` to add line number into variable name, since we could have +// multiple macros used in one code block. +#define RAY_ASSIGN_OR_RETURN(var, expr) \ + __RAY_ASSIGN_OR_RETURN_IMPL(var, expr, RAY_UNIQUE_VARIABLE(statusor)) + +#define __RAY_ASSIGN_OR_CHECK_IMPL(var, expr, statusor_name) \ + auto statusor_name = (expr); \ + RAY_CHECK_OK(statusor_name.status()); \ + var = std::move(statusor_name).value() + +#define RAY_ASSIGN_OR_CHECK(var, expr) \ + __RAY_ASSIGN_OR_CHECK_IMPL(var, expr, RAY_UNIQUE_VARIABLE(statusor)) + // To make the logging lib plugable with other logging libs and make // the implementation unawared by the user, RayLog is only a declaration // which hide the implementation into logging.cc file. diff --git a/src/ray/util/tests/BUILD b/src/ray/util/tests/BUILD index 7159a0f61a69f..7e8608e90e19d 100644 --- a/src/ray/util/tests/BUILD +++ b/src/ray/util/tests/BUILD @@ -217,6 +217,7 @@ ray_cc_test( name = "pipe_logger_test", srcs = ["pipe_logger_test.cc"], deps = [ + "//src/ray/common/test:testing", "//src/ray/util", "//src/ray/util:pipe_logger", "@com_google_googletest//:gtest_main", @@ -230,6 +231,7 @@ ray_cc_test( name = "stream_redirection_utils_test", srcs = ["stream_redirection_utils_test.cc"], deps = [ + "//src/ray/common/test:testing", "//src/ray/util", "//src/ray/util:stream_redirection_utils", "@com_google_googletest//:gtest_main", diff --git a/src/ray/util/tests/pipe_logger_test.cc b/src/ray/util/tests/pipe_logger_test.cc index cfa3f149d19d3..96114bab33210 100644 --- a/src/ray/util/tests/pipe_logger_test.cc +++ b/src/ray/util/tests/pipe_logger_test.cc @@ -23,6 +23,7 @@ #include #include "absl/cleanup/cleanup.h" +#include "ray/common/test/testing.h" #include "ray/util/filesystem.h" #include "ray/util/util.h" @@ -61,7 +62,7 @@ TEST_P(PipeLoggerTest, NoPipeWrite) { stream_redirection_handle.Close(); // Check log content after completion. - const auto actual_content = CompleteReadFile(test_file_path); + RAY_ASSIGN_OR_EXPECT(const auto actual_content, CompleteReadFile(test_file_path)); const std::string expected_content = absl::StrFormat("%s%s", kLogLine1, kLogLine2); EXPECT_EQ(actual_content, expected_content); } @@ -114,8 +115,13 @@ TEST_P(PipeLoggerTest, PipeWrite) { stream_redirection_handle.Close(); // Check log content after completion. - EXPECT_EQ(CompleteReadFile(log_file_path1), kLogLine2); - EXPECT_EQ(CompleteReadFile(log_file_path2), kLogLine1); + const auto actual_content1 = CompleteReadFile(test_file_path1); + RAY_ASSERT_OK(actual_content1); + EXPECT_EQ(*actual_content1, kLogLine2); + + const auto actual_content2 = CompleteReadFile(test_file_path2); + RAY_ASSERT_OK(actual_content1); + EXPECT_EQ(*actual_content2, kLogLine1); } TEST(PipeLoggerTestWithTee, RedirectionWithTee) { @@ -145,8 +151,9 @@ TEST(PipeLoggerTestWithTee, RedirectionWithTee) { EXPECT_EQ(stdout_content, absl::StrFormat("%s%s", kLogLine1, kLogLine2)); // Check log content after completion. - EXPECT_EQ(CompleteReadFile(test_file_path), - absl::StrFormat("%s%s", kLogLine1, kLogLine2)); + const auto actual_content = CompleteReadFile(test_file_path); + RAY_ASSERT_OK(actual_content); + EXPECT_EQ(*actual_content, absl::StrFormat("%s%s", kLogLine1, kLogLine2)); } TEST(PipeLoggerTestWithTee, RotatedRedirectionWithTee) { @@ -181,8 +188,13 @@ TEST(PipeLoggerTestWithTee, RotatedRedirectionWithTee) { EXPECT_EQ(stderr_content, absl::StrFormat("%s%s", kLogLine1, kLogLine2)); // Check log content after completion. - EXPECT_EQ(CompleteReadFile(test_file_path), kLogLine2); - EXPECT_EQ(CompleteReadFile(log_file_path2), kLogLine1); + const auto actual_content1 = CompleteReadFile(test_file_path1); + RAY_ASSERT_OK(actual_content1); + EXPECT_EQ(*actual_content1, kLogLine2); + + const auto actual_content2 = CompleteReadFile(test_file_path2); + RAY_ASSERT_OK(actual_content2); + EXPECT_EQ(*actual_content2, kLogLine1); } } // namespace diff --git a/src/ray/util/tests/stream_redirection_utils_test.cc b/src/ray/util/tests/stream_redirection_utils_test.cc index f6995e68fde93..cdeb418be11db 100644 --- a/src/ray/util/tests/stream_redirection_utils_test.cc +++ b/src/ray/util/tests/stream_redirection_utils_test.cc @@ -22,6 +22,7 @@ #include #include +#include "ray/common/test/testing.h" #include "ray/util/filesystem.h" #include "ray/util/util.h" @@ -56,10 +57,14 @@ TEST(LoggingUtilTest, RedirectStderr) { // Check log content after completion. const std::string log_file_path1 = test_file_path; - EXPECT_EQ(CompleteReadFile(test_file_path), kLogLine2); + const auto actual_content1 = CompleteReadFile(log_file_path1); + RAY_ASSERT_OK(actual_content1); + EXPECT_EQ(*actual_content1, kLogLine2); const std::string log_file_path2 = absl::StrFormat("%s.1", test_file_path); - EXPECT_EQ(CompleteReadFile(log_file_path2), kLogLine1); + const auto actual_content2 = CompleteReadFile(log_file_path2); + RAY_ASSERT_OK(actual_content2); + EXPECT_EQ(*actual_content2, kLogLine1); // Check tee-ed to stderr content. std::string stderr_content = testing::internal::GetCapturedStderr();