This is an automated email from the ASF dual-hosted git repository.
apitrou pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/master by this push:
new 135d481 ARROW-4583: [Plasma] Fix some small bugs reported by code
scan tool
135d481 is described below
commit 135d48146e980b6d3d6b72eb2d9455bb8a5f1394
Author: Yuhong Guo <[email protected]>
AuthorDate: Tue Feb 19 12:05:24 2019 +0100
ARROW-4583: [Plasma] Fix some small bugs reported by code scan tool
We used a static code scan tool to scan arrow code. There are several
possible bugs:
1. The return value of `PyDict_SetItem` is not used.
2. Currently, `EventLoop:: Shutdown` should be called explicitly, which is
error-prone and causing leak when the user forgets to call it.
3. There is an unclosed file descriptor in `io.cc` when path name is too
long.
Besides, we also made the following small changes:
1. When we use Plasma in Yarn and when a node uses too much memory, a
SIGTERM signal will be sent to Plasma. Current plasma will exit silently. We
also some log to plasma store to help us to debug.
2. `ARROW_LOG` will always evaluate the output expression even when it is
not enabled, which is not efficient.
3. The constructor of Java class `ObjectStoreData` is private, which is not
convenient when we want to create a mock plasma store.
4. Fix a call to `ObjectStoreData` which misplaces `meta` and `data`
according to
https://github.com/apache/arrow/blob/master/java/plasma/src/main/java/org/apache/arrow/plasma/ObjectStoreLink.java#L32
.
Author: Yuhong Guo <[email protected]>
Author: Antoine Pitrou <[email protected]>
Closes #3656 from guoyuhong/fixPlasma and squashes the following commits:
634e36a44 <Antoine Pitrou> Use default argument value to `ConvertPyError`
b547f2f5d <Yuhong Guo> remove if from ARROW_LOG
440f0975f <Yuhong Guo> Address comment
d3eb22ff8 <Yuhong Guo> Lint
79b4af3ae <Yuhong Guo> Fix and Lint
434a039d2 <Yuhong Guo> Make constructor of ObjectStoreData public
b2ddba6ce <Yuhong Guo> Fix misplace of meta and data in PlasmaClient.java
a66740258 <Yuhong Guo> Do not evaluate logging strings when logging is not
enabled.
5be7b89ff <Yuhong Guo> Fix unclosed fd reported by code scan tool
3a917d41d <Yuhong Guo> Fix not used return value in deserialize.cc reported
by code scan tool
ed56a4808 <Yuhong Guo> Fix possible unclosed EventLoop reported by code
scanning tool
3fed9264a <Yuhong Guo> Add plasma log
---
cpp/src/arrow/python/deserialize.cc | 7 +++++--
cpp/src/arrow/util/logging.cc | 4 ++++
cpp/src/arrow/util/logging.h | 7 +++++++
cpp/src/plasma/events.cc | 9 ++++++++-
cpp/src/plasma/events.h | 2 ++
cpp/src/plasma/io.cc | 1 +
cpp/src/plasma/store.cc | 5 +++++
.../src/main/java/org/apache/arrow/plasma/ObjectStoreLink.java | 2 +-
.../src/main/java/org/apache/arrow/plasma/PlasmaClient.java | 2 +-
9 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/cpp/src/arrow/python/deserialize.cc
b/cpp/src/arrow/python/deserialize.cc
index f13070a..f17abbe 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),
- PyList_GET_ITEM(vals.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 ConvertPyError();
+ }
}
static PyObject* py_type = PyUnicode_FromString("_pytype_");
if (PyDict_Contains(result.obj(), py_type)) {
diff --git a/cpp/src/arrow/util/logging.cc b/cpp/src/arrow/util/logging.cc
index 7e36840..2d7cc60 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 5ea7820..18746c5 100644
--- a/cpp/src/arrow/util/logging.h
+++ b/cpp/src/arrow/util/logging.h
@@ -56,6 +56,7 @@ 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_IGNORE_EXPR(expr) ((void)(expr))
#define ARROW_CHECK(condition)
\
@@ -173,6 +174,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);
diff --git a/cpp/src/plasma/events.cc b/cpp/src/plasma/events.cc
index 7c977b0..d49c577 100644
--- a/cpp/src/plasma/events.cc
+++ b/cpp/src/plasma/events.cc
@@ -80,7 +80,14 @@ 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<TimerCallback>(new TimerCallback(callback));
diff --git a/cpp/src/plasma/events.h b/cpp/src/plasma/events.h
index 109540e..765be9c 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.
diff --git a/cpp/src/plasma/io.cc b/cpp/src/plasma/io.cc
index cc42542..9ba23e5 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);
diff --git a/cpp/src/plasma/store.cc b/cpp/src/plasma/store.cc
index 82f648c..c55da30 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 create 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<PlasmaStoreRunner> g_runner =
nullptr;
void HandleSignal(int signal) {
if (signal == SIGTERM) {
+ ARROW_LOG(INFO) << "SIGTERM Signal received, closing Plasma Server...";
if (g_runner != nullptr) {
g_runner->Stop();
}
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 f933c85..520549b 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;
}
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 a708f41..af53c61 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 class PlasmaClient implements ObjectStoreLink {
} else {
meta = null;
}
- ret.add(new ObjectStoreData(data, meta));
+ ret.add(new ObjectStoreData(meta, data));
}
}
return ret;
