From 3fed9264a9aa1c40225610162ee7ff29d226bedf Mon Sep 17 00:00:00 2001 From: Yuhong Guo Date: Fri, 15 Feb 2019 19:37:34 +0800 Subject: [PATCH 01/12] Add plasma log --- cpp/src/plasma/store.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cpp/src/plasma/store.cc b/cpp/src/plasma/store.cc index 82f648cc6ad..94e74906246 100644 --- a/cpp/src/plasma/store.cc +++ b/cpp/src/plasma/store.cc @@ -232,6 +232,10 @@ PlasmaError PlasmaStore::CreateObject(const ObjectID& object_id, int64_t data_si } else { pointer = AllocateMemory(total_size, &fd, &map_size, &offset); if (!pointer) { + ARROW_LOG(ERROR) << "Not enough memory to creat the object " << object_id.hex() + << ", data_size=" << data_size << ", metadata_size=" + << metadata_size + << ", will send a reply of PlasmaError::OutOfMemory"; return PlasmaError::OutOfMemory; } } @@ -1036,6 +1040,7 @@ static std::unique_ptr g_runner = nullptr; void HandleSignal(int signal) { if (signal == SIGTERM) { + ARROW_LOG(INFO) << "SIGTERM Signal received, close Plasma Server..."; if (g_runner != nullptr) { g_runner->Stop(); } From ed56a4808526b8ff9a7aea95bd5b0067df95fcbb Mon Sep 17 00:00:00 2001 From: Yuhong Guo Date: Fri, 15 Feb 2019 19:40:13 +0800 Subject: [PATCH 02/12] Fix possible unclosed EventLoop reported by code scanning tool --- cpp/src/plasma/events.cc | 11 ++++++++++- cpp/src/plasma/events.h | 2 ++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/cpp/src/plasma/events.cc b/cpp/src/plasma/events.cc index 7c977b00069..69896c02906 100644 --- a/cpp/src/plasma/events.cc +++ b/cpp/src/plasma/events.cc @@ -80,7 +80,16 @@ void EventLoop::Start() { aeMain(loop_); } void EventLoop::Stop() { aeStop(loop_); } -void EventLoop::Shutdown() { aeDeleteEventLoop(loop_); } +void EventLoop::Shutdown() { + if (loop_ != nullptr) { + aeDeleteEventLoop(loop_); + loop_ = nullptr; + } +} + +EventLoop::~EventLoop() { + Shutdown(); +} int64_t EventLoop::AddTimer(int64_t timeout, const TimerCallback& callback) { auto data = std::unique_ptr(new TimerCallback(callback)); diff --git a/cpp/src/plasma/events.h b/cpp/src/plasma/events.h index 109540e6d1a..765be9c01fb 100644 --- a/cpp/src/plasma/events.h +++ b/cpp/src/plasma/events.h @@ -59,6 +59,8 @@ class EventLoop { EventLoop(); + ~EventLoop(); + /// Add a new file event handler to the event loop. /// /// \param fd The file descriptor we are listening to. From 3a917d41d0bbcca1d0ecbcee52a608b99dce48da Mon Sep 17 00:00:00 2001 From: Yuhong Guo Date: Fri, 15 Feb 2019 19:41:01 +0800 Subject: [PATCH 03/12] Fix not used return value in deserialize.cc reported by code scan tool --- cpp/src/arrow/python/deserialize.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cpp/src/arrow/python/deserialize.cc b/cpp/src/arrow/python/deserialize.cc index f13070a5883..23a47485a00 100644 --- a/cpp/src/arrow/python/deserialize.cc +++ b/cpp/src/arrow/python/deserialize.cc @@ -84,8 +84,11 @@ Status DeserializeDict(PyObject* context, const Array& array, int64_t start_idx, // The latter two steal references whereas PyDict_SetItem does not. So we need // to make sure the reference count is decremented by letting the OwnedRef // go out of scope at the end. - PyDict_SetItem(result.obj(), PyList_GET_ITEM(keys.obj(), i - start_idx), + int ret = PyDict_SetItem(result.obj(), PyList_GET_ITEM(keys.obj(), i - start_idx), PyList_GET_ITEM(vals.obj(), i - start_idx)); + if (ret == 0) { + return Status(StatusCode::PythonError, "PyDict_SetItem failed."); + } } static PyObject* py_type = PyUnicode_FromString("_pytype_"); if (PyDict_Contains(result.obj(), py_type)) { From 5be7b89ff41f7a676e827e01db93a42afb8679db Mon Sep 17 00:00:00 2001 From: Yuhong Guo Date: Fri, 15 Feb 2019 19:41:38 +0800 Subject: [PATCH 04/12] Fix unclosed fd reported by code scan tool --- cpp/src/plasma/io.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/src/plasma/io.cc b/cpp/src/plasma/io.cc index cc425428ece..9ba23e552c2 100644 --- a/cpp/src/plasma/io.cc +++ b/cpp/src/plasma/io.cc @@ -195,6 +195,7 @@ int ConnectIpcSock(const std::string& pathname) { socket_address.sun_family = AF_UNIX; if (pathname.size() + 1 > sizeof(socket_address.sun_path)) { ARROW_LOG(ERROR) << "Socket pathname is too long."; + close(socket_fd); return -1; } strncpy(socket_address.sun_path, pathname.c_str(), pathname.size() + 1); From a667402589cfaf8fc332a56a7671f062dbc5d891 Mon Sep 17 00:00:00 2001 From: Yuhong Guo Date: Fri, 15 Feb 2019 19:42:25 +0800 Subject: [PATCH 05/12] Do not evaluate logging strings when logging is not enabled. --- cpp/src/arrow/util/logging.cc | 4 ++++ cpp/src/arrow/util/logging.h | 11 ++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/cpp/src/arrow/util/logging.cc b/cpp/src/arrow/util/logging.cc index 7e368409c02..2d7cc605f8b 100644 --- a/cpp/src/arrow/util/logging.cc +++ b/cpp/src/arrow/util/logging.cc @@ -152,6 +152,10 @@ void ArrowLog::InstallFailureSignalHandler() { #endif } +bool ArrowLog::IsLevelEnabled(ArrowLogLevel log_level) { + return log_level >= severity_threshold_; +} + ArrowLog::ArrowLog(const char* file_name, int line_number, ArrowLogLevel severity) // glog does not have DEBUG level, we can handle it using is_enabled_. : logging_provider_(nullptr), is_enabled_(severity >= severity_threshold_) { diff --git a/cpp/src/arrow/util/logging.h b/cpp/src/arrow/util/logging.h index 5ea78206a73..10ef15855d1 100644 --- a/cpp/src/arrow/util/logging.h +++ b/cpp/src/arrow/util/logging.h @@ -55,7 +55,10 @@ enum class ArrowLogLevel : int { }; #define ARROW_LOG_INTERNAL(level) ::arrow::util::ArrowLog(__FILE__, __LINE__, level) -#define ARROW_LOG(level) ARROW_LOG_INTERNAL(::arrow::util::ArrowLogLevel::ARROW_##level) +#define ARROW_LOG(level) \ + if (arrow::util::ArrowLog::IsLevelEnabled(arrow::util::ArrowLogLevel::ARROW_##level)) \ + ARROW_LOG_INTERNAL(arrow::util::ArrowLogLevel::ARROW_##level) + #define ARROW_IGNORE_EXPR(expr) ((void)(expr)) #define ARROW_CHECK(condition) \ @@ -173,6 +176,12 @@ class ARROW_EXPORT ArrowLog : public ArrowLogBase { /// If glog is not installed, this function won't do anything. static void InstallFailureSignalHandler(); + /// Return whether or not the log level is enabled in current setting. + /// + /// \param log_level The input log level to test. + /// \return True if input log level is not lower than the threshold. + static bool IsLevelEnabled(ArrowLogLevel log_level); + private: ARROW_DISALLOW_COPY_AND_ASSIGN(ArrowLog); From b2ddba6ce42a37406d66f04b672c388e940267df Mon Sep 17 00:00:00 2001 From: Yuhong Guo Date: Fri, 15 Feb 2019 19:43:06 +0800 Subject: [PATCH 06/12] Fix misplace of meta and data in PlasmaClient.java --- .../src/main/java/org/apache/arrow/plasma/PlasmaClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/plasma/src/main/java/org/apache/arrow/plasma/PlasmaClient.java b/java/plasma/src/main/java/org/apache/arrow/plasma/PlasmaClient.java index a708f41853d..af53c6190ee 100644 --- a/java/plasma/src/main/java/org/apache/arrow/plasma/PlasmaClient.java +++ b/java/plasma/src/main/java/org/apache/arrow/plasma/PlasmaClient.java @@ -99,7 +99,7 @@ public List get(byte[][] objectIds, int timeoutMs) { } else { meta = null; } - ret.add(new ObjectStoreData(data, meta)); + ret.add(new ObjectStoreData(meta, data)); } } return ret; From 434a039d218a0c377dd2789453ad2d1cfe4afabb Mon Sep 17 00:00:00 2001 From: Yuhong Guo Date: Fri, 15 Feb 2019 19:43:25 +0800 Subject: [PATCH 07/12] Make constructor of ObjectStoreData public --- .../src/main/java/org/apache/arrow/plasma/ObjectStoreLink.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/plasma/src/main/java/org/apache/arrow/plasma/ObjectStoreLink.java b/java/plasma/src/main/java/org/apache/arrow/plasma/ObjectStoreLink.java index f933c85b836..520549b8d33 100644 --- a/java/plasma/src/main/java/org/apache/arrow/plasma/ObjectStoreLink.java +++ b/java/plasma/src/main/java/org/apache/arrow/plasma/ObjectStoreLink.java @@ -29,7 +29,7 @@ public interface ObjectStoreLink { class ObjectStoreData { - ObjectStoreData(byte[] metadata, byte[] data) { + public ObjectStoreData(byte[] metadata, byte[] data) { this.data = data; this.metadata = metadata; } From 79b4af3ae36989caf0e0505c073b8cc6e6f95b2a Mon Sep 17 00:00:00 2001 From: Yuhong Guo Date: Fri, 15 Feb 2019 23:23:03 +0800 Subject: [PATCH 08/12] Fix and Lint --- cpp/src/arrow/python/deserialize.cc | 2 +- cpp/src/arrow/util/logging.h | 4 ++-- cpp/src/plasma/store.cc | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cpp/src/arrow/python/deserialize.cc b/cpp/src/arrow/python/deserialize.cc index 23a47485a00..3220d6227eb 100644 --- a/cpp/src/arrow/python/deserialize.cc +++ b/cpp/src/arrow/python/deserialize.cc @@ -86,7 +86,7 @@ Status DeserializeDict(PyObject* context, const Array& array, int64_t start_idx, // go out of scope at the end. int ret = PyDict_SetItem(result.obj(), PyList_GET_ITEM(keys.obj(), i - start_idx), PyList_GET_ITEM(vals.obj(), i - start_idx)); - if (ret == 0) { + if (ret != 0) { return Status(StatusCode::PythonError, "PyDict_SetItem failed."); } } diff --git a/cpp/src/arrow/util/logging.h b/cpp/src/arrow/util/logging.h index 10ef15855d1..079e8369942 100644 --- a/cpp/src/arrow/util/logging.h +++ b/cpp/src/arrow/util/logging.h @@ -55,8 +55,8 @@ enum class ArrowLogLevel : int { }; #define ARROW_LOG_INTERNAL(level) ::arrow::util::ArrowLog(__FILE__, __LINE__, level) -#define ARROW_LOG(level) \ - if (arrow::util::ArrowLog::IsLevelEnabled(arrow::util::ArrowLogLevel::ARROW_##level)) \ +#define ARROW_LOG(level) if ( \ + arrow::util::ArrowLog::IsLevelEnabled(arrow::util::ArrowLogLevel::ARROW_##level)) \ ARROW_LOG_INTERNAL(arrow::util::ArrowLogLevel::ARROW_##level) #define ARROW_IGNORE_EXPR(expr) ((void)(expr)) diff --git a/cpp/src/plasma/store.cc b/cpp/src/plasma/store.cc index 94e74906246..610aa56da98 100644 --- a/cpp/src/plasma/store.cc +++ b/cpp/src/plasma/store.cc @@ -233,9 +233,9 @@ PlasmaError PlasmaStore::CreateObject(const ObjectID& object_id, int64_t data_si pointer = AllocateMemory(total_size, &fd, &map_size, &offset); if (!pointer) { ARROW_LOG(ERROR) << "Not enough memory to creat the object " << object_id.hex() - << ", data_size=" << data_size << ", metadata_size=" - << metadata_size - << ", will send a reply of PlasmaError::OutOfMemory"; + << ", data_size=" << data_size << ", metadata_size=" + << metadata_size + << ", will send a reply of PlasmaError::OutOfMemory"; return PlasmaError::OutOfMemory; } } From d3eb22ff8790a50d29d8e74cafb1b1c358afb59b Mon Sep 17 00:00:00 2001 From: Yuhong Guo Date: Sat, 16 Feb 2019 00:02:45 +0800 Subject: [PATCH 09/12] Lint --- cpp/src/arrow/python/deserialize.cc | 2 +- cpp/src/arrow/util/logging.h | 4 +++- cpp/src/plasma/events.cc | 4 +--- cpp/src/plasma/store.cc | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cpp/src/arrow/python/deserialize.cc b/cpp/src/arrow/python/deserialize.cc index 3220d6227eb..de84e31c8eb 100644 --- a/cpp/src/arrow/python/deserialize.cc +++ b/cpp/src/arrow/python/deserialize.cc @@ -85,7 +85,7 @@ Status DeserializeDict(PyObject* context, const Array& array, int64_t start_idx, // to make sure the reference count is decremented by letting the OwnedRef // go out of scope at the end. int ret = PyDict_SetItem(result.obj(), PyList_GET_ITEM(keys.obj(), i - start_idx), - PyList_GET_ITEM(vals.obj(), i - start_idx)); + PyList_GET_ITEM(vals.obj(), i - start_idx)); if (ret != 0) { return Status(StatusCode::PythonError, "PyDict_SetItem failed."); } diff --git a/cpp/src/arrow/util/logging.h b/cpp/src/arrow/util/logging.h index 079e8369942..f186a9621ee 100644 --- a/cpp/src/arrow/util/logging.h +++ b/cpp/src/arrow/util/logging.h @@ -55,9 +55,11 @@ enum class ArrowLogLevel : int { }; #define ARROW_LOG_INTERNAL(level) ::arrow::util::ArrowLog(__FILE__, __LINE__, level) +// clang-format off #define ARROW_LOG(level) if ( \ arrow::util::ArrowLog::IsLevelEnabled(arrow::util::ArrowLogLevel::ARROW_##level)) \ - ARROW_LOG_INTERNAL(arrow::util::ArrowLogLevel::ARROW_##level) + ARROW_LOG_INTERNAL(arrow::util::ArrowLogLevel::ARROW_##level) +// clang-format on #define ARROW_IGNORE_EXPR(expr) ((void)(expr)) diff --git a/cpp/src/plasma/events.cc b/cpp/src/plasma/events.cc index 69896c02906..d49c577fdf1 100644 --- a/cpp/src/plasma/events.cc +++ b/cpp/src/plasma/events.cc @@ -87,9 +87,7 @@ void EventLoop::Shutdown() { } } -EventLoop::~EventLoop() { - Shutdown(); -} +EventLoop::~EventLoop() { Shutdown(); } int64_t EventLoop::AddTimer(int64_t timeout, const TimerCallback& callback) { auto data = std::unique_ptr(new TimerCallback(callback)); diff --git a/cpp/src/plasma/store.cc b/cpp/src/plasma/store.cc index 610aa56da98..23d29d45b8a 100644 --- a/cpp/src/plasma/store.cc +++ b/cpp/src/plasma/store.cc @@ -233,8 +233,8 @@ PlasmaError PlasmaStore::CreateObject(const ObjectID& object_id, int64_t data_si pointer = AllocateMemory(total_size, &fd, &map_size, &offset); if (!pointer) { ARROW_LOG(ERROR) << "Not enough memory to creat the object " << object_id.hex() - << ", data_size=" << data_size << ", metadata_size=" - << metadata_size + << ", data_size=" << data_size + << ", metadata_size=" << metadata_size << ", will send a reply of PlasmaError::OutOfMemory"; return PlasmaError::OutOfMemory; } From 440f0975fee7a9aeb6a6c0d6e3ef4c5bd7460070 Mon Sep 17 00:00:00 2001 From: Yuhong Guo Date: Mon, 18 Feb 2019 22:55:04 +0800 Subject: [PATCH 10/12] Address comment --- cpp/src/arrow/python/deserialize.cc | 2 +- cpp/src/plasma/store.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/src/arrow/python/deserialize.cc b/cpp/src/arrow/python/deserialize.cc index de84e31c8eb..95b7b7a4ec9 100644 --- a/cpp/src/arrow/python/deserialize.cc +++ b/cpp/src/arrow/python/deserialize.cc @@ -87,7 +87,7 @@ Status DeserializeDict(PyObject* context, const Array& array, int64_t start_idx, int ret = PyDict_SetItem(result.obj(), PyList_GET_ITEM(keys.obj(), i - start_idx), PyList_GET_ITEM(vals.obj(), i - start_idx)); if (ret != 0) { - return Status(StatusCode::PythonError, "PyDict_SetItem failed."); + return ConvertPyError(StatusCode::PythonError); } } static PyObject* py_type = PyUnicode_FromString("_pytype_"); diff --git a/cpp/src/plasma/store.cc b/cpp/src/plasma/store.cc index 23d29d45b8a..c55da30573f 100644 --- a/cpp/src/plasma/store.cc +++ b/cpp/src/plasma/store.cc @@ -232,7 +232,7 @@ PlasmaError PlasmaStore::CreateObject(const ObjectID& object_id, int64_t data_si } else { pointer = AllocateMemory(total_size, &fd, &map_size, &offset); if (!pointer) { - ARROW_LOG(ERROR) << "Not enough memory to creat the object " << object_id.hex() + ARROW_LOG(ERROR) << "Not enough memory to create the object " << object_id.hex() << ", data_size=" << data_size << ", metadata_size=" << metadata_size << ", will send a reply of PlasmaError::OutOfMemory"; @@ -1040,7 +1040,7 @@ static std::unique_ptr g_runner = nullptr; void HandleSignal(int signal) { if (signal == SIGTERM) { - ARROW_LOG(INFO) << "SIGTERM Signal received, close Plasma Server..."; + ARROW_LOG(INFO) << "SIGTERM Signal received, closing Plasma Server..."; if (g_runner != nullptr) { g_runner->Stop(); } From b547f2f5d323762c082361c6336e7323711d20d1 Mon Sep 17 00:00:00 2001 From: Yuhong Guo Date: Tue, 19 Feb 2019 00:14:08 +0800 Subject: [PATCH 11/12] remove if from ARROW_LOG --- cpp/src/arrow/util/logging.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/cpp/src/arrow/util/logging.h b/cpp/src/arrow/util/logging.h index f186a9621ee..18746c5885b 100644 --- a/cpp/src/arrow/util/logging.h +++ b/cpp/src/arrow/util/logging.h @@ -55,11 +55,7 @@ enum class ArrowLogLevel : int { }; #define ARROW_LOG_INTERNAL(level) ::arrow::util::ArrowLog(__FILE__, __LINE__, level) -// clang-format off -#define ARROW_LOG(level) if ( \ - arrow::util::ArrowLog::IsLevelEnabled(arrow::util::ArrowLogLevel::ARROW_##level)) \ - ARROW_LOG_INTERNAL(arrow::util::ArrowLogLevel::ARROW_##level) -// clang-format on +#define ARROW_LOG(level) ARROW_LOG_INTERNAL(::arrow::util::ArrowLogLevel::ARROW_##level) #define ARROW_IGNORE_EXPR(expr) ((void)(expr)) From 634e36a44c6cba7c214b440313705600fabaad88 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Mon, 18 Feb 2019 17:43:58 +0100 Subject: [PATCH 12/12] Use default argument value to `ConvertPyError` --- cpp/src/arrow/python/deserialize.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/python/deserialize.cc b/cpp/src/arrow/python/deserialize.cc index 95b7b7a4ec9..f17abbe7cb3 100644 --- a/cpp/src/arrow/python/deserialize.cc +++ b/cpp/src/arrow/python/deserialize.cc @@ -87,7 +87,7 @@ Status DeserializeDict(PyObject* context, const Array& array, int64_t start_idx, int ret = PyDict_SetItem(result.obj(), PyList_GET_ITEM(keys.obj(), i - start_idx), PyList_GET_ITEM(vals.obj(), i - start_idx)); if (ret != 0) { - return ConvertPyError(StatusCode::PythonError); + return ConvertPyError(); } } static PyObject* py_type = PyUnicode_FromString("_pytype_");