From a87199d240bdecaaa703f1af0d8499cf05ec843f Mon Sep 17 00:00:00 2001 From: mehrdadn Date: Sat, 14 Mar 2020 12:44:53 -0700 Subject: [PATCH] Fix cyclic dependency between ray/util and ray/common (#7581) * Fix cyclic dependency Headers in ray/util should not depend on those in ray/common * Move random generations to ray/common/test_util.h * Add license header Co-authored-by: Mehrdad Co-authored-by: Philipp Moritz --- BUILD.bazel | 1 + src/ray/common/status.cc | 11 +++ src/ray/common/status.h | 12 +++ .../{util/test_util.h => common/test_util.cc} | 75 +++++++------------ src/ray/common/test_util.h | 69 +++++++++++++++++ src/ray/core_worker/test/core_worker_test.cc | 3 +- .../test/direct_actor_transport_test.cc | 2 +- .../test/direct_task_transport_test.cc | 2 +- src/ray/core_worker/test/mock_worker.cc | 2 +- src/ray/core_worker/test/task_manager_test.cc | 2 +- .../test/service_based_gcs_client_test.cc | 3 +- .../gcs_server/test/gcs_server_rpc_test.cc | 2 +- src/ray/gcs/test/accessor_test_base.h | 3 +- src/ray/gcs/test/asio_test.cc | 5 +- .../test/redis_actor_info_accessor_test.cc | 3 +- src/ray/gcs/test/redis_gcs_client_test.cc | 2 +- .../gcs/test/redis_job_info_accessor_test.cc | 3 +- .../test/redis_object_info_accessor_test.cc | 3 +- src/ray/raylet/lineage_cache_test.cc | 9 +-- .../raylet/task_dependency_manager_test.cc | 9 +-- src/ray/util/util.h | 12 --- streaming/src/test/mock_actor.cc | 10 +-- 22 files changed, 151 insertions(+), 92 deletions(-) rename src/ray/{util/test_util.h => common/test_util.cc} (51%) create mode 100644 src/ray/common/test_util.h diff --git a/BUILD.bazel b/BUILD.bazel index bcc2e861a..106c706a2 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -843,6 +843,7 @@ cc_library( "@com_github_google_glog//:glog", "@com_google_absl//absl/synchronization", "@com_google_absl//absl/time", + "@com_google_googletest//:gtest_main", "@plasma//:plasma_client", ], ) diff --git a/src/ray/common/status.cc b/src/ray/common/status.cc index ac8263ae8..40657d394 100644 --- a/src/ray/common/status.cc +++ b/src/ray/common/status.cc @@ -31,6 +31,8 @@ #include +#include + namespace ray { #define STATUS_CODE_OK "OK" @@ -104,4 +106,13 @@ std::string Status::ToString() const { return result; } +Status boost_to_ray_status(const boost::system::error_code &error) { + switch (error.value()) { + case boost::system::errc::success: + return Status::OK(); + default: + return Status::IOError(strerror(error.value())); + } +} + } // namespace ray diff --git a/src/ray/common/status.h b/src/ray/common/status.h index 144fa5501..f119e7d9c 100644 --- a/src/ray/common/status.h +++ b/src/ray/common/status.h @@ -37,6 +37,16 @@ #include "ray/util/macros.h" #include "ray/util/visibility.h" +namespace boost { + +namespace system { + +class error_code; + +} // namespace system + +} // namespace boost + // Return the given status if it is not OK. #define RAY_RETURN_NOT_OK(s) \ do { \ @@ -239,6 +249,8 @@ inline void Status::operator=(const Status &s) { } } +Status boost_to_ray_status(const boost::system::error_code &error); + } // namespace ray #endif // RAY_STATUS_H_ diff --git a/src/ray/util/test_util.h b/src/ray/common/test_util.cc similarity index 51% rename from src/ray/util/test_util.h rename to src/ray/common/test_util.cc index f0868b938..683827046 100644 --- a/src/ray/util/test_util.h +++ b/src/ray/common/test_util.cc @@ -12,29 +12,39 @@ // See the License for the specific language governing permissions and // limitations under the License. -#ifndef RAY_UTIL_TEST_UTIL_H -#define RAY_UTIL_TEST_UTIL_H +#include "ray/common/test_util.h" -#include +#include -#include - -#include "gtest/gtest.h" #include "ray/common/buffer.h" -#include "ray/common/id.h" #include "ray/common/ray_object.h" -#include "ray/util/util.h" +#include "ray/util/logging.h" namespace ray { -// Magic argument to signal to mock_worker we should check message order. -int64_t SHOULD_CHECK_MESSAGE_ORDER = 123450000; +void RedisServiceManagerForTest::SetUpTestCase() { + auto seed = std::chrono::high_resolution_clock::now().time_since_epoch().count(); + std::mt19937 gen(seed); + std::uniform_int_distribution random_gen{2000, 7000}; + // Use random port to avoid port conflicts between UTs. + REDIS_SERVER_PORT = random_gen(gen); + + std::string start_redis_command = + REDIS_SERVER_EXEC_PATH + " --loglevel warning --loadmodule " + + REDIS_MODULE_LIBRARY_PATH + " --port " + std::to_string(REDIS_SERVER_PORT) + " &"; + RAY_LOG(INFO) << "Start redis command is: " << start_redis_command; + RAY_CHECK(system(start_redis_command.c_str()) == 0); + usleep(200 * 1000); +} + +void RedisServiceManagerForTest::TearDownTestCase() { + std::string stop_redis_command = + REDIS_CLIENT_EXEC_PATH + " -p " + std::to_string(REDIS_SERVER_PORT) + " shutdown"; + RAY_LOG(INFO) << "Stop redis command is: " << stop_redis_command; + RAY_CHECK(system(stop_redis_command.c_str()) == 0); + usleep(100 * 1000); +} -/// Wait until the condition is met, or timeout is reached. -/// -/// \param[in] condition The condition to wait for. -/// \param[in] timeout_ms Timeout in milliseconds to wait for for. -/// \return Whether the condition is met. bool WaitForCondition(std::function condition, int timeout_ms) { int wait_time = 0; while (true) { @@ -53,8 +63,7 @@ bool WaitForCondition(std::function condition, int timeout_ms) { return false; } -// A helper function to return a random task id. -inline TaskID RandomTaskId() { +TaskID RandomTaskId() { std::string data(TaskID::Size(), 0); FillRandom(&data); return TaskID::FromBinary(data); @@ -71,7 +80,7 @@ std::shared_ptr GenerateRandomBuffer() { } std::shared_ptr GenerateRandomObject( - const std::vector &inlined_ids = {}) { + const std::vector &inlined_ids) { return std::shared_ptr( new RayObject(GenerateRandomBuffer(), nullptr, inlined_ids)); } @@ -85,34 +94,4 @@ std::string REDIS_MODULE_LIBRARY_PATH; /// Port of redis server. int REDIS_SERVER_PORT; -/// Test helper class, it will start redis server before the test runs, -/// and stop redis server after the test is completed. -class RedisServiceManagerForTest : public ::testing::Test { - public: - static void SetUpTestCase() { - auto seed = std::chrono::high_resolution_clock::now().time_since_epoch().count(); - std::mt19937 gen(seed); - std::uniform_int_distribution random_gen{2000, 7000}; - // Use random port to avoid port conflicts between UTs. - REDIS_SERVER_PORT = random_gen(gen); - - std::string start_redis_command = - REDIS_SERVER_EXEC_PATH + " --loglevel warning --loadmodule " + - REDIS_MODULE_LIBRARY_PATH + " --port " + std::to_string(REDIS_SERVER_PORT) + " &"; - RAY_LOG(INFO) << "Start redis command is: " << start_redis_command; - RAY_CHECK(system(start_redis_command.c_str()) == 0); - usleep(200 * 1000); - } - - static void TearDownTestCase() { - std::string stop_redis_command = - REDIS_CLIENT_EXEC_PATH + " -p " + std::to_string(REDIS_SERVER_PORT) + " shutdown"; - RAY_LOG(INFO) << "Stop redis command is: " << stop_redis_command; - RAY_CHECK(system(stop_redis_command.c_str()) == 0); - usleep(100 * 1000); - } -}; - } // namespace ray - -#endif // RAY_UTIL_TEST_UTIL_H diff --git a/src/ray/common/test_util.h b/src/ray/common/test_util.h new file mode 100644 index 000000000..342462f5a --- /dev/null +++ b/src/ray/common/test_util.h @@ -0,0 +1,69 @@ +// Copyright 2017 The Ray Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef RAY_COMMON_TEST_UTIL_H +#define RAY_COMMON_TEST_UTIL_H + +#include + +#include +#include + +#include "gtest/gtest.h" +#include "ray/common/id.h" +#include "ray/util/util.h" + +namespace ray { + +class Buffer; +class RayObject; + +// Magic argument to signal to mock_worker we should check message order. +static const int64_t SHOULD_CHECK_MESSAGE_ORDER = 123450000; + +/// Wait until the condition is met, or timeout is reached. +/// +/// \param[in] condition The condition to wait for. +/// \param[in] timeout_ms Timeout in milliseconds to wait for for. +/// \return Whether the condition is met. +bool WaitForCondition(std::function condition, int timeout_ms); + +// A helper function to return a random task id. +TaskID RandomTaskId(); + +std::shared_ptr GenerateRandomBuffer(); + +std::shared_ptr GenerateRandomObject( + const std::vector &inlined_ids = {}); + +/// Path to redis server executable binary. +extern std::string REDIS_SERVER_EXEC_PATH; +/// Path to redis client executable binary. +extern std::string REDIS_CLIENT_EXEC_PATH; +/// Path to redis module library. +extern std::string REDIS_MODULE_LIBRARY_PATH; +/// Port of redis server. +extern int REDIS_SERVER_PORT; + +/// Test helper class, it will start redis server before the test runs, +/// and stop redis server after the test is completed. +class RedisServiceManagerForTest : public ::testing::Test { + public: + static void SetUpTestCase(); + static void TearDownTestCase(); +}; + +} // namespace ray + +#endif // RAY_UTIL_TEST_UTIL_H diff --git a/src/ray/core_worker/test/core_worker_test.cc b/src/ray/core_worker/test/core_worker_test.cc index 6d597b893..c4bdc80ef 100644 --- a/src/ray/core_worker/test/core_worker_test.cc +++ b/src/ray/core_worker/test/core_worker_test.cc @@ -27,14 +27,13 @@ #include "hiredis/hiredis.h" #include "ray/common/buffer.h" #include "ray/common/ray_object.h" +#include "ray/common/test_util.h" #include "ray/core_worker/context.h" #include "ray/core_worker/store_provider/memory_store/memory_store.h" #include "ray/core_worker/transport/direct_actor_transport.h" #include "ray/raylet/raylet_client.h" -#include "ray/util/test_util.h" #include "src/ray/protobuf/core_worker.pb.h" #include "src/ray/protobuf/gcs.pb.h" -#include "src/ray/util/test_util.h" namespace { diff --git a/src/ray/core_worker/test/direct_actor_transport_test.cc b/src/ray/core_worker/test/direct_actor_transport_test.cc index 60c774b8a..acb359f6f 100644 --- a/src/ray/core_worker/test/direct_actor_transport_test.cc +++ b/src/ray/core_worker/test/direct_actor_transport_test.cc @@ -15,11 +15,11 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include "ray/common/task/task_spec.h" +#include "ray/common/test_util.h" #include "ray/core_worker/store_provider/memory_store/memory_store.h" #include "ray/core_worker/transport/direct_task_transport.h" #include "ray/raylet/raylet_client.h" #include "ray/rpc/worker/core_worker_client.h" -#include "src/ray/util/test_util.h" namespace ray { diff --git a/src/ray/core_worker/test/direct_task_transport_test.cc b/src/ray/core_worker/test/direct_task_transport_test.cc index 41b0c856e..259a2c20e 100644 --- a/src/ray/core_worker/test/direct_task_transport_test.cc +++ b/src/ray/core_worker/test/direct_task_transport_test.cc @@ -17,10 +17,10 @@ #include "gtest/gtest.h" #include "ray/common/task/task_spec.h" #include "ray/common/task/task_util.h" +#include "ray/common/test_util.h" #include "ray/core_worker/store_provider/memory_store/memory_store.h" #include "ray/raylet/raylet_client.h" #include "ray/rpc/worker/core_worker_client.h" -#include "src/ray/util/test_util.h" namespace ray { diff --git a/src/ray/core_worker/test/mock_worker.cc b/src/ray/core_worker/test/mock_worker.cc index ad34cccaf..580ce99cd 100644 --- a/src/ray/core_worker/test/mock_worker.cc +++ b/src/ray/core_worker/test/mock_worker.cc @@ -13,9 +13,9 @@ // limitations under the License. #define BOOST_BIND_NO_PLACEHOLDERS +#include "ray/common/test_util.h" #include "ray/core_worker/context.h" #include "ray/core_worker/core_worker.h" -#include "src/ray/util/test_util.h" using namespace std::placeholders; diff --git a/src/ray/core_worker/test/task_manager_test.cc b/src/ray/core_worker/test/task_manager_test.cc index 2bc05b210..291f8143c 100644 --- a/src/ray/core_worker/test/task_manager_test.cc +++ b/src/ray/core_worker/test/task_manager_test.cc @@ -16,10 +16,10 @@ #include "gtest/gtest.h" #include "ray/common/task/task_spec.h" +#include "ray/common/test_util.h" #include "ray/core_worker/actor_manager.h" #include "ray/core_worker/reference_count.h" #include "ray/core_worker/store_provider/memory_store/memory_store.h" -#include "ray/util/test_util.h" namespace ray { diff --git a/src/ray/gcs/gcs_client/test/service_based_gcs_client_test.cc b/src/ray/gcs/gcs_client/test/service_based_gcs_client_test.cc index 2dab31b13..bd4bbfe60 100644 --- a/src/ray/gcs/gcs_client/test/service_based_gcs_client_test.cc +++ b/src/ray/gcs/gcs_client/test/service_based_gcs_client_test.cc @@ -13,11 +13,12 @@ // limitations under the License. #include "ray/gcs/gcs_client/service_based_gcs_client.h" + #include "gtest/gtest.h" +#include "ray/common/test_util.h" #include "ray/gcs/gcs_client/service_based_accessor.h" #include "ray/gcs/gcs_server/gcs_server.h" #include "ray/rpc/gcs_server/gcs_rpc_client.h" -#include "ray/util/test_util.h" namespace ray { diff --git a/src/ray/gcs/gcs_server/test/gcs_server_rpc_test.cc b/src/ray/gcs/gcs_server/test/gcs_server_rpc_test.cc index c7ad75447..2c459bd27 100644 --- a/src/ray/gcs/gcs_server/test/gcs_server_rpc_test.cc +++ b/src/ray/gcs/gcs_server/test/gcs_server_rpc_test.cc @@ -13,9 +13,9 @@ // limitations under the License. #include "gtest/gtest.h" +#include "ray/common/test_util.h" #include "ray/gcs/gcs_server/gcs_server.h" #include "ray/rpc/gcs_server/gcs_rpc_client.h" -#include "ray/util/test_util.h" namespace ray { diff --git a/src/ray/gcs/test/accessor_test_base.h b/src/ray/gcs/test/accessor_test_base.h index 7612a7f6a..ca7defa30 100644 --- a/src/ray/gcs/test/accessor_test_base.h +++ b/src/ray/gcs/test/accessor_test_base.h @@ -20,10 +20,11 @@ #include #include #include + #include "gtest/gtest.h" +#include "ray/common/test_util.h" #include "ray/gcs/redis_accessor.h" #include "ray/gcs/redis_gcs_client.h" -#include "ray/util/test_util.h" namespace ray { diff --git a/src/ray/gcs/test/asio_test.cc b/src/ray/gcs/test/asio_test.cc index f5a0178e5..5410bb67e 100644 --- a/src/ray/gcs/test/asio_test.cc +++ b/src/ray/gcs/test/asio_test.cc @@ -12,12 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include "ray/gcs/asio.h" + #include #include "gtest/gtest.h" -#include "ray/gcs/asio.h" +#include "ray/common/test_util.h" #include "ray/util/logging.h" -#include "ray/util/test_util.h" extern "C" { #include "hiredis/async.h" diff --git a/src/ray/gcs/test/redis_actor_info_accessor_test.cc b/src/ray/gcs/test/redis_actor_info_accessor_test.cc index 20f81b420..704a4c1c1 100644 --- a/src/ray/gcs/test/redis_actor_info_accessor_test.cc +++ b/src/ray/gcs/test/redis_actor_info_accessor_test.cc @@ -17,10 +17,11 @@ #include #include #include + #include "gtest/gtest.h" +#include "ray/common/test_util.h" #include "ray/gcs/redis_gcs_client.h" #include "ray/gcs/test/accessor_test_base.h" -#include "ray/util/test_util.h" namespace ray { diff --git a/src/ray/gcs/test/redis_gcs_client_test.cc b/src/ray/gcs/test/redis_gcs_client_test.cc index 186096b88..75f915e39 100644 --- a/src/ray/gcs/test/redis_gcs_client_test.cc +++ b/src/ray/gcs/test/redis_gcs_client_test.cc @@ -20,10 +20,10 @@ extern "C" { } #include "ray/common/ray_config.h" +#include "ray/common/test_util.h" #include "ray/gcs/pb_util.h" #include "ray/gcs/redis_gcs_client.h" #include "ray/gcs/tables.h" -#include "ray/util/test_util.h" namespace ray { diff --git a/src/ray/gcs/test/redis_job_info_accessor_test.cc b/src/ray/gcs/test/redis_job_info_accessor_test.cc index 6d2666424..1c55adeab 100644 --- a/src/ray/gcs/test/redis_job_info_accessor_test.cc +++ b/src/ray/gcs/test/redis_job_info_accessor_test.cc @@ -13,11 +13,12 @@ // limitations under the License. #include + #include "gtest/gtest.h" +#include "ray/common/test_util.h" #include "ray/gcs/pb_util.h" #include "ray/gcs/redis_gcs_client.h" #include "ray/gcs/test/accessor_test_base.h" -#include "ray/util/test_util.h" namespace ray { diff --git a/src/ray/gcs/test/redis_object_info_accessor_test.cc b/src/ray/gcs/test/redis_object_info_accessor_test.cc index 7d61252e0..cac6c19d8 100644 --- a/src/ray/gcs/test/redis_object_info_accessor_test.cc +++ b/src/ray/gcs/test/redis_object_info_accessor_test.cc @@ -14,11 +14,12 @@ #include #include + #include "gtest/gtest.h" +#include "ray/common/test_util.h" #include "ray/gcs/redis_accessor.h" #include "ray/gcs/redis_gcs_client.h" #include "ray/gcs/test/accessor_test_base.h" -#include "ray/util/test_util.h" namespace ray { diff --git a/src/ray/raylet/lineage_cache_test.cc b/src/ray/raylet/lineage_cache_test.cc index 305953e4f..f380c5378 100644 --- a/src/ray/raylet/lineage_cache_test.cc +++ b/src/ray/raylet/lineage_cache_test.cc @@ -12,25 +12,22 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include "ray/raylet/lineage_cache.h" + #include #include #include "gmock/gmock.h" #include "gtest/gtest.h" - #include "ray/common/task/task.h" #include "ray/common/task/task_execution_spec.h" #include "ray/common/task/task_spec.h" #include "ray/common/task/task_util.h" - +#include "ray/common/test_util.h" #include "ray/gcs/callback.h" #include "ray/gcs/redis_accessor.h" #include "ray/gcs/redis_gcs_client.h" - #include "ray/raylet/format/node_manager_generated.h" -#include "ray/raylet/lineage_cache.h" - -#include "ray/util/test_util.h" namespace ray { diff --git a/src/ray/raylet/task_dependency_manager_test.cc b/src/ray/raylet/task_dependency_manager_test.cc index a8ff38cd0..f63561c77 100644 --- a/src/ray/raylet/task_dependency_manager_test.cc +++ b/src/ray/raylet/task_dependency_manager_test.cc @@ -12,18 +12,17 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include "ray/raylet/task_dependency_manager.h" + +#include #include #include "gmock/gmock.h" #include "gtest/gtest.h" - -#include - #include "ray/common/task/task_util.h" +#include "ray/common/test_util.h" #include "ray/gcs/redis_accessor.h" #include "ray/gcs/redis_gcs_client.h" -#include "ray/raylet/task_dependency_manager.h" -#include "ray/util/test_util.h" namespace ray { diff --git a/src/ray/util/util.h b/src/ray/util/util.h index b67af6d16..a6fd6f194 100644 --- a/src/ray/util/util.h +++ b/src/ray/util/util.h @@ -15,7 +15,6 @@ #ifndef RAY_UTIL_UTIL_H #define RAY_UTIL_UTIL_H -#include #include #include #include @@ -25,8 +24,6 @@ #include #include -#include "ray/common/status.h" - /// Return the number of milliseconds since the steady clock epoch. NOTE: The /// returned timestamp may be used for accurately measuring intervals but has /// no relation to wall clock time. It must not be used for synchronization @@ -43,15 +40,6 @@ inline int64_t current_time_ms() { return ms_since_epoch.count(); } -inline ray::Status boost_to_ray_status(const boost::system::error_code &error) { - switch (error.value()) { - case boost::system::errc::success: - return ray::Status::OK(); - default: - return ray::Status::IOError(strerror(error.value())); - } -} - /// A helper function to split a string by whitespaces. /// /// \param str The string with whitespaces. diff --git a/streaming/src/test/mock_actor.cc b/streaming/src/test/mock_actor.cc index 84d2b20b1..5762d81ff 100644 --- a/streaming/src/test/mock_actor.cc +++ b/streaming/src/test/mock_actor.cc @@ -1,17 +1,15 @@ #define BOOST_BIND_NO_PLACEHOLDERS -#include "ray/core_worker/context.h" -#include "ray/core_worker/core_worker.h" -#include "src/ray/util/test_util.h" - #include "data_reader.h" #include "data_writer.h" +#include "gtest/gtest.h" #include "message/message.h" #include "message/message_bundle.h" #include "queue/queue_client.h" +#include "ray/common/test_util.h" +#include "ray/core_worker/context.h" +#include "ray/core_worker/core_worker.h" #include "ring_buffer.h" #include "status.h" - -#include "gtest/gtest.h" using namespace std::placeholders; const uint32_t MESSAGE_BOUND_SIZE = 10000;