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 <noreply@github.com>
Co-authored-by: Philipp Moritz <pcmoritz@gmail.com>

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",
     ],
 )

src/ray/common/status.cc

@@ -31,6 +31,8 @@
 
 #include <assert.h>
 
+#include <boost/system/error_code.hpp>
+
 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

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_

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 <unistd.h>
+#include <functional>
 
-#include <string>
-
-#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<int> 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<bool()> condition, int timeout_ms) {
   int wait_time = 0;
   while (true) {
@@ -53,8 +63,7 @@ bool WaitForCondition(std::function<bool()> 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<Buffer> GenerateRandomBuffer() {
 }
 
 std::shared_ptr<RayObject> GenerateRandomObject(
-    const std::vector<ObjectID> &inlined_ids = {}) {
+    const std::vector<ObjectID> &inlined_ids) {
   return std::shared_ptr<RayObject>(
       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<int> 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

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 <unistd.h>
+
+#include <functional>
+#include <string>
+
+#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<bool()> condition, int timeout_ms);
+
+// A helper function to return a random task id.
+TaskID RandomTaskId();
+
+std::shared_ptr<Buffer> GenerateRandomBuffer();
+
+std::shared_ptr<RayObject> GenerateRandomObject(
+    const std::vector<ObjectID> &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

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 {
 

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 {
 

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 {
 

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;
 

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 {
 

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 {
 

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 {
 

src/ray/gcs/test/accessor_test_base.h

@@ -20,10 +20,11 @@
 #include <string>
 #include <thread>
 #include <vector>
+
 #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 {
 

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 <iostream>
 
 #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"

src/ray/gcs/test/redis_actor_info_accessor_test.cc

@@ -17,10 +17,11 @@
 #include <string>
 #include <thread>
 #include <vector>
+
 #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 {
 

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 {
 

src/ray/gcs/test/redis_job_info_accessor_test.cc

@@ -13,11 +13,12 @@
 // limitations under the License.
 
 #include <memory>
+
 #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 {
 

src/ray/gcs/test/redis_object_info_accessor_test.cc

@@ -14,11 +14,12 @@
 
 #include <unordered_map>
 #include <vector>
+
 #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 {
 

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 <list>
 #include <memory>
 
 #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 {
 

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 <boost/asio.hpp>
 #include <list>
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
-
-#include <boost/asio.hpp>
-
 #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 {
 

src/ray/util/util.h

@@ -15,7 +15,6 @@
 #ifndef RAY_UTIL_UTIL_H
 #define RAY_UTIL_UTIL_H
 
-#include <boost/system/error_code.hpp>
 #include <chrono>
 #include <iterator>
 #include <mutex>
@@ -25,8 +24,6 @@
 #include <thread>
 #include <unordered_map>
 
-#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.

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;