From 34fa5fd2dab873340a5441493626e37224d203cb Mon Sep 17 00:00:00 2001 From: suquark Date: Thu, 10 Jan 2019 11:21:19 +0800 Subject: [PATCH 1/6] convert code to proper C++ --- src/ray/id.h | 10 +++ src/ray/raylet/lib/python/common_extension.cc | 8 -- src/ray/raylet/lib/python/raylet_extension.cc | 2 - src/ray/util/logging.cc | 84 ++----------------- src/ray/util/logging.h | 60 +++++++++++-- 5 files changed, 72 insertions(+), 92 deletions(-) diff --git a/src/ray/id.h b/src/ray/id.h index 0ab0c56408e7..ffea499c9e24 100644 --- a/src/ray/id.h +++ b/src/ray/id.h @@ -7,6 +7,16 @@ #include #include "plasma/common.h" +// The "arrow/util/logging.h" will define some macros +// that conflict with libraries like Google Log. Undefine them below. +#undef DCHECK +#undef DCHECK_NE +#undef DCHECK_EQ +#undef DCHECK_GE +#undef DCHECK_GT +#undef DCHECK_LE +#undef DCHECK_LT + #include "ray/constants.h" #include "ray/util/visibility.h" diff --git a/src/ray/raylet/lib/python/common_extension.cc b/src/ray/raylet/lib/python/common_extension.cc index ccfcf5cb3b12..b37443dfdcb0 100644 --- a/src/ray/raylet/lib/python/common_extension.cc +++ b/src/ray/raylet/lib/python/common_extension.cc @@ -58,8 +58,6 @@ void init_pickle_module(void) { RAY_CHECK(pickle_protocol != NULL); } -TaskBuilder *g_task_builder = NULL; - /* Define the PyObjectID class. */ int PyStringToUniqueID(PyObject *object, ObjectID *object_id) { @@ -135,12 +133,6 @@ PyObject *PyObjectID_make(ObjectID object_id) { return (PyObject *)result; } -TaskSpec *TaskSpec_copy(TaskSpec *spec, int64_t task_spec_size) { - TaskSpec *copy = (TaskSpec *)malloc(task_spec_size); - memcpy(copy, spec, task_spec_size); - return copy; -} - /** * Convert a string to a Ray task specification Python object. * diff --git a/src/ray/raylet/lib/python/raylet_extension.cc b/src/ray/raylet/lib/python/raylet_extension.cc index 755ddf1ddd1c..6105f2826f56 100644 --- a/src/ray/raylet/lib/python/raylet_extension.cc +++ b/src/ray/raylet/lib/python/raylet_extension.cc @@ -5,8 +5,6 @@ #include "config_extension.h" #include "ray/raylet/raylet_client.h" -PyObject *LocalSchedulerError; - // clang-format off typedef struct { PyObject_HEAD diff --git a/src/ray/util/logging.cc b/src/ray/util/logging.cc index 97c871d8cb7f..6c56b1931c36 100644 --- a/src/ray/util/logging.cc +++ b/src/ray/util/logging.cc @@ -1,70 +1,10 @@ #include "ray/util/logging.h" - -#ifndef _WIN32 -#include -#endif - #include -#include #include #include -#include - -#ifdef RAY_USE_GLOG -#include "glog/logging.h" -#endif namespace ray { -// This is the default implementation of ray log, -// which is independent of any libs. -class CerrLog { - public: - CerrLog(RayLogLevel severity) : severity_(severity), has_logged_(false) {} - - virtual ~CerrLog() { - if (has_logged_) { - std::cerr << std::endl; - } - if (severity_ == RayLogLevel::FATAL) { - PrintBackTrace(); - std::abort(); - } - } - - std::ostream &Stream() { - has_logged_ = true; - return std::cerr; - } - - template - CerrLog &operator<<(const T &t) { - if (severity_ != RayLogLevel::DEBUG) { - has_logged_ = true; - std::cerr << t; - } - return *this; - } - - protected: - const RayLogLevel severity_; - bool has_logged_; - - void PrintBackTrace() { -#if defined(_EXECINFO_H) || !defined(_WIN32) - void *buffer[255]; - const int calls = backtrace(buffer, sizeof(buffer) / sizeof(void *)); - backtrace_symbols_fd(buffer, calls, 1); -#endif - } -}; - -#ifdef RAY_USE_GLOG -typedef google::LogMessage LoggingProvider; -#else -typedef ray::CerrLog LoggingProvider; -#endif - RayLogLevel RayLog::severity_threshold_ = RayLogLevel::INFO; std::string RayLog::app_name_ = ""; std::string RayLog::log_dir_ = ""; @@ -186,38 +126,28 @@ bool RayLog::IsLevelEnabled(RayLogLevel log_level) { RayLog::RayLog(const char *file_name, int line_number, RayLogLevel severity) // glog does not have DEBUG level, we can handle it using is_enabled_. - : logging_provider_(nullptr), - is_enabled_(severity >= severity_threshold_) { + : is_enabled_(severity >= severity_threshold_) { #ifdef RAY_USE_GLOG if (is_enabled_) { - logging_provider_ = - new google::LogMessage(file_name, line_number, GetMappedSeverity(severity)); + logging_provider_.reset( + new google::LogMessage(file_name, line_number, GetMappedSeverity(severity))); } #else - auto logging_provider = new CerrLog(severity); - *logging_provider << file_name << ":" << line_number << ": "; - logging_provider_ = logging_provider; + logging_provider_.reset(new CerrLog(severity)); + *logging_provider_ << file_name << ":" << line_number << ": "; #endif } std::ostream &RayLog::Stream() { - auto logging_provider = reinterpret_cast(logging_provider_); #ifdef RAY_USE_GLOG // Before calling this function, user should check IsEnabled. // When IsEnabled == false, logging_provider_ will be empty. - return logging_provider->stream(); + return logging_provider_->stream(); #else - return logging_provider->Stream(); + return logging_provider_->Stream(); #endif } bool RayLog::IsEnabled() const { return is_enabled_; } -RayLog::~RayLog() { - if (logging_provider_ != nullptr) { - delete reinterpret_cast(logging_provider_); - logging_provider_ = nullptr; - } -} - } // namespace ray diff --git a/src/ray/util/logging.h b/src/ray/util/logging.h index 6f657142efd7..5f16a2846792 100644 --- a/src/ray/util/logging.h +++ b/src/ray/util/logging.h @@ -2,12 +2,66 @@ #define RAY_UTIL_LOGGING_H #include +#include #include +#ifndef _WIN32 +#include +#endif + +#ifdef RAY_USE_GLOG +#include "glog/logging.h" +typedef google::LogMessage LoggingProvider; +#else +typedef ray::CerrLog LoggingProvider; +#endif namespace ray { enum class RayLogLevel { DEBUG = -1, INFO = 0, WARNING = 1, ERROR = 2, FATAL = 3 }; +// This is the default implementation of ray log, +// which is independent of any libs. +class CerrLog { + public: + CerrLog(RayLogLevel severity) : severity_(severity), has_logged_(false) {} + + virtual ~CerrLog() { + if (has_logged_) { + std::cerr << std::endl; + } + if (severity_ == RayLogLevel::FATAL) { + PrintBackTrace(); + std::abort(); + } + } + + std::ostream &Stream() { + has_logged_ = true; + return std::cerr; + } + + template + CerrLog &operator<<(const T &t) { + if (severity_ != RayLogLevel::DEBUG) { + has_logged_ = true; + std::cerr << t; + } + return *this; + } + + protected: + const RayLogLevel severity_; + bool has_logged_; + + void PrintBackTrace() { +#if defined(_EXECINFO_H) || !defined(_WIN32) + void *buffer[255]; + const int calls = backtrace(buffer, sizeof(buffer) / sizeof(void *)); + backtrace_symbols_fd(buffer, calls, 1); +#endif + } +}; + #define RAY_LOG_INTERNAL(level) ::ray::RayLog(__FILE__, __LINE__, level) #define RAY_LOG(level) \ @@ -63,8 +117,6 @@ class RayLog : public RayLogBase { public: RayLog(const char *file_name, int line_number, RayLogLevel severity); - virtual ~RayLog(); - /// Return whether or not current logging instance is enabled. /// /// \return True if logging is enabled and false otherwise. @@ -98,9 +150,7 @@ class RayLog : public RayLogBase { static RayLogLevel GetLogLevelFromEnv(); private: - // Hide the implementation of log provider by void *. - // Otherwise, lib user may define the same macro to use the correct header file. - void *logging_provider_; + std::unique_ptr logging_provider_; /// True if log messages should be logged and false if they should be ignored. bool is_enabled_; static RayLogLevel severity_threshold_; From 645c3f8d1f6b7f1ac346b9c5e35dcb4b891b43e0 Mon Sep 17 00:00:00 2001 From: suquark Date: Tue, 15 Jan 2019 00:11:56 +0800 Subject: [PATCH 2/6] revert changes to "id.h" because #3765 has been merged. --- src/ray/id.h | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/ray/id.h b/src/ray/id.h index ffea499c9e24..0ab0c56408e7 100644 --- a/src/ray/id.h +++ b/src/ray/id.h @@ -7,16 +7,6 @@ #include #include "plasma/common.h" -// The "arrow/util/logging.h" will define some macros -// that conflict with libraries like Google Log. Undefine them below. -#undef DCHECK -#undef DCHECK_NE -#undef DCHECK_EQ -#undef DCHECK_GE -#undef DCHECK_GT -#undef DCHECK_LE -#undef DCHECK_LT - #include "ray/constants.h" #include "ray/util/visibility.h" From 370e0ec5a45a6593ce565ff95abe5e0131f02d12 Mon Sep 17 00:00:00 2001 From: suquark Date: Tue, 15 Jan 2019 00:15:10 +0800 Subject: [PATCH 3/6] revert changes to Python bindings because they will be removed in #3541 --- src/ray/raylet/lib/python/common_extension.cc | 8 ++++++++ src/ray/raylet/lib/python/raylet_extension.cc | 2 ++ 2 files changed, 10 insertions(+) diff --git a/src/ray/raylet/lib/python/common_extension.cc b/src/ray/raylet/lib/python/common_extension.cc index b37443dfdcb0..ccfcf5cb3b12 100644 --- a/src/ray/raylet/lib/python/common_extension.cc +++ b/src/ray/raylet/lib/python/common_extension.cc @@ -58,6 +58,8 @@ void init_pickle_module(void) { RAY_CHECK(pickle_protocol != NULL); } +TaskBuilder *g_task_builder = NULL; + /* Define the PyObjectID class. */ int PyStringToUniqueID(PyObject *object, ObjectID *object_id) { @@ -133,6 +135,12 @@ PyObject *PyObjectID_make(ObjectID object_id) { return (PyObject *)result; } +TaskSpec *TaskSpec_copy(TaskSpec *spec, int64_t task_spec_size) { + TaskSpec *copy = (TaskSpec *)malloc(task_spec_size); + memcpy(copy, spec, task_spec_size); + return copy; +} + /** * Convert a string to a Ray task specification Python object. * diff --git a/src/ray/raylet/lib/python/raylet_extension.cc b/src/ray/raylet/lib/python/raylet_extension.cc index 6105f2826f56..755ddf1ddd1c 100644 --- a/src/ray/raylet/lib/python/raylet_extension.cc +++ b/src/ray/raylet/lib/python/raylet_extension.cc @@ -5,6 +5,8 @@ #include "config_extension.h" #include "ray/raylet/raylet_client.h" +PyObject *LocalSchedulerError; + // clang-format off typedef struct { PyObject_HEAD From 458ccd63178847bb06107756a9f440aa1c8417ad Mon Sep 17 00:00:00 2001 From: suquark Date: Wed, 16 Jan 2019 23:37:35 +0800 Subject: [PATCH 4/6] remove dependencies of Arrow logging --- src/ray/object_manager/object_buffer_pool.cc | 23 ++++++++++--------- .../object_store_notification_manager.cc | 8 +++---- .../test/object_manager_stress_test.cc | 14 +++++------ .../test/object_manager_test.cc | 10 ++++---- src/ray/raylet/node_manager.cc | 8 +++---- .../raylet/object_manager_integration_test.cc | 10 ++++---- src/ray/status.h | 10 ++++++++ 7 files changed, 47 insertions(+), 36 deletions(-) diff --git a/src/ray/object_manager/object_buffer_pool.cc b/src/ray/object_manager/object_buffer_pool.cc index a650928e3a7e..8f9b8a461e64 100644 --- a/src/ray/object_manager/object_buffer_pool.cc +++ b/src/ray/object_manager/object_buffer_pool.cc @@ -1,6 +1,7 @@ #include "ray/object_manager/object_buffer_pool.h" -#include "arrow/util/logging.h" +#include "ray/status.h" +#include "ray/util/logging.h" namespace ray { @@ -8,7 +9,7 @@ ObjectBufferPool::ObjectBufferPool(const std::string &store_socket_name, uint64_t chunk_size) : default_chunk_size_(chunk_size) { store_socket_name_ = store_socket_name; - ARROW_CHECK_OK(store_client_.Connect(store_socket_name_.c_str())); + RAY_ARROW_CHECK_OK(store_client_.Connect(store_socket_name_.c_str())); } ObjectBufferPool::~ObjectBufferPool() { @@ -23,7 +24,7 @@ ObjectBufferPool::~ObjectBufferPool() { } RAY_CHECK(get_buffer_state_.empty()); RAY_CHECK(create_buffer_state_.empty()); - ARROW_CHECK_OK(store_client_.Disconnect()); + RAY_ARROW_CHECK_OK(store_client_.Disconnect()); } uint64_t ObjectBufferPool::GetNumChunks(uint64_t data_size) { @@ -44,7 +45,7 @@ std::pair ObjectBufferPool::Ge if (get_buffer_state_.count(object_id) == 0) { plasma::ObjectBuffer object_buffer; plasma::ObjectID plasma_id = object_id.to_plasma_id(); - ARROW_CHECK_OK(store_client_.Get(&plasma_id, 1, 0, &object_buffer)); + RAY_ARROW_CHECK_OK(store_client_.Get(&plasma_id, 1, 0, &object_buffer)); if (object_buffer.data == nullptr) { RAY_LOG(ERROR) << "Failed to get object"; return std::pair( @@ -73,14 +74,14 @@ void ObjectBufferPool::ReleaseGetChunk(const ObjectID &object_id, uint64_t chunk buffer_state.references--; RAY_LOG(DEBUG) << "ReleaseBuffer " << object_id << " " << buffer_state.references; if (buffer_state.references == 0) { - ARROW_CHECK_OK(store_client_.Release(object_id.to_plasma_id())); + RAY_ARROW_CHECK_OK(store_client_.Release(object_id.to_plasma_id())); get_buffer_state_.erase(object_id); } } void ObjectBufferPool::AbortGet(const ObjectID &object_id) { std::lock_guard lock(pool_mutex_); - ARROW_CHECK_OK(store_client_.Release(object_id.to_plasma_id())); + RAY_ARROW_CHECK_OK(store_client_.Release(object_id.to_plasma_id())); get_buffer_state_.erase(object_id); } @@ -156,16 +157,16 @@ void ObjectBufferPool::SealChunk(const ObjectID &object_id, const uint64_t chunk << create_buffer_state_[object_id].num_seals_remaining; if (create_buffer_state_[object_id].num_seals_remaining == 0) { const plasma::ObjectID plasma_id = object_id.to_plasma_id(); - ARROW_CHECK_OK(store_client_.Seal(plasma_id)); - ARROW_CHECK_OK(store_client_.Release(plasma_id)); + RAY_ARROW_CHECK_OK(store_client_.Seal(plasma_id)); + RAY_ARROW_CHECK_OK(store_client_.Release(plasma_id)); create_buffer_state_.erase(object_id); } } void ObjectBufferPool::AbortCreate(const ObjectID &object_id) { const plasma::ObjectID plasma_id = object_id.to_plasma_id(); - ARROW_CHECK_OK(store_client_.Release(plasma_id)); - ARROW_CHECK_OK(store_client_.Abort(plasma_id)); + RAY_ARROW_CHECK_OK(store_client_.Release(plasma_id)); + RAY_ARROW_CHECK_OK(store_client_.Abort(plasma_id)); create_buffer_state_.erase(object_id); } @@ -194,7 +195,7 @@ void ObjectBufferPool::FreeObjects(const std::vector &object_ids) { plasma_ids.push_back(id.to_plasma_id()); } std::lock_guard lock(pool_mutex_); - ARROW_CHECK_OK(store_client_.Delete(plasma_ids)); + RAY_ARROW_CHECK_OK(store_client_.Delete(plasma_ids)); } std::string ObjectBufferPool::DebugString() const { diff --git a/src/ray/object_manager/object_store_notification_manager.cc b/src/ray/object_manager/object_store_notification_manager.cc index 7c327ebbdbad..efa0c211d51d 100644 --- a/src/ray/object_manager/object_store_notification_manager.cc +++ b/src/ray/object_manager/object_store_notification_manager.cc @@ -5,7 +5,7 @@ #include #include -#include "arrow/util/logging.h" +#include "ray/status.h" #include "ray/common/common_protocol.h" #include "ray/object_manager/object_store_notification_manager.h" @@ -20,9 +20,9 @@ ObjectStoreNotificationManager::ObjectStoreNotificationManager( num_adds_processed_(0), num_removes_processed_(0), socket_(io_service) { - ARROW_CHECK_OK(store_client_.Connect(store_socket_name.c_str())); + RAY_ARROW_CHECK_OK(store_client_.Connect(store_socket_name.c_str())); - ARROW_CHECK_OK(store_client_.Subscribe(&c_socket_)); + RAY_ARROW_CHECK_OK(store_client_.Subscribe(&c_socket_)); boost::system::error_code ec; socket_.assign(boost::asio::local::stream_protocol(), c_socket_, ec); assert(!ec.value()); @@ -30,7 +30,7 @@ ObjectStoreNotificationManager::ObjectStoreNotificationManager( } ObjectStoreNotificationManager::~ObjectStoreNotificationManager() { - ARROW_CHECK_OK(store_client_.Disconnect()); + RAY_ARROW_CHECK_OK(store_client_.Disconnect()); } void ObjectStoreNotificationManager::NotificationWait() { diff --git a/src/ray/object_manager/test/object_manager_stress_test.cc b/src/ray/object_manager/test/object_manager_stress_test.cc index a1f792727685..8b5d71572ea2 100644 --- a/src/ray/object_manager/test/object_manager_stress_test.cc +++ b/src/ray/object_manager/test/object_manager_stress_test.cc @@ -5,7 +5,7 @@ #include "gtest/gtest.h" -#include "arrow/util/logging.h" +#include "ray/status.h" #include "ray/object_manager/object_manager.h" @@ -157,8 +157,8 @@ class TestObjectManagerBase : public ::testing::Test { server2.reset(new MockServer(main_service, om_config_2, gcs_client_2)); // connect to stores. - ARROW_CHECK_OK(client1.Connect(store_id_1)); - ARROW_CHECK_OK(client2.Connect(store_id_2)); + RAY_ARROW_CHECK_OK(client1.Connect(store_id_1)); + RAY_ARROW_CHECK_OK(client2.Connect(store_id_2)); } void TearDown() { @@ -179,9 +179,9 @@ class TestObjectManagerBase : public ::testing::Test { uint8_t metadata[] = {5}; int64_t metadata_size = sizeof(metadata); std::shared_ptr data; - ARROW_CHECK_OK(client.Create(object_id.to_plasma_id(), data_size, metadata, + RAY_ARROW_CHECK_OK(client.Create(object_id.to_plasma_id(), data_size, metadata, metadata_size, &data)); - ARROW_CHECK_OK(client.Seal(object_id.to_plasma_id())); + RAY_ARROW_CHECK_OK(client.Seal(object_id.to_plasma_id())); return object_id; } @@ -291,14 +291,14 @@ class StressTestObjectManager : public TestObjectManagerBase { plasma::ObjectBuffer GetObject(plasma::PlasmaClient &client, ObjectID &object_id) { plasma::ObjectBuffer object_buffer; plasma::ObjectID plasma_id = object_id.to_plasma_id(); - ARROW_CHECK_OK(client.Get(&plasma_id, 1, 0, &object_buffer)); + RAY_ARROW_CHECK_OK(client.Get(&plasma_id, 1, 0, &object_buffer)); return object_buffer; } static unsigned char *GetDigest(plasma::PlasmaClient &client, ObjectID &object_id) { const int64_t size = sizeof(uint64_t); static unsigned char digest_1[size]; - ARROW_CHECK_OK(client.Hash(object_id.to_plasma_id(), &digest_1[0])); + RAY_ARROW_CHECK_OK(client.Hash(object_id.to_plasma_id(), &digest_1[0])); return digest_1; } diff --git a/src/ray/object_manager/test/object_manager_test.cc b/src/ray/object_manager/test/object_manager_test.cc index a956c1a0aa3e..e47c218156c7 100644 --- a/src/ray/object_manager/test/object_manager_test.cc +++ b/src/ray/object_manager/test/object_manager_test.cc @@ -3,7 +3,7 @@ #include "gtest/gtest.h" -#include "arrow/util/logging.h" +#include "ray/status.h" #include "ray/object_manager/object_manager.h" @@ -142,8 +142,8 @@ class TestObjectManagerBase : public ::testing::Test { server2.reset(new MockServer(main_service, om_config_2, gcs_client_2)); // connect to stores. - ARROW_CHECK_OK(client1.Connect(store_id_1)); - ARROW_CHECK_OK(client2.Connect(store_id_2)); + RAY_ARROW_CHECK_OK(client1.Connect(store_id_1)); + RAY_ARROW_CHECK_OK(client2.Connect(store_id_2)); } void TearDown() { @@ -168,9 +168,9 @@ class TestObjectManagerBase : public ::testing::Test { uint8_t metadata[] = {5}; int64_t metadata_size = sizeof(metadata); std::shared_ptr data; - ARROW_CHECK_OK(client.Create(object_id.to_plasma_id(), data_size, metadata, + RAY_ARROW_CHECK_OK(client.Create(object_id.to_plasma_id(), data_size, metadata, metadata_size, &data)); - ARROW_CHECK_OK(client.Seal(object_id.to_plasma_id())); + RAY_ARROW_CHECK_OK(client.Seal(object_id.to_plasma_id())); return object_id; } diff --git a/src/ray/raylet/node_manager.cc b/src/ray/raylet/node_manager.cc index afee3369a4c3..e20173cb8e6a 100644 --- a/src/ray/raylet/node_manager.cc +++ b/src/ray/raylet/node_manager.cc @@ -2,7 +2,7 @@ #include -#include "arrow/util/logging.h" +#include "ray/status.h" #include "ray/common/common_protocol.h" #include "ray/id.h" @@ -90,7 +90,7 @@ NodeManager::NodeManager(boost::asio::io_service &io_service, RAY_CHECK_OK(object_manager_.SubscribeObjDeleted( [this](const ObjectID &object_id) { HandleObjectMissing(object_id); })); - ARROW_CHECK_OK(store_client_.Connect(config.store_socket_name.c_str())); + RAY_ARROW_CHECK_OK(store_client_.Connect(config.store_socket_name.c_str())); } ray::Status NodeManager::RegisterGcs() { @@ -1148,8 +1148,8 @@ void NodeManager::TreatTaskAsFailed(const Task &task) { if (!status.IsPlasmaObjectExists()) { // TODO(rkn): We probably don't want this checks. E.g., if the object // store is full, we don't want to kill the raylet. - ARROW_CHECK_OK(status); - ARROW_CHECK_OK(store_client_.Seal(object_id.to_plasma_id())); + RAY_ARROW_CHECK_OK(status); + RAY_ARROW_CHECK_OK(store_client_.Seal(object_id.to_plasma_id())); } } // A task failing is equivalent to assigning and finishing the task, so clean diff --git a/src/ray/raylet/object_manager_integration_test.cc b/src/ray/raylet/object_manager_integration_test.cc index e1570dc9be18..a6ac6f3f399d 100644 --- a/src/ray/raylet/object_manager_integration_test.cc +++ b/src/ray/raylet/object_manager_integration_test.cc @@ -3,7 +3,7 @@ #include "gtest/gtest.h" -#include "arrow/util/logging.h" +#include "ray/status.h" #include "ray/raylet/raylet.h" @@ -76,8 +76,8 @@ class TestObjectManagerBase : public ::testing::Test { GetNodeManagerConfig("raylet_2", store_sock_2), om_config_2, gcs_client_2)); // connect to stores. - ARROW_CHECK_OK(client1.Connect(store_sock_1)); - ARROW_CHECK_OK(client2.Connect(store_sock_2)); + RAY_ARROW_CHECK_OK(client1.Connect(store_sock_1)); + RAY_ARROW_CHECK_OK(client2.Connect(store_sock_2)); } void TearDown() { @@ -104,9 +104,9 @@ class TestObjectManagerBase : public ::testing::Test { uint8_t metadata[] = {5}; int64_t metadata_size = sizeof(metadata); std::shared_ptr data; - ARROW_CHECK_OK(client.Create(object_id.to_plasma_id(), data_size, metadata, + RAY_ARROW_CHECK_OK(client.Create(object_id.to_plasma_id(), data_size, metadata, metadata_size, &data)); - ARROW_CHECK_OK(client.Seal(object_id.to_plasma_id())); + RAY_ARROW_CHECK_OK(client.Seal(object_id.to_plasma_id())); return object_id; } diff --git a/src/ray/status.h b/src/ray/status.h index 47629f32ad87..143ffc134d68 100644 --- a/src/ray/status.h +++ b/src/ray/status.h @@ -44,6 +44,16 @@ // logged message. #define RAY_CHECK_OK(s) RAY_CHECK_OK_PREPEND(s, "Bad status") +// This macro is used to replace the "ARROW_CHECK_OK_PREPEND" macro. +#define RAY_ARROW_CHECK_OK_PREPEND(to_call, msg) \ + do { \ + ::arrow::Status _s = (to_call); \ + RAY_CHECK(_s.ok()) << (msg) << ": " << _s.ToString(); \ + } while (false) + +// This macro is used to replace the "ARROW_CHECK_OK" macro. +#define RAY_ARROW_CHECK_OK(s) RAY_ARROW_CHECK_OK_PREPEND(s, "Bad status") + namespace ray { enum class StatusCode : char { From 6fb75b0c343d53441e772df3e0596e9f60d0ad37 Mon Sep 17 00:00:00 2001 From: suquark Date: Wed, 16 Jan 2019 23:39:04 +0800 Subject: [PATCH 5/6] revert changes to Arrow logging --- src/ray/util/logging.cc | 84 +++++++++++++++++++++++++++++++++++++---- src/ray/util/logging.h | 60 +++-------------------------- 2 files changed, 82 insertions(+), 62 deletions(-) diff --git a/src/ray/util/logging.cc b/src/ray/util/logging.cc index 6c56b1931c36..97c871d8cb7f 100644 --- a/src/ray/util/logging.cc +++ b/src/ray/util/logging.cc @@ -1,10 +1,70 @@ #include "ray/util/logging.h" + +#ifndef _WIN32 +#include +#endif + #include +#include #include #include +#include + +#ifdef RAY_USE_GLOG +#include "glog/logging.h" +#endif namespace ray { +// This is the default implementation of ray log, +// which is independent of any libs. +class CerrLog { + public: + CerrLog(RayLogLevel severity) : severity_(severity), has_logged_(false) {} + + virtual ~CerrLog() { + if (has_logged_) { + std::cerr << std::endl; + } + if (severity_ == RayLogLevel::FATAL) { + PrintBackTrace(); + std::abort(); + } + } + + std::ostream &Stream() { + has_logged_ = true; + return std::cerr; + } + + template + CerrLog &operator<<(const T &t) { + if (severity_ != RayLogLevel::DEBUG) { + has_logged_ = true; + std::cerr << t; + } + return *this; + } + + protected: + const RayLogLevel severity_; + bool has_logged_; + + void PrintBackTrace() { +#if defined(_EXECINFO_H) || !defined(_WIN32) + void *buffer[255]; + const int calls = backtrace(buffer, sizeof(buffer) / sizeof(void *)); + backtrace_symbols_fd(buffer, calls, 1); +#endif + } +}; + +#ifdef RAY_USE_GLOG +typedef google::LogMessage LoggingProvider; +#else +typedef ray::CerrLog LoggingProvider; +#endif + RayLogLevel RayLog::severity_threshold_ = RayLogLevel::INFO; std::string RayLog::app_name_ = ""; std::string RayLog::log_dir_ = ""; @@ -126,28 +186,38 @@ bool RayLog::IsLevelEnabled(RayLogLevel log_level) { RayLog::RayLog(const char *file_name, int line_number, RayLogLevel severity) // glog does not have DEBUG level, we can handle it using is_enabled_. - : is_enabled_(severity >= severity_threshold_) { + : logging_provider_(nullptr), + is_enabled_(severity >= severity_threshold_) { #ifdef RAY_USE_GLOG if (is_enabled_) { - logging_provider_.reset( - new google::LogMessage(file_name, line_number, GetMappedSeverity(severity))); + logging_provider_ = + new google::LogMessage(file_name, line_number, GetMappedSeverity(severity)); } #else - logging_provider_.reset(new CerrLog(severity)); - *logging_provider_ << file_name << ":" << line_number << ": "; + auto logging_provider = new CerrLog(severity); + *logging_provider << file_name << ":" << line_number << ": "; + logging_provider_ = logging_provider; #endif } std::ostream &RayLog::Stream() { + auto logging_provider = reinterpret_cast(logging_provider_); #ifdef RAY_USE_GLOG // Before calling this function, user should check IsEnabled. // When IsEnabled == false, logging_provider_ will be empty. - return logging_provider_->stream(); + return logging_provider->stream(); #else - return logging_provider_->Stream(); + return logging_provider->Stream(); #endif } bool RayLog::IsEnabled() const { return is_enabled_; } +RayLog::~RayLog() { + if (logging_provider_ != nullptr) { + delete reinterpret_cast(logging_provider_); + logging_provider_ = nullptr; + } +} + } // namespace ray diff --git a/src/ray/util/logging.h b/src/ray/util/logging.h index 5f16a2846792..6f657142efd7 100644 --- a/src/ray/util/logging.h +++ b/src/ray/util/logging.h @@ -2,66 +2,12 @@ #define RAY_UTIL_LOGGING_H #include -#include #include -#ifndef _WIN32 -#include -#endif - -#ifdef RAY_USE_GLOG -#include "glog/logging.h" -typedef google::LogMessage LoggingProvider; -#else -typedef ray::CerrLog LoggingProvider; -#endif namespace ray { enum class RayLogLevel { DEBUG = -1, INFO = 0, WARNING = 1, ERROR = 2, FATAL = 3 }; -// This is the default implementation of ray log, -// which is independent of any libs. -class CerrLog { - public: - CerrLog(RayLogLevel severity) : severity_(severity), has_logged_(false) {} - - virtual ~CerrLog() { - if (has_logged_) { - std::cerr << std::endl; - } - if (severity_ == RayLogLevel::FATAL) { - PrintBackTrace(); - std::abort(); - } - } - - std::ostream &Stream() { - has_logged_ = true; - return std::cerr; - } - - template - CerrLog &operator<<(const T &t) { - if (severity_ != RayLogLevel::DEBUG) { - has_logged_ = true; - std::cerr << t; - } - return *this; - } - - protected: - const RayLogLevel severity_; - bool has_logged_; - - void PrintBackTrace() { -#if defined(_EXECINFO_H) || !defined(_WIN32) - void *buffer[255]; - const int calls = backtrace(buffer, sizeof(buffer) / sizeof(void *)); - backtrace_symbols_fd(buffer, calls, 1); -#endif - } -}; - #define RAY_LOG_INTERNAL(level) ::ray::RayLog(__FILE__, __LINE__, level) #define RAY_LOG(level) \ @@ -117,6 +63,8 @@ class RayLog : public RayLogBase { public: RayLog(const char *file_name, int line_number, RayLogLevel severity); + virtual ~RayLog(); + /// Return whether or not current logging instance is enabled. /// /// \return True if logging is enabled and false otherwise. @@ -150,7 +98,9 @@ class RayLog : public RayLogBase { static RayLogLevel GetLogLevelFromEnv(); private: - std::unique_ptr logging_provider_; + // Hide the implementation of log provider by void *. + // Otherwise, lib user may define the same macro to use the correct header file. + void *logging_provider_; /// True if log messages should be logged and false if they should be ignored. bool is_enabled_; static RayLogLevel severity_threshold_; From 910ac61b0bf5834dddd8e35168094ed08491ead8 Mon Sep 17 00:00:00 2001 From: suquark Date: Thu, 17 Jan 2019 12:04:10 +0800 Subject: [PATCH 6/6] lint --- src/ray/object_manager/test/object_manager_stress_test.cc | 2 +- src/ray/object_manager/test/object_manager_test.cc | 2 +- src/ray/raylet/object_manager_integration_test.cc | 2 +- src/ray/status.h | 8 ++++---- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/ray/object_manager/test/object_manager_stress_test.cc b/src/ray/object_manager/test/object_manager_stress_test.cc index 8b5d71572ea2..3f2d50ead52a 100644 --- a/src/ray/object_manager/test/object_manager_stress_test.cc +++ b/src/ray/object_manager/test/object_manager_stress_test.cc @@ -180,7 +180,7 @@ class TestObjectManagerBase : public ::testing::Test { int64_t metadata_size = sizeof(metadata); std::shared_ptr data; RAY_ARROW_CHECK_OK(client.Create(object_id.to_plasma_id(), data_size, metadata, - metadata_size, &data)); + metadata_size, &data)); RAY_ARROW_CHECK_OK(client.Seal(object_id.to_plasma_id())); return object_id; } diff --git a/src/ray/object_manager/test/object_manager_test.cc b/src/ray/object_manager/test/object_manager_test.cc index e47c218156c7..a71d7636ae2d 100644 --- a/src/ray/object_manager/test/object_manager_test.cc +++ b/src/ray/object_manager/test/object_manager_test.cc @@ -169,7 +169,7 @@ class TestObjectManagerBase : public ::testing::Test { int64_t metadata_size = sizeof(metadata); std::shared_ptr data; RAY_ARROW_CHECK_OK(client.Create(object_id.to_plasma_id(), data_size, metadata, - metadata_size, &data)); + metadata_size, &data)); RAY_ARROW_CHECK_OK(client.Seal(object_id.to_plasma_id())); return object_id; } diff --git a/src/ray/raylet/object_manager_integration_test.cc b/src/ray/raylet/object_manager_integration_test.cc index a6ac6f3f399d..861020595448 100644 --- a/src/ray/raylet/object_manager_integration_test.cc +++ b/src/ray/raylet/object_manager_integration_test.cc @@ -105,7 +105,7 @@ class TestObjectManagerBase : public ::testing::Test { int64_t metadata_size = sizeof(metadata); std::shared_ptr data; RAY_ARROW_CHECK_OK(client.Create(object_id.to_plasma_id(), data_size, metadata, - metadata_size, &data)); + metadata_size, &data)); RAY_ARROW_CHECK_OK(client.Seal(object_id.to_plasma_id())); return object_id; } diff --git a/src/ray/status.h b/src/ray/status.h index 143ffc134d68..fb6252b34667 100644 --- a/src/ray/status.h +++ b/src/ray/status.h @@ -45,11 +45,11 @@ #define RAY_CHECK_OK(s) RAY_CHECK_OK_PREPEND(s, "Bad status") // This macro is used to replace the "ARROW_CHECK_OK_PREPEND" macro. -#define RAY_ARROW_CHECK_OK_PREPEND(to_call, msg) \ - do { \ - ::arrow::Status _s = (to_call); \ +#define RAY_ARROW_CHECK_OK_PREPEND(to_call, msg) \ + do { \ + ::arrow::Status _s = (to_call); \ RAY_CHECK(_s.ok()) << (msg) << ": " << _s.ToString(); \ - } while (false) + } while (0) // This macro is used to replace the "ARROW_CHECK_OK" macro. #define RAY_ARROW_CHECK_OK(s) RAY_ARROW_CHECK_OK_PREPEND(s, "Bad status")