[
https://issues.apache.org/jira/browse/ARROW-2494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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<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);
----------------------------------------------------------------
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
> Fix For: 0.10.0
>
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
