This is an automated email from the ASF dual-hosted git repository.
lidavidm 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 50f6fcad6c ARROW-16769: [C++] Add Warn() function to Status (#13383)
50f6fcad6c is described below
commit 50f6fcad6cc09c06e78dcd09ad07218b86e689de
Author: octalene <[email protected]>
AuthorDate: Wed Jun 22 08:00:26 2022 -0700
ARROW-16769: [C++] Add Warn() function to Status (#13383)
This defines 2 new functions: `Status::Warn()` and `Status::Warn(const
string&)`.
For non-successful statuses, these functions log:
* a simple header -- `-- Arrow Warning --`
* a detail message (optional)
* the string representation of the status itself
Here is example output:
```cpp
/.../arrow/status.cc:137: -- Arrow Warning --
/.../arrow/status.cc:139: Test warning message
/.../arrow/status.cc:142: Invalid: warn_summary
```
For the following example code:
```cpp
Status warn_status { Status::Invalid("warn_summary") };
warn_status.Warn("Test warning message");
```
Lead-authored-by: Aldrin M <[email protected]>
Co-authored-by: octalene <[email protected]>
Signed-off-by: David Li <[email protected]>
---
cpp/src/arrow/filesystem/hdfs.cc | 7 +-----
cpp/src/arrow/flight/client.cc | 11 ++-------
cpp/src/arrow/flight/server.cc | 5 +---
cpp/src/arrow/flight/transport/ucx/ucx_client.cc | 6 +----
cpp/src/arrow/flight/transport/ucx/ucx_server.cc | 10 ++------
cpp/src/arrow/io/hdfs.cc | 8 +++++--
cpp/src/arrow/python/flight.cc | 29 +++++++++---------------
cpp/src/arrow/record_batch.cc | 5 +---
cpp/src/arrow/record_batch_test.cc | 1 +
cpp/src/arrow/status.cc | 6 +++++
cpp/src/arrow/status.h | 12 ++++++++++
cpp/src/arrow/status_test.cc | 4 ++++
cpp/src/arrow/util/io_util.cc | 18 ++++-----------
cpp/src/arrow/util/logging.h | 2 --
14 files changed, 52 insertions(+), 72 deletions(-)
diff --git a/cpp/src/arrow/filesystem/hdfs.cc b/cpp/src/arrow/filesystem/hdfs.cc
index a9d8f0bba9..6f16667ffd 100644
--- a/cpp/src/arrow/filesystem/hdfs.cc
+++ b/cpp/src/arrow/filesystem/hdfs.cc
@@ -48,12 +48,7 @@ class HadoopFileSystem::Impl {
Impl(HdfsOptions options, const io::IOContext& io_context)
: options_(std::move(options)), io_context_(io_context) {}
- ~Impl() {
- Status st = Close();
- if (!st.ok()) {
- ARROW_LOG(WARNING) << "Failed to disconnect hdfs client: " <<
st.ToString();
- }
- }
+ ~Impl() { ARROW_WARN_NOT_OK(Close(), "Failed to disconnect hdfs client"); }
Status Init() {
io::internal::LibHdfsShim* driver_shim;
diff --git a/cpp/src/arrow/flight/client.cc b/cpp/src/arrow/flight/client.cc
index dbf840db05..c88ce4a08b 100644
--- a/cpp/src/arrow/flight/client.cc
+++ b/cpp/src/arrow/flight/client.cc
@@ -377,10 +377,7 @@ class ClientStreamWriter : public FlightStreamWriter {
if (closed_) return;
// Implicitly Close() on destruction, though it's best if the
// application closes explicitly
- auto status = Close();
- if (!status.ok()) {
- ARROW_LOG(WARNING) << "Close() failed: " << status.ToString();
- }
+ ARROW_WARN_NOT_OK(Close(), "Close() failed");
}
Status Begin(const std::shared_ptr<Schema>& schema,
@@ -513,11 +510,7 @@ class ClientStreamWriter : public FlightStreamWriter {
FlightClient::FlightClient() : closed_(false), write_size_limit_bytes_(0) {}
FlightClient::~FlightClient() {
- auto st = Close();
- if (!st.ok()) {
- ARROW_LOG(WARNING) << "FlightClient::~FlightClient(): Close() failed: "
- << st.ToString();
- }
+ ARROW_WARN_NOT_OK(Close(), "FlightClient::~FlightClient(): Close() failed");
}
arrow::Result<std::unique_ptr<FlightClient>> FlightClient::Connect(
diff --git a/cpp/src/arrow/flight/server.cc b/cpp/src/arrow/flight/server.cc
index ae15585621..428cd9794d 100644
--- a/cpp/src/arrow/flight/server.cc
+++ b/cpp/src/arrow/flight/server.cc
@@ -127,10 +127,7 @@ struct FlightServerBase::Impl {
}
auto instance = running_instance_.load();
if (instance != nullptr) {
- auto status = instance->transport_->Shutdown();
- if (!status.ok()) {
- ARROW_LOG(WARNING) << "Error shutting down server: " <<
status.ToString();
- }
+ ARROW_WARN_NOT_OK(instance->transport_->Shutdown(), "Error shutting down
server");
}
}
};
diff --git a/cpp/src/arrow/flight/transport/ucx/ucx_client.cc
b/cpp/src/arrow/flight/transport/ucx/ucx_client.cc
index 810f2c482a..14b5638ada 100644
--- a/cpp/src/arrow/flight/transport/ucx/ucx_client.cc
+++ b/cpp/src/arrow/flight/transport/ucx/ucx_client.cc
@@ -513,11 +513,7 @@ class UcxClientImpl : public
arrow::flight::internal::ClientTransport {
virtual ~UcxClientImpl() {
if (!ucp_context_) return;
- auto status = Close();
- if (!status.ok()) {
- ARROW_LOG(WARNING) << "UcxClientImpl errored in Close() in destructor: "
- << status.ToString();
- }
+ ARROW_WARN_NOT_OK(Close(), "UcxClientImpl errored in Close() in
destructor");
}
Status Init(const FlightClientOptions& options, const Location& location,
diff --git a/cpp/src/arrow/flight/transport/ucx/ucx_server.cc
b/cpp/src/arrow/flight/transport/ucx/ucx_server.cc
index 74a9311d0c..d7ddbfab06 100644
--- a/cpp/src/arrow/flight/transport/ucx/ucx_server.cc
+++ b/cpp/src/arrow/flight/transport/ucx/ucx_server.cc
@@ -185,10 +185,7 @@ class UcxServerImpl : public
arrow::flight::internal::ServerTransport {
virtual ~UcxServerImpl() {
if (listening_.load()) {
- auto st = Shutdown();
- if (!st.ok()) {
- ARROW_LOG(WARNING) << "Server did not shut down properly: " <<
st.ToString();
- }
+ ARROW_WARN_NOT_OK(Shutdown(), "Server did not shut down properly");
}
}
@@ -521,10 +518,7 @@ class UcxServerImpl : public
arrow::flight::internal::ServerTransport {
pending_connections_.pop();
auto submitted = rpc_pool_->Submit([this, request]() {
WorkerLoop(request); });
- if (!submitted.ok()) {
- ARROW_LOG(WARNING) << "Failed to submit task to handle client "
- << submitted.status().ToString();
- }
+ ARROW_WARN_NOT_OK(submitted.status(), "Failed to submit task to
handle client");
}
}
diff --git a/cpp/src/arrow/io/hdfs.cc b/cpp/src/arrow/io/hdfs.cc
index 5cd3369383..5d3edcd3ba 100644
--- a/cpp/src/arrow/io/hdfs.cc
+++ b/cpp/src/arrow/io/hdfs.cc
@@ -228,7 +228,9 @@ HdfsReadableFile::HdfsReadableFile(const io::IOContext&
io_context) {
impl_.reset(new HdfsReadableFileImpl(io_context.pool()));
}
-HdfsReadableFile::~HdfsReadableFile() { ARROW_WARN_NOT_OK(impl_->Close()); }
+HdfsReadableFile::~HdfsReadableFile() {
+ ARROW_WARN_NOT_OK(impl_->Close(), "Failed to close HdfsReadableFile");
+}
Status HdfsReadableFile::Close() { return impl_->Close(); }
@@ -314,7 +316,9 @@ class HdfsOutputStream::HdfsOutputStreamImpl : public
HdfsAnyFileImpl {
HdfsOutputStream::HdfsOutputStream() { impl_.reset(new
HdfsOutputStreamImpl()); }
-HdfsOutputStream::~HdfsOutputStream() { ARROW_WARN_NOT_OK(impl_->Close()); }
+HdfsOutputStream::~HdfsOutputStream() {
+ ARROW_WARN_NOT_OK(impl_->Close(), "Failed to close HdfsOutputStream");
+}
Status HdfsOutputStream::Close() { return impl_->Close(); }
diff --git a/cpp/src/arrow/python/flight.cc b/cpp/src/arrow/python/flight.cc
index 1180f41a82..51155e5383 100644
--- a/cpp/src/arrow/python/flight.cc
+++ b/cpp/src/arrow/python/flight.cc
@@ -291,9 +291,7 @@ void
PyServerMiddleware::SendingHeaders(arrow::flight::AddCallHeaders* outgoing_
return status;
});
- if (!status.ok()) {
- ARROW_LOG(WARNING) << "Python server middleware failed in SendingHeaders:
" << status;
- }
+ ARROW_WARN_NOT_OK(status, "Python server middleware failed in
SendingHeaders");
}
void PyServerMiddleware::CallCompleted(const Status& call_status) {
@@ -302,9 +300,8 @@ void PyServerMiddleware::CallCompleted(const Status&
call_status) {
RETURN_NOT_OK(CheckPyError());
return status;
});
- if (!status.ok()) {
- ARROW_LOG(WARNING) << "Python server middleware failed in CallCompleted: "
<< status;
- }
+
+ ARROW_WARN_NOT_OK(status, "Python server middleware failed in
CallCompleted");
}
std::string PyServerMiddleware::name() const { return kPyServerMiddlewareName;
}
@@ -328,9 +325,8 @@ void PyClientMiddlewareFactory::StartCall(
RETURN_NOT_OK(CheckPyError());
return status;
});
- if (!status.ok()) {
- ARROW_LOG(WARNING) << "Python client middleware failed in StartCall: " <<
status;
- }
+
+ ARROW_WARN_NOT_OK(status, "Python client middleware failed in StartCall");
}
PyClientMiddleware::PyClientMiddleware(PyObject* middleware, Vtable vtable)
@@ -345,9 +341,8 @@ void
PyClientMiddleware::SendingHeaders(arrow::flight::AddCallHeaders* outgoing_
RETURN_NOT_OK(CheckPyError());
return status;
});
- if (!status.ok()) {
- ARROW_LOG(WARNING) << "Python client middleware failed in StartCall: " <<
status;
- }
+
+ ARROW_WARN_NOT_OK(status, "Python client middleware failed in StartCall");
}
void PyClientMiddleware::ReceivedHeaders(
@@ -357,9 +352,8 @@ void PyClientMiddleware::ReceivedHeaders(
RETURN_NOT_OK(CheckPyError());
return status;
});
- if (!status.ok()) {
- ARROW_LOG(WARNING) << "Python client middleware failed in StartCall: " <<
status;
- }
+
+ ARROW_WARN_NOT_OK(status, "Python client middleware failed in StartCall");
}
void PyClientMiddleware::CallCompleted(const Status& call_status) {
@@ -368,9 +362,8 @@ void PyClientMiddleware::CallCompleted(const Status&
call_status) {
RETURN_NOT_OK(CheckPyError());
return status;
});
- if (!status.ok()) {
- ARROW_LOG(WARNING) << "Python client middleware failed in StartCall: " <<
status;
- }
+
+ ARROW_WARN_NOT_OK(status, "Python client middleware failed in StartCall");
}
Status CreateFlightInfo(const std::shared_ptr<arrow::Schema>& schema,
diff --git a/cpp/src/arrow/record_batch.cc b/cpp/src/arrow/record_batch.cc
index 6210eb405d..ba9a843690 100644
--- a/cpp/src/arrow/record_batch.cc
+++ b/cpp/src/arrow/record_batch.cc
@@ -391,10 +391,7 @@ Result<std::shared_ptr<RecordBatchReader>>
RecordBatchReader::Make(
}
RecordBatchReader::~RecordBatchReader() {
- auto st = this->Close();
- if (!st.ok()) {
- ARROW_LOG(WARNING) << "Implicitly called RecordBatchReader::Close failed:
" << st;
- }
+ ARROW_WARN_NOT_OK(this->Close(), "Implicitly called RecordBatchReader::Close
failed");
}
} // namespace arrow
diff --git a/cpp/src/arrow/record_batch_test.cc
b/cpp/src/arrow/record_batch_test.cc
index b9d4ce65cf..83371c94e4 100644
--- a/cpp/src/arrow/record_batch_test.cc
+++ b/cpp/src/arrow/record_batch_test.cc
@@ -34,6 +34,7 @@
#include "arrow/testing/gtest_util.h"
#include "arrow/testing/random.h"
#include "arrow/type.h"
+#include "arrow/util/iterator.h"
#include "arrow/util/key_value_metadata.h"
namespace arrow {
diff --git a/cpp/src/arrow/status.cc b/cpp/src/arrow/status.cc
index 0f02cb57a2..168b05df33 100644
--- a/cpp/src/arrow/status.cc
+++ b/cpp/src/arrow/status.cc
@@ -131,6 +131,12 @@ void Status::Abort(const std::string& message) const {
std::abort();
}
+void Status::Warn() const { ARROW_LOG(WARNING) << ToString(); }
+
+void Status::Warn(const std::string& message) const {
+ ARROW_LOG(WARNING) << message << ": " << ToString();
+}
+
#ifdef ARROW_EXTRA_ERROR_CONTEXT
void Status::AddContextLine(const char* filename, int line, const char* expr) {
ARROW_CHECK(!ok()) << "Cannot add context line to ok status";
diff --git a/cpp/src/arrow/status.h b/cpp/src/arrow/status.h
index 056d60d6f3..3bf6ca8b37 100644
--- a/cpp/src/arrow/status.h
+++ b/cpp/src/arrow/status.h
@@ -58,6 +58,15 @@
ARROW_RETURN_IF_(!__s.ok(), __s, ARROW_STRINGIFY(status)); \
} while (false)
+/// \brief Given `expr` and `warn_msg`; log `warn_msg` if `expr` is a non-ok
status
+#define ARROW_WARN_NOT_OK(expr, warn_msg) \
+ do { \
+ ::arrow::Status _s = (expr); \
+ if (ARROW_PREDICT_FALSE(!_s.ok())) { \
+ _s.Warn(warn_msg); \
+ } \
+ } while (false)
+
#define RETURN_NOT_OK_ELSE(s, else_) \
do { \
::arrow::Status _s = ::arrow::internal::GenericToStatus(s); \
@@ -336,6 +345,9 @@ class ARROW_MUST_USE_TYPE ARROW_EXPORT Status : public
util::EqualityComparable<
return FromArgs(code(), std::forward<Args>(args)...).WithDetail(detail());
}
+ void Warn() const;
+ void Warn(const std::string& message) const;
+
[[noreturn]] void Abort() const;
[[noreturn]] void Abort(const std::string& message) const;
diff --git a/cpp/src/arrow/status_test.cc b/cpp/src/arrow/status_test.cc
index a8e1d1ca9a..b446705933 100644
--- a/cpp/src/arrow/status_test.cc
+++ b/cpp/src/arrow/status_test.cc
@@ -72,6 +72,10 @@ TEST(StatusTest, TestWithDetail) {
ASSERT_EQ(new_status.detail(), detail);
}
+TEST(StatusTest, TestCoverageWarnNotOK) {
+ ARROW_WARN_NOT_OK(Status::Invalid("invalid"), "Expected warning");
+}
+
TEST(StatusTest, AndStatus) {
Status a = Status::OK();
Status b = Status::OK();
diff --git a/cpp/src/arrow/util/io_util.cc b/cpp/src/arrow/util/io_util.cc
index eacaba4e60..11ae80d03e 100644
--- a/cpp/src/arrow/util/io_util.cc
+++ b/cpp/src/arrow/util/io_util.cc
@@ -1002,10 +1002,7 @@ FileDescriptor&
FileDescriptor::operator=(FileDescriptor&& other) {
}
void FileDescriptor::CloseFromDestructor(int fd) {
- auto st = FileClose(fd);
- if (!st.ok()) {
- ARROW_LOG(WARNING) << "Failed to close file descriptor: " << st.ToString();
- }
+ ARROW_WARN_NOT_OK(FileClose(fd), "Failed to close file descriptor");
}
FileDescriptor::~FileDescriptor() {
@@ -1272,12 +1269,7 @@ class SelfPipeImpl : public SelfPipe {
return pipe_.wfd.Close();
}
- ~SelfPipeImpl() {
- auto st = Shutdown();
- if (!st.ok()) {
- ARROW_LOG(WARNING) << "On self-pipe destruction: " << st.ToString();
- }
- }
+ ~SelfPipeImpl() { ARROW_WARN_NOT_OK(Shutdown(), "On self-pipe destruction");
}
protected:
Status ClosedPipe() const { return Status::Invalid("Self-pipe closed"); }
@@ -1912,10 +1904,8 @@ Result<std::unique_ptr<TemporaryDir>>
TemporaryDir::Make(const std::string& pref
TemporaryDir::TemporaryDir(PlatformFilename&& path) : path_(std::move(path)) {}
TemporaryDir::~TemporaryDir() {
- Status st = DeleteDirTree(path_).status();
- if (!st.ok()) {
- ARROW_LOG(WARNING) << "When trying to delete temporary directory: " << st;
- }
+ ARROW_WARN_NOT_OK(DeleteDirTree(path_).status(),
+ "When trying to delete temporary directory");
}
SignalHandler::SignalHandler() : SignalHandler(static_cast<Callback>(nullptr))
{}
diff --git a/cpp/src/arrow/util/logging.h b/cpp/src/arrow/util/logging.h
index f9e78e008f..2baa560563 100644
--- a/cpp/src/arrow/util/logging.h
+++ b/cpp/src/arrow/util/logging.h
@@ -86,8 +86,6 @@ enum class ArrowLogLevel : int {
#define ARROW_CHECK_GE(val1, val2) ARROW_CHECK((val1) >= (val2))
#define ARROW_CHECK_GT(val1, val2) ARROW_CHECK((val1) > (val2))
-#define ARROW_WARN_NOT_OK(s) ARROW_CHECK_OK_PREPEND(s, "Bad status", WARNING)
-
#ifdef NDEBUG
#define ARROW_DFATAL ::arrow::util::ArrowLogLevel::ARROW_WARNING
