[jira] [Commented] (ARROW-2494) Return status codes from PlasmaClient::Seal
[ https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16448362#comment-16448362 ] ASF GitHub Bot commented on ARROW-2494: --- pitrou closed pull request #1932: ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing URL: https://github.com/apache/arrow/pull/1932 This is a PR merged from a forked repository. As GitHub hides the original diff on merge, it is displayed below for the sake of provenance: As this is a foreign pull request (from a fork), the diff is supplied below (as it won't show otherwise due to GitHub magic): diff --git a/cpp/src/arrow/status.h b/cpp/src/arrow/status.h index ed524ae6c..8b82e2ab7 100644 --- a/cpp/src/arrow/status.h +++ b/cpp/src/arrow/status.h @@ -80,7 +80,8 @@ enum class StatusCode : char { PythonError = 12, PlasmaObjectExists = 20, PlasmaObjectNonexistent = 21, - PlasmaStoreFull = 22 + PlasmaStoreFull = 22, + PlasmaObjectAlreadySealed = 23, }; #if defined(__clang__) @@ -144,6 +145,10 @@ class ARROW_EXPORT Status { return Status(StatusCode::PlasmaObjectNonexistent, msg); } + static Status PlasmaObjectAlreadySealed(const std::string& msg) { +return Status(StatusCode::PlasmaObjectAlreadySealed, msg); + } + static Status PlasmaStoreFull(const std::string& msg) { return Status(StatusCode::PlasmaStoreFull, msg); } @@ -168,6 +173,10 @@ class ARROW_EXPORT Status { bool IsPlasmaObjectNonexistent() const { return code() == StatusCode::PlasmaObjectNonexistent; } + // An already sealed object is tried to be sealed again. + bool IsPlasmaObjectAlreadySealed() const { +return code() == StatusCode::PlasmaObjectAlreadySealed; + } // An object is too large to fit into the plasma store. bool IsPlasmaStoreFull() const { return code() == StatusCode::PlasmaStoreFull; } diff --git a/cpp/src/plasma/client.cc b/cpp/src/plasma/client.cc index 0d44b1135..015c9731a 100644 --- a/cpp/src/plasma/client.cc +++ b/cpp/src/plasma/client.cc @@ -604,10 +604,15 @@ Status PlasmaClient::Seal(const ObjectID& object_id) { // Make sure this client has a reference to the object before sending the // request to Plasma. auto object_entry = objects_in_use_.find(object_id); - ARROW_CHECK(object_entry != objects_in_use_.end()) - << "Plasma client called seal an object without a reference to it"; - ARROW_CHECK(!object_entry->second->is_sealed) - << "Plasma client called seal an already sealed object"; + + if (object_entry == objects_in_use_.end()) { +return Status::PlasmaObjectNonexistent( +"Seal() called on an object without a reference to it"); + } + if (object_entry->second->is_sealed) { +return Status::PlasmaObjectAlreadySealed("Seal() called on an already sealed object"); + } + object_entry->second->is_sealed = true; /// Send the seal request to Plasma. static unsigned char digest[kDigestSize]; diff --git a/cpp/src/plasma/test/client_tests.cc b/cpp/src/plasma/test/client_tests.cc index 10e4e4f64..dad7688ba 100644 --- a/cpp/src/plasma/test/client_tests.cc +++ b/cpp/src/plasma/test/client_tests.cc @@ -90,12 +90,27 @@ class TestPlasmaStore : public ::testing::Test { PlasmaClient client2_; }; +TEST_F(TestPlasmaStore, SealErrorsTest) { + ObjectID object_id = ObjectID::from_random(); + + Status result = client_.Seal(object_id); + ASSERT_TRUE(result.IsPlasmaObjectNonexistent()); + + // Create object. + std::vector data(100, 0); + CreateObject(client_, object_id, {42}, data); + + // Trying to seal it again. + result = client_.Seal(object_id); + ASSERT_TRUE(result.IsPlasmaObjectAlreadySealed()); +} + TEST_F(TestPlasmaStore, DeleteTest) { ObjectID object_id = ObjectID::from_random(); // Test for deleting non-existance object. Status result = client_.Delete(object_id); - ASSERT_EQ(result.IsPlasmaObjectNonexistent(), true); + ASSERT_TRUE(result.IsPlasmaObjectNonexistent()); // Test for the object being in local Plasma store. // First create object. @@ -108,7 +123,7 @@ TEST_F(TestPlasmaStore, DeleteTest) { // Object is in use, can't be delete. result = client_.Delete(object_id); - ASSERT_EQ(result.IsUnknownError(), true); + ASSERT_TRUE(result.IsUnknownError()); // Avoid race condition of Plasma Manager waiting for notification. ARROW_CHECK_OK(client_.Release(object_id)); @@ -121,7 +136,7 @@ TEST_F(TestPlasmaStore, ContainsTest) { // Test for object non-existence. bool has_object; ARROW_CHECK_OK(client_.Contains(object_id, _object)); - ASSERT_EQ(has_object, false); + ASSERT_FALSE(has_object); // Test for the object being in local Plasma store. // First create object. @@ -131,7 +146,7 @@ TEST_F(TestPlasmaStore, ContainsTest) { std::vector object_buffers; ARROW_CHECK_OK(client_.Get({object_id}, -1, _buffers)); ARROW_CHECK_OK(client_.Contains(object_id, _object)); -
[jira] [Commented] (ARROW-2494) Return status codes from PlasmaClient::Seal
[ https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16447992#comment-16447992 ] ASF GitHub Bot commented on ARROW-2494: --- pitrou commented on a change in pull request #1932: ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing URL: https://github.com/apache/arrow/pull/1932#discussion_r183360950 ## File path: cpp/src/plasma/client.cc ## @@ -604,10 +604,16 @@ Status PlasmaClient::Seal(const ObjectID& object_id) { // Make sure this client has a reference to the object before sending the // request to Plasma. auto object_entry = objects_in_use_.find(object_id); - ARROW_CHECK(object_entry != objects_in_use_.end()) - << "Plasma client called seal an object without a reference to it"; - ARROW_CHECK(!object_entry->second->is_sealed) - << "Plasma client called seal an already sealed object"; + + if (object_entry == objects_in_use_.end()) { +return Status::PlasmaObjectNonexistent( +"Plasma client called seal an object without a reference to it"); Review comment: I'm not sure, but that sounds reasonable, and can be discussed on a JIRA issue indeed. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Return status codes from PlasmaClient::Seal > --- > > Key: ARROW-2494 > URL: https://issues.apache.org/jira/browse/ARROW-2494 > Project: Apache Arrow > Issue Type: Sub-task > Components: C++ >Reporter: Krisztian Szucs >Assignee: Krisztian Szucs >Priority: Major > Labels: pull-request-available > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2494) Return status codes from PlasmaClient::Seal
[ https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16447983#comment-16447983 ] ASF GitHub Bot commented on ARROW-2494: --- kszucs commented on a change in pull request #1932: ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing URL: https://github.com/apache/arrow/pull/1932#discussion_r183360587 ## File path: cpp/src/plasma/client.cc ## @@ -604,10 +604,16 @@ Status PlasmaClient::Seal(const ObjectID& object_id) { // Make sure this client has a reference to the object before sending the // request to Plasma. auto object_entry = objects_in_use_.find(object_id); - ARROW_CHECK(object_entry != objects_in_use_.end()) - << "Plasma client called seal an object without a reference to it"; - ARROW_CHECK(!object_entry->second->is_sealed) - << "Plasma client called seal an already sealed object"; + + if (object_entry == objects_in_use_.end()) { +return Status::PlasmaObjectNonexistent( +"Plasma client called seal an object without a reference to it"); Review comment: @pitrou I guess pretty printing of plasma object should follow the API of [arrow::pretty_print](https://github.com/apache/arrow/blob/master/cpp/src/arrow/pretty_print.h), if so should I create a JIRA ticket instead? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Return status codes from PlasmaClient::Seal > --- > > Key: ARROW-2494 > URL: https://issues.apache.org/jira/browse/ARROW-2494 > Project: Apache Arrow > Issue Type: Sub-task > Components: C++ >Reporter: Krisztian Szucs >Assignee: Krisztian Szucs >Priority: Major > Labels: pull-request-available > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2494) Return status codes from PlasmaClient::Seal
[ https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16447858#comment-16447858 ] ASF GitHub Bot commented on ARROW-2494: --- pitrou commented on issue #1932: ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing URL: https://github.com/apache/arrow/pull/1932#issuecomment-383517652 When `ARROW_CHECK` checks for an internal invariant, we can keep it. But in other cases (such as non-existent object ID or IO error), it would be better to replace it with an error return, yes. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Return status codes from PlasmaClient::Seal > --- > > Key: ARROW-2494 > URL: https://issues.apache.org/jira/browse/ARROW-2494 > Project: Apache Arrow > Issue Type: Sub-task > Components: C++ >Reporter: Krisztian Szucs >Assignee: Krisztian Szucs >Priority: Major > Labels: pull-request-available > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2494) Return status codes from PlasmaClient::Seal
[ https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16447856#comment-16447856 ] ASF GitHub Bot commented on ARROW-2494: --- pitrou commented on a change in pull request #1932: ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing URL: https://github.com/apache/arrow/pull/1932#discussion_r18281 ## File path: cpp/src/plasma/client.cc ## @@ -604,10 +604,16 @@ Status PlasmaClient::Seal(const ObjectID& object_id) { // Make sure this client has a reference to the object before sending the // request to Plasma. auto object_entry = objects_in_use_.find(object_id); - ARROW_CHECK(object_entry != objects_in_use_.end()) - << "Plasma client called seal an object without a reference to it"; - ARROW_CHECK(!object_entry->second->is_sealed) - << "Plasma client called seal an already sealed object"; + + if (object_entry == objects_in_use_.end()) { +return Status::PlasmaObjectNonexistent( +"Plasma client called seal an object without a reference to it"); Review comment: If there's a function to pretty-print object IDs then why not :-) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Return status codes from PlasmaClient::Seal > --- > > Key: ARROW-2494 > URL: https://issues.apache.org/jira/browse/ARROW-2494 > Project: Apache Arrow > Issue Type: Sub-task > Components: C++ >Reporter: Krisztian Szucs >Assignee: Krisztian Szucs >Priority: Major > Labels: pull-request-available > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2494) Return status codes from PlasmaClient::Seal
[ https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16447855#comment-16447855 ] ASF GitHub Bot commented on ARROW-2494: --- pitrou commented on a change in pull request #1932: ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing URL: https://github.com/apache/arrow/pull/1932#discussion_r18281 ## File path: cpp/src/plasma/client.cc ## @@ -604,10 +604,16 @@ Status PlasmaClient::Seal(const ObjectID& object_id) { // Make sure this client has a reference to the object before sending the // request to Plasma. auto object_entry = objects_in_use_.find(object_id); - ARROW_CHECK(object_entry != objects_in_use_.end()) - << "Plasma client called seal an object without a reference to it"; - ARROW_CHECK(!object_entry->second->is_sealed) - << "Plasma client called seal an already sealed object"; + + if (object_entry == objects_in_use_.end()) { +return Status::PlasmaObjectNonexistent( +"Plasma client called seal an object without a reference to it"); Review comment: If there's a function to pretty-print object IDs when why not :-) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Return status codes from PlasmaClient::Seal > --- > > Key: ARROW-2494 > URL: https://issues.apache.org/jira/browse/ARROW-2494 > Project: Apache Arrow > Issue Type: Sub-task > Components: C++ >Reporter: Krisztian Szucs >Assignee: Krisztian Szucs >Priority: Major > Labels: pull-request-available > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2494) Return status codes from PlasmaClient::Seal
[ https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16447853#comment-16447853 ] ASF GitHub Bot commented on ARROW-2494: --- kszucs commented on issue #1932: ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing URL: https://github.com/apache/arrow/pull/1932#issuecomment-383516365 @pitrou Generally We should replace all occurrences of `ARROW_CHECK`, right? I've created a subtask in JIRA, should I do the same with the remaining ones? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Return status codes from PlasmaClient::Seal > --- > > Key: ARROW-2494 > URL: https://issues.apache.org/jira/browse/ARROW-2494 > Project: Apache Arrow > Issue Type: Sub-task > Components: C++ >Reporter: Krisztian Szucs >Assignee: Krisztian Szucs >Priority: Major > Labels: pull-request-available > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2494) Return status codes from PlasmaClient::Seal
[ https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16447852#comment-16447852 ] ASF GitHub Bot commented on ARROW-2494: --- kszucs commented on issue #1932: ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing URL: https://github.com/apache/arrow/pull/1932#issuecomment-383516365 @pitrou I've created a subtask in JIRA. Generally We should replace all occurrences of `ARROW_CHECK`, right? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Return status codes from PlasmaClient::Seal > --- > > Key: ARROW-2494 > URL: https://issues.apache.org/jira/browse/ARROW-2494 > Project: Apache Arrow > Issue Type: Sub-task > Components: C++ >Reporter: Krisztian Szucs >Assignee: Krisztian Szucs >Priority: Major > Labels: pull-request-available > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2494) Return status codes from PlasmaClient::Seal
[ https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16447848#comment-16447848 ] ASF GitHub Bot commented on ARROW-2494: --- kszucs commented on a change in pull request #1932: ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing URL: https://github.com/apache/arrow/pull/1932#discussion_r183331248 ## File path: cpp/src/plasma/client.cc ## @@ -604,10 +604,16 @@ Status PlasmaClient::Seal(const ObjectID& object_id) { // Make sure this client has a reference to the object before sending the // request to Plasma. auto object_entry = objects_in_use_.find(object_id); - ARROW_CHECK(object_entry != objects_in_use_.end()) - << "Plasma client called seal an object without a reference to it"; - ARROW_CHECK(!object_entry->second->is_sealed) - << "Plasma client called seal an already sealed object"; + + if (object_entry == objects_in_use_.end()) { +return Status::PlasmaObjectNonexistent( +"Plasma client called seal an object without a reference to it"); Review comment: Me neither :) Shouldn't we incorporate the object ID in the message? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Return status codes from PlasmaClient::Seal > --- > > Key: ARROW-2494 > URL: https://issues.apache.org/jira/browse/ARROW-2494 > Project: Apache Arrow > Issue Type: Sub-task > Components: C++ >Reporter: Krisztian Szucs >Assignee: Krisztian Szucs >Priority: Major > Labels: pull-request-available > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2494) Return status codes from PlasmaClient::Seal
[ https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16447827#comment-16447827 ] ASF GitHub Bot commented on ARROW-2494: --- pitrou commented on a change in pull request #1932: ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing URL: https://github.com/apache/arrow/pull/1932#discussion_r183327690 ## File path: cpp/src/plasma/test/client_tests.cc ## @@ -90,6 +90,21 @@ class TestPlasmaStore : public ::testing::Test { PlasmaClient client2_; }; +TEST_F(TestPlasmaStore, SealErrorsTest) { + ObjectID object_id = ObjectID::from_random(); + + Status result = client_.Seal(object_id); + ASSERT_EQ(result.IsPlasmaObjectNonexistent(), true); Review comment: You can use ASSERT_TRUE(). This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Return status codes from PlasmaClient::Seal > --- > > Key: ARROW-2494 > URL: https://issues.apache.org/jira/browse/ARROW-2494 > Project: Apache Arrow > Issue Type: Sub-task > Components: C++ >Reporter: Krisztian Szucs >Assignee: Krisztian Szucs >Priority: Major > Labels: pull-request-available > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2494) Return status codes from PlasmaClient::Seal
[ https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16447826#comment-16447826 ] ASF GitHub Bot commented on ARROW-2494: --- pitrou commented on a change in pull request #1932: ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing URL: https://github.com/apache/arrow/pull/1932#discussion_r183327354 ## File path: cpp/src/plasma/client.cc ## @@ -604,10 +604,16 @@ Status PlasmaClient::Seal(const ObjectID& object_id) { // Make sure this client has a reference to the object before sending the // request to Plasma. auto object_entry = objects_in_use_.find(object_id); - ARROW_CHECK(object_entry != objects_in_use_.end()) - << "Plasma client called seal an object without a reference to it"; - ARROW_CHECK(!object_entry->second->is_sealed) - << "Plasma client called seal an already sealed object"; + + if (object_entry == objects_in_use_.end()) { +return Status::PlasmaObjectNonexistent( +"Plasma client called seal an object without a reference to it"); Review comment: I'm not a native English speaker, but while we're at it, perhaps we could fix the grammar in this error message and the one below? Something like "Seal() called on an object without a reference to it". This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Return status codes from PlasmaClient::Seal > --- > > Key: ARROW-2494 > URL: https://issues.apache.org/jira/browse/ARROW-2494 > Project: Apache Arrow > Issue Type: Sub-task > Components: C++ >Reporter: Krisztian Szucs >Assignee: Krisztian Szucs >Priority: Major > Labels: pull-request-available > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2494) Return status codes from PlasmaClient::Seal
[ https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16447828#comment-16447828 ] ASF GitHub Bot commented on ARROW-2494: --- pitrou commented on a change in pull request #1932: ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing URL: https://github.com/apache/arrow/pull/1932#discussion_r183327738 ## File path: cpp/src/plasma/test/client_tests.cc ## @@ -90,6 +90,21 @@ class TestPlasmaStore : public ::testing::Test { PlasmaClient client2_; }; +TEST_F(TestPlasmaStore, SealErrorsTest) { + ObjectID object_id = ObjectID::from_random(); + + Status result = client_.Seal(object_id); + ASSERT_EQ(result.IsPlasmaObjectNonexistent(), true); + + // Create object. + std::vector data(100, 0); + CreateObject(client_, object_id, {42}, data); + + // Trying to seal it again. + result = client_.Seal(object_id); + ASSERT_EQ(result.IsPlasmaObjectAlreadySealed(), true); Review comment: Same here. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Return status codes from PlasmaClient::Seal > --- > > Key: ARROW-2494 > URL: https://issues.apache.org/jira/browse/ARROW-2494 > Project: Apache Arrow > Issue Type: Sub-task > Components: C++ >Reporter: Krisztian Szucs >Assignee: Krisztian Szucs >Priority: Major > Labels: pull-request-available > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2494) Return status codes from PlasmaClient::Seal
[ https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16447815#comment-16447815 ] ASF GitHub Bot commented on ARROW-2494: --- kszucs opened a new pull request #1932: ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing URL: https://github.com/apache/arrow/pull/1932 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Return status codes from PlasmaClient::Seal > --- > > Key: ARROW-2494 > URL: https://issues.apache.org/jira/browse/ARROW-2494 > Project: Apache Arrow > Issue Type: Sub-task > Components: C++ >Reporter: Krisztian Szucs >Assignee: Krisztian Szucs >Priority: Major > Labels: pull-request-available > -- This message was sent by Atlassian JIRA (v7.6.3#76005)