You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by ap...@apache.org on 2018/04/23 16:08:37 UTC
[arrow] branch master updated: ARROW-2494: [C++] Return status
codes from PlasmaClient::Seal instead of crashing
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 77a5c59 ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing
77a5c59 is described below
commit 77a5c596b80d50dcc7d2486cd8875e1649199d89
Author: Krisztián Szűcs <sz...@gmail.com>
AuthorDate: Mon Apr 23 18:08:17 2018 +0200
ARROW-2494: [C++] Return status codes from PlasmaClient::Seal instead of crashing
Author: Krisztián Szűcs <sz...@gmail.com>
Closes #1932 from kszucs/ARROW-2494 and squashes the following commits:
699b11a <Krisztián Szűcs> fix syntax of error messages
8b41bca <Krisztián Szűcs> use boolean assertions
0c13ecd <Krisztián Szűcs> return status codes from PlasmaClient::Seal
---
cpp/src/arrow/status.h | 11 +++++++++-
cpp/src/plasma/client.cc | 13 +++++++----
cpp/src/plasma/test/client_tests.cc | 43 +++++++++++++++++++++++++------------
3 files changed, 48 insertions(+), 19 deletions(-)
diff --git a/cpp/src/arrow/status.h b/cpp/src/arrow/status.h
index ed524ae..8b82e2a 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 0d44b11..015c973 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 10e4e4f..dad7688 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<uint8_t> 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, &has_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<ObjectBuffer> object_buffers;
ARROW_CHECK_OK(client_.Get({object_id}, -1, &object_buffers));
ARROW_CHECK_OK(client_.Contains(object_id, &has_object));
- ASSERT_EQ(has_object, true);
+ ASSERT_TRUE(has_object);
}
TEST_F(TestPlasmaStore, GetTest) {
@@ -277,7 +292,7 @@ TEST_F(TestPlasmaStore, MultipleClientTest) {
// Test for object non-existence on the first client.
bool has_object;
ARROW_CHECK_OK(client_.Contains(object_id, &has_object));
- ASSERT_EQ(has_object, false);
+ ASSERT_FALSE(has_object);
// Test for the object being in local Plasma store.
// First create and seal object on the second client.
@@ -291,7 +306,7 @@ TEST_F(TestPlasmaStore, MultipleClientTest) {
ARROW_CHECK_OK(client_.Get({object_id}, -1, &object_buffers));
ASSERT_TRUE(object_buffers[0].data);
ARROW_CHECK_OK(client_.Contains(object_id, &has_object));
- ASSERT_EQ(has_object, true);
+ ASSERT_TRUE(has_object);
// Test that one client disconnecting does not interfere with the other.
// First create object on the second client.
@@ -304,7 +319,7 @@ TEST_F(TestPlasmaStore, MultipleClientTest) {
ARROW_CHECK_OK(client2_.Get({object_id}, -1, &object_buffers));
ASSERT_TRUE(object_buffers[0].data);
ARROW_CHECK_OK(client2_.Contains(object_id, &has_object));
- ASSERT_EQ(has_object, true);
+ ASSERT_TRUE(has_object);
}
TEST_F(TestPlasmaStore, ManyObjectTest) {
@@ -318,7 +333,7 @@ TEST_F(TestPlasmaStore, ManyObjectTest) {
// Test for object non-existence on the first client.
bool has_object;
ARROW_CHECK_OK(client_.Contains(object_id, &has_object));
- ASSERT_EQ(has_object, false);
+ ASSERT_FALSE(has_object);
// Test for the object being in local Plasma store.
// First create and seal object on the first client.
@@ -333,7 +348,7 @@ TEST_F(TestPlasmaStore, ManyObjectTest) {
ARROW_CHECK_OK(client_.Seal(object_id));
// Test that the first client can get the object.
ARROW_CHECK_OK(client_.Contains(object_id, &has_object));
- ASSERT_EQ(has_object, true);
+ ASSERT_TRUE(has_object);
} else if (i % 3 == 1) {
// Abort one third of the objects.
ARROW_CHECK_OK(client_.Release(object_id));
@@ -351,10 +366,10 @@ TEST_F(TestPlasmaStore, ManyObjectTest) {
ARROW_CHECK_OK(client2_.Contains(object_id, &has_object));
if (i % 3 == 0) {
// The first third should be sealed.
- ASSERT_EQ(has_object, true);
+ ASSERT_TRUE(has_object);
} else {
// The rest were aborted, so the object is not in the store.
- ASSERT_EQ(has_object, false);
+ ASSERT_FALSE(has_object);
}
i++;
}
@@ -429,7 +444,7 @@ TEST_F(TestPlasmaStore, MultipleClientGPUTest) {
// Test for object non-existence on the first client.
bool has_object;
ARROW_CHECK_OK(client_.Contains(object_id, &has_object));
- ASSERT_EQ(has_object, false);
+ ASSERT_FALSE(has_object);
// Test for the object being in local Plasma store.
// First create and seal object on the second client.
@@ -443,7 +458,7 @@ TEST_F(TestPlasmaStore, MultipleClientGPUTest) {
// Test that the first client can get the object.
ARROW_CHECK_OK(client_.Get({object_id}, -1, &object_buffers));
ARROW_CHECK_OK(client_.Contains(object_id, &has_object));
- ASSERT_EQ(has_object, true);
+ ASSERT_TRUE(has_object);
// Test that one client disconnecting does not interfere with the other.
// First create object on the second client.
@@ -456,7 +471,7 @@ TEST_F(TestPlasmaStore, MultipleClientGPUTest) {
ARROW_CHECK_OK(client2_.Seal(object_id));
object_buffers.clear();
ARROW_CHECK_OK(client2_.Contains(object_id, &has_object));
- ASSERT_EQ(has_object, true);
+ ASSERT_TRUE(has_object);
ARROW_CHECK_OK(client2_.Get({object_id}, -1, &object_buffers));
ASSERT_EQ(object_buffers.size(), 1);
ASSERT_EQ(object_buffers[0].device_num, 1);
--
To stop receiving notification emails like this one, please contact
apitrou@apache.org.