You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/06/15 03:20:41 UTC
[GitHub] [arrow] drin opened a new pull request, #13383: ARROW-16769: [C++] Add Warn() function to Status
drin opened a new pull request, #13383:
URL: https://github.com/apache/arrow/pull/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");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898051392
##########
cpp/src/arrow/status.cc:
##########
@@ -131,6 +131,17 @@ void Status::Abort(const std::string& message) const {
std::abort();
}
+void Status::Warn() const { Warn(std::string()); }
+
+void Status::Warn(const std::string& message) const {
+ ARROW_LOG(WARNING) << "-- Arrow Warning --";
+ if (!message.empty()) {
+ ARROW_LOG(WARNING) << message;
+ }
+
+ ARROW_LOG(WARNING) << ToString() << "\n";
Review Comment:
will do!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vibhatha commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r897520475
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,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;
Review Comment:
I meant the warn message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898005116
##########
cpp/src/arrow/status.cc:
##########
@@ -131,6 +131,17 @@ void Status::Abort(const std::string& message) const {
std::abort();
}
+void Status::Warn() const { Warn(std::string()); }
+
+void Status::Warn(const std::string& message) const {
+ ARROW_LOG(WARNING) << "-- Arrow Warning --";
+ if (!message.empty()) {
+ ARROW_LOG(WARNING) << message;
+ }
+
+ ARROW_LOG(WARNING) << ToString() << "\n";
Review Comment:
Why not just put it all on one line? `WARNING: Warning message verbatim: Invalid: warn_summary`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r899683135
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,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;
+ st.Warn("Implicitly called RecordBatchReader::Close failed");
Review Comment:
So, I looked at why ARROW-16515 was created and it was related to `SubstraitExecutor` and keeping a reference to it. The destructor in that class uses `ARROW_CHECK_OK` which is similar to `ARROW_WARN_NOT_OK` but logs at the `FATAL` level instead. I don't see a clear test case for `Warn()` in that context.
For now, I created a test class that extends RecordBatchReader and returns non-OK in it's Close() method as a simple, lightweight approach to exercising this. I am not sure how important it is to validate the logging mechanism itself, but at least this adds code coverage of the macro and a "failing" Close() method called from a constructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898311816
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,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;
+ st.Warn("Implicitly called RecordBatchReader::Close failed");
Review Comment:
Ah, right. Since the current macro isn't used in many places I would be in favor of just making the macro always require a message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898400059
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,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;
+ st.Warn("Implicitly called RecordBatchReader::Close failed");
Review Comment:
cc @vibhatha
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898260491
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,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;
+ st.Warn("Implicitly called RecordBatchReader::Close failed");
Review Comment:
use ARROW_WARN_NOT_OK?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13383:
URL: https://github.com/apache/arrow/pull/13383#issuecomment-1158893351
Looks like everything's passing. Is this ready @drin?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on PR #13383:
URL: https://github.com/apache/arrow/pull/13383#issuecomment-1158969053
yep, I think so!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r899682481
##########
cpp/src/arrow/record_batch_test.cc:
##########
@@ -388,6 +389,34 @@ class TestRecordBatchReader : public ::testing::Test {
std::shared_ptr<RecordBatchReader> reader_;
};
+// A minimal class to test Status::Warn()
+class UncloseableReader : public RecordBatchReader {
+ public:
+ UncloseableReader(RecordBatchVector batches, std::shared_ptr<Schema> schema)
+ : schema_(std::move(schema)), it_(MakeVectorIterator(std::move(batches))) {}
+
+ ~UncloseableReader() { ARROW_WARN_NOT_OK(Close(), "Expect: uncloseable reader"); }
+
+ Status ReadNext(std::shared_ptr<RecordBatch>* batch) override {
+ return it_.Next().Value(batch);
+ }
+
+ std::shared_ptr<Schema> schema() const override { return schema_; }
+
+ Status Close() override { return Status::Invalid("uncloseable reader"); }
+
+ protected:
+ std::shared_ptr<Schema> schema_;
+ RecordBatchIterator it_;
+};
+
+TEST_F(TestRecordBatchReader, CloseAndWarn) {
+ auto uncloseable_reader_ =
+ std::make_shared<UncloseableReader>(batches_, reader_->schema());
+
+ ASSERT_EQ(Status::Invalid("uncloseable reader"), uncloseable_reader_->Close());
+}
Review Comment:
I'm not so sure this test is all that useful? If we just want to have coverage of Status::Warn, why not just call it directly from a test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vibhatha commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r897519297
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,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;
Review Comment:
Should it be
“Implicitly called RecordBatchReader::Close failed”?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on PR #13383:
URL: https://github.com/apache/arrow/pull/13383#issuecomment-1156932069
haha yeah, I just realized after some of the CI failed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vibhatha commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r897551565
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,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;
+ st.Warn("Implicitly called RecordBatchReader::Close failed: ");
Review Comment:
Sorry for the confusion, I have clicked the worng line. I meant this line. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898884145
##########
cpp/src/arrow/status.cc:
##########
@@ -131,6 +131,13 @@ void Status::Abort(const std::string& message) const {
std::abort();
}
+void Status::Warn() const { Warn(std::string()); }
+
+void Status::Warn(const std::string& message) const {
+ ARROW_LOG(WARNING) << "Arrow Warning -- "
Review Comment:
I don't think you need to add the "Arrow Warning" phrase. Instead that should be handled by `ARROW_LOG` itself, or whatever it redirects to.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13383:
URL: https://github.com/apache/arrow/pull/13383#issuecomment-1157437255
@drin There are more places that could be migrated. For example `FileDescriptor::CloseFromDestructor` and `internal::CloseFromDestructor`. Perhaps you can grep the codebase for warnings?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13383:
URL: https://github.com/apache/arrow/pull/13383#issuecomment-1158184003
Hmm, possibly a flaky test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vibhatha commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898559809
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,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;
+ st.Warn("Implicitly called RecordBatchReader::Close failed");
Review Comment:
@drin it would be good to add a test case like that. I am definitely sure, there could have been a good use case when this JIRA was created. We can gather some thoughts from there too.
cc @pitrou
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13383:
URL: https://github.com/apache/arrow/pull/13383#issuecomment-1163266599
Thanks for doing this @drin !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on PR #13383:
URL: https://github.com/apache/arrow/pull/13383#issuecomment-1163385164
thanks for all the help!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898277387
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,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;
+ st.Warn("Implicitly called RecordBatchReader::Close failed");
Review Comment:
I meant to ask earlier: if `ARROW_WARN_NOT_OK` only takes an expr, then to print a custom message you need to call `Warn()` directly; should I add another macro, something like `ARROW_WARN_MSG_NOT_OK` to also take a message, or should we drop that custom message?
I assume you can't overload a macro, not sure if there's a preference on an approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r899683807
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,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;
+ st.Warn("Implicitly called RecordBatchReader::Close failed");
Review Comment:
Well, there's another PR that's just removing the log instead…see https://github.com/apache/arrow/pull/13375/files#diff-0078df867c0f64dcfe79c149ee007f0933d164a32f122e28d79872a969b5be05
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r897527445
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,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;
Review Comment:
I mostly wanted to maintain the message that was already there. But otherwise the message is meant to convey that Close was called implicitly (user did not explicitly close the reader) and that the status was non-successful.
I could change it but I don't have strong opinions on what would be better
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898398633
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,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;
+ st.Warn("Implicitly called RecordBatchReader::Close failed");
Review Comment:
done. With this change, I was thinking of trying to have a test that implicitly closes a RecordBatchReader in a scenario where there is a non-successful status. Looking at the PR that added a definition for `Close()` (https://github.com/apache/arrow/pull/13205), it seems like places where this might be done would be `compute/exec/plan_test.cc` or `dataset/scanner_test.cc`.
Should I try to add this? I am not sure that there's much value add validation-wise (seems like log unit tests and status unit tests should be sufficient).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r899169277
##########
cpp/src/arrow/status.cc:
##########
@@ -131,6 +131,13 @@ void Status::Abort(const std::string& message) const {
std::abort();
}
+void Status::Warn() const { Warn(std::string()); }
+
+void Status::Warn(const std::string& message) const {
+ ARROW_LOG(WARNING) << "Arrow Warning -- "
+ << message << ": " << ToString();
Review Comment:
good point, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on PR #13383:
URL: https://github.com/apache/arrow/pull/13383#issuecomment-1158159560
link to error (for convenience): https://app.travis-ci.com/github/apache/arrow/jobs/573832051#L9619
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898008205
##########
cpp/src/arrow/status.cc:
##########
@@ -134,9 +134,8 @@ void Status::Abort(const std::string& message) const {
void Status::Warn() const { Warn(std::string()); }
void Status::Warn(const std::string& message) const {
- ARROW_LOG(WARNING) << "-- Arrow Warning --"
- << message << "\n"
- << ToString() << "\n";
+ ARROW_LOG(WARNING) << "-- Arrow Warning --\n"
+ << message << "\n" << ToString();
Review Comment:
maybe I should remove the "\n" and replace with ": "?
I also kept the header (`-- Arrow Warning --`) in the same style as the abort message, but maybe it'd be nice to change it to `[Arrow Warning] -- ` and then remove that newline as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898009543
##########
cpp/src/arrow/status.cc:
##########
@@ -134,9 +134,8 @@ void Status::Abort(const std::string& message) const {
void Status::Warn() const { Warn(std::string()); }
void Status::Warn(const std::string& message) const {
- ARROW_LOG(WARNING) << "-- Arrow Warning --"
- << message << "\n"
- << ToString() << "\n";
+ ARROW_LOG(WARNING) << "-- Arrow Warning --\n"
+ << message << "\n" << ToString();
Review Comment:
or maybe adding `[]` isn't that helpful, but removing the newline would be.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vibhatha commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r897552560
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,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;
+ st.Warn("Implicitly called RecordBatchReader::Close failed: ");
Review Comment:
Should it be?
```suggestion
st.Warn("Implicitly called RecordBatchReader::Close failed");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vibhatha commented on pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13383:
URL: https://github.com/apache/arrow/pull/13383#issuecomment-1156009570
cc @lidavidm need some help to activate the CIs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13383:
URL: https://github.com/apache/arrow/pull/13383#issuecomment-1156911302
You're gonna want to [lint](https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci) the code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898885164
##########
cpp/src/arrow/status.cc:
##########
@@ -131,6 +131,13 @@ void Status::Abort(const std::string& message) const {
std::abort();
}
+void Status::Warn() const { Warn(std::string()); }
+
+void Status::Warn(const std::string& message) const {
+ ARROW_LOG(WARNING) << "Arrow Warning -- "
+ << message << ": " << ToString();
Review Comment:
If message is empty (which is the case when calling `Warn()` without arguments, this will leave a spurious colon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r897999493
##########
cpp/src/arrow/status.cc:
##########
@@ -131,6 +131,17 @@ void Status::Abort(const std::string& message) const {
std::abort();
}
+void Status::Warn() const { Warn(std::string()); }
+
+void Status::Warn(const std::string& message) const {
+ ARROW_LOG(WARNING) << "-- Arrow Warning --";
+ if (!message.empty()) {
+ ARROW_LOG(WARNING) << message;
+ }
+
+ ARROW_LOG(WARNING) << ToString() << "\n";
Review Comment:
I liked these suggestions, but I figured it'd be better to go ahead and combine all of them. Since it seems like this comment becomes a hanging comment, this is what I changed the code to:
```cpp
ARROW_LOG(WARNING) << "-- Arrow Warning --\n"
<< message << "\n" << ToString();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898003955
##########
cpp/src/arrow/status.cc:
##########
@@ -131,6 +131,17 @@ void Status::Abort(const std::string& message) const {
std::abort();
}
+void Status::Warn() const { Warn(std::string()); }
+
+void Status::Warn(const std::string& message) const {
+ ARROW_LOG(WARNING) << "-- Arrow Warning --";
+ if (!message.empty()) {
+ ARROW_LOG(WARNING) << message;
+ }
+
+ ARROW_LOG(WARNING) << ToString() << "\n";
Review Comment:
Here's an update to what the logging looks like. I think left alignment leaves something to be desired:
```
/Users/octalene/code/arrow-dev/cpp/src/arrow/status.cc:137: -- Arrow Warning --
Test warning message
Invalid: warn_summary
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] drin commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r899683441
##########
cpp/src/arrow/record_batch_test.cc:
##########
@@ -388,6 +389,34 @@ class TestRecordBatchReader : public ::testing::Test {
std::shared_ptr<RecordBatchReader> reader_;
};
+// A minimal class to test Status::Warn()
+class UncloseableReader : public RecordBatchReader {
+ public:
+ UncloseableReader(RecordBatchVector batches, std::shared_ptr<Schema> schema)
+ : schema_(std::move(schema)), it_(MakeVectorIterator(std::move(batches))) {}
+
+ ~UncloseableReader() { ARROW_WARN_NOT_OK(Close(), "Expect: uncloseable reader"); }
+
+ Status ReadNext(std::shared_ptr<RecordBatch>* batch) override {
+ return it_.Next().Value(batch);
+ }
+
+ std::shared_ptr<Schema> schema() const override { return schema_; }
+
+ Status Close() override { return Status::Invalid("uncloseable reader"); }
+
+ protected:
+ std::shared_ptr<Schema> schema_;
+ RecordBatchIterator it_;
+};
+
+TEST_F(TestRecordBatchReader, CloseAndWarn) {
+ auto uncloseable_reader_ =
+ std::make_shared<UncloseableReader>(batches_, reader_->schema());
+
+ ASSERT_EQ(Status::Invalid("uncloseable reader"), uncloseable_reader_->Close());
+}
Review Comment:
That's reasonable. I can do that instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13383:
URL: https://github.com/apache/arrow/pull/13383#issuecomment-1155939339
https://issues.apache.org/jira/browse/ARROW-16769
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r897870038
##########
cpp/src/arrow/status.cc:
##########
@@ -131,6 +131,17 @@ void Status::Abort(const std::string& message) const {
std::abort();
}
+void Status::Warn() const { Warn(std::string()); }
+
+void Status::Warn(const std::string& message) const {
+ ARROW_LOG(WARNING) << "-- Arrow Warning --";
+ if (!message.empty()) {
+ ARROW_LOG(WARNING) << message;
+ }
+
+ ARROW_LOG(WARNING) << ToString() << "\n";
Review Comment:
FWIW, it might be better to consolidate the message and status into a single log so that concurrent logs don't get mixed up.
```suggestion
ARROW_LOG(WARNING) << "-- Arrow Warning --";
if (!message.empty()) {
ARROW_LOG(WARNING) << message;
}
ARROW_LOG(WARNING) << ToString();
```
##########
cpp/src/arrow/status.cc:
##########
@@ -131,6 +131,17 @@ void Status::Abort(const std::string& message) const {
std::abort();
}
+void Status::Warn() const { Warn(std::string()); }
+
+void Status::Warn(const std::string& message) const {
+ ARROW_LOG(WARNING) << "-- Arrow Warning --";
+ if (!message.empty()) {
+ ARROW_LOG(WARNING) << message;
+ }
+
+ ARROW_LOG(WARNING) << ToString() << "\n";
Review Comment:
That way even the original status messages will keep working
##########
cpp/src/arrow/status.cc:
##########
@@ -131,6 +131,17 @@ void Status::Abort(const std::string& message) const {
std::abort();
}
+void Status::Warn() const { Warn(std::string()); }
+
+void Status::Warn(const std::string& message) const {
+ ARROW_LOG(WARNING) << "-- Arrow Warning --";
+ if (!message.empty()) {
+ ARROW_LOG(WARNING) << message;
+ }
+
+ ARROW_LOG(WARNING) << ToString() << "\n";
Review Comment:
I would honestly just make it
```suggestion
ARROW_LOG(WARNING) << "-- Arrow Warning --";
ARROW_LOG(WARNING) << message << ToString();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm merged pull request #13383: ARROW-16769: [C++] Add Warn() function to Status
Posted by GitBox <gi...@apache.org>.
lidavidm merged PR #13383:
URL: https://github.com/apache/arrow/pull/13383
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org