Make the Plasma store ready for Arrow integration #579

pcmoritz · 2017-05-21T00:11:26Z

This PR disentangles the plasma store from the rest of Ray and prepares the merge into Apache Arrow.

Going forward there are two possibilities: Put the store into arrow and then create a new PR that integrates it into Ray again, or get this PR into a mergeable state, merge it into Ray and then put the store into arrow. Any opinions on this? [Right now there is a bunch of code duplication that will go away]

Note this makes a bunch of progress on #406.

AmplabJenkins · 2017-05-28T01:49:41Z

Build finished. Test PASSed.

AmplabJenkins · 2017-05-28T01:49:42Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/903/
Test PASSed.

AmplabJenkins · 2017-05-28T02:04:31Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-28T02:04:31Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/904/
Test PASSed.

AmplabJenkins · 2017-05-28T02:44:31Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-28T02:44:31Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/905/
Test PASSed.

robertnishihara · 2017-05-30T04:20:41Z

src/plasma/status.h

+  bool IsTypeError() const { return code() == StatusCode::TypeError; }
+  bool IsUnknownError() const { return code() == StatusCode::UnknownError; }
+  bool IsNotImplemented() const { return code() == StatusCode::NotImplemented; }
+  bool IsPlasmaObjectExists() const { return code() == StatusCode::PlasmaObjectExists; }


What exactly are the semantics of IsPlasmaObjectExists and IsPlasmaStoreFull? I suppose they just return whatever the last status code that they got from the store, right?

This is a very minor point, but it is possible to add a large object, then IsPlasmaStoreFull will return true. Then add a small object and IsPlasmaStoreFull will return false. Also, it is possible for the store to be literally full, but for `IsPlasmaStoreFull to return false. I guess all I'm saying is that the naming is a little unintuitive.

Note, I'm not actually suggesting that you change the name, maybe add a comment though.

robertnishihara · 2017-05-30T04:35:09Z

src/plasma/plasma_io.h

+
+#include "status.h"
+
+#define RAY_PROTOCOL_VERSION 0x0000000000000000


Do we want to replace this with PLASMA_PROTOCOL_VERSION?

robertnishihara · 2017-05-30T04:38:50Z

src/plasma/plasma_common.h

+  }
+};
+
+typedef UniqueID ObjectID;


I'm so happy to finally make these classes (it's especially great to have the hash method). If we go ahead and make ObjectID a subclass would that break anything? It'd be nice to get rid of the typedef.

It doesn't work right now but can be made to work; I'd like to merge the PR first and do it as part of a subsequent PR, because I'm also working on the arrow integration PR right now and would like to keep the changes that are needed to backport minimal.

robertnishihara · 2017-05-30T04:39:39Z

src/plasma/plasma_common.h

+  const uint8_t *data() const;
+  uint8_t *mutable_data();
+  std::string binary() const;
+  std::string sha1() const;


What about a more generic name like hash or hex? Some of these IDs are just generated randomly and it's possible we'd change the hash function later.

robertnishihara · 2017-05-30T04:42:00Z

src/plasma/status.h

@@ -0,0 +1,210 @@
+// Copyright (c) 2011 The LevelDB Authors. All rights reserved.


I don't know if it matters, but some of this code does not come from the LevelDB authors.

This code is actually from arrow (which presumably took it from leveldb) and will be removed from our codebase after the arrow merge

Right, but the copyright seems inaccurate regardless of which codebase it is in, right?

AmplabJenkins · 2017-05-30T22:44:30Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-30T22:44:31Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/911/
Test PASSed.

AmplabJenkins · 2017-05-30T23:49:42Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-30T23:49:42Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/912/
Test PASSed.

AmplabJenkins · 2017-05-31T00:10:22Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-05-31T00:10:23Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/913/
Test FAILed.

AmplabJenkins · 2017-05-31T03:59:54Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-31T03:59:54Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/914/
Test PASSed.

AmplabJenkins · 2017-05-31T05:14:45Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-31T05:14:46Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/915/
Test PASSed.

robertnishihara

Again, this looks great, I left a few comments! At a high level, the code for the manager and store seem to be very tightly coupled. What is your plan for decoupling them?

robertnishihara · 2017-05-31T18:04:28Z

src/numbuf/CMakeLists.txt

@@ -45,7 +45,6 @@ if(HAS_PLASMA)
  include_directories("${CMAKE_CURRENT_LIST_DIR}/../plasma")
  include_directories("${CMAKE_CURRENT_LIST_DIR}/../common")
  include_directories("${CMAKE_CURRENT_LIST_DIR}/../common/thirdparty")
-  set(COMMON_EXTENSION ../common/lib/python/common_extension.cc)


Can you explain this change, note that we still use the variable COMMON_EXTENSION later in this file.

robertnishihara · 2017-05-31T18:05:52Z

src/numbuf/python/src/pynumbuf/numbuf.cc

 #ifdef HAS_PLASMA
+#include "plasma_common.h"
+


there's no reason to have a space here, right?

If the space is not there, clang-format tries to reorder the includes to sort them alphabetically, but this include needs to come first, so I think we do need the space here.

robertnishihara · 2017-05-31T18:07:56Z

src/plasma/CMakeLists.txt


 add_dependencies(plasma protocol_fbs)

-add_executable(plasma_manager
-  plasma_manager.cc)
+ add_executable(plasma_manager


Do we actually want these extra spaces?

robertnishihara · 2017-05-31T18:08:57Z

src/plasma/plasma_manager.cc

+    return 0;
+  }
+  if (errno == EPIPE || errno == EBADF || errno == ECONNRESET) {
+    ARROW_LOG(WARNING)


Does it really make sense to convert the plasma manager to use arrow macros for logging? Are you planning on moving plasma_manager.cc into Arrow? Other than for checking status codes for the manager's interaction with the store?

I'd like to use ARROW_LOG like macros throughout the codebase eventually. We can copy logging.h over and rename it to RAY_LOG.

robertnishihara · 2017-05-31T18:14:01Z

src/plasma/plasma_store.h

+  ///           present in the store. In this case, the client should not call
+  ///           plasma_release.
+  ///         - PlasmaError_OutOfMemory, if the store is out of memory and
+  ///         cannot


Fix indentation here.

robertnishihara · 2017-05-31T18:15:19Z

src/plasma/status.h

@@ -0,0 +1,210 @@
+// Copyright (c) 2011 The LevelDB Authors. All rights reserved.


Right, but the copyright seems inaccurate regardless of which codebase it is in, right?

robertnishihara · 2017-05-31T18:20:40Z

test/component_failures_test.py

@@ -163,7 +163,7 @@ def check_components_alive(self, component_type, check_component_alive):
      if check_component_alive:
        self.assertTrue(component.poll() is None)
      else:
-        self.assertTrue(component.poll() <= 0)
+        self.assertTrue(not component.poll() is None)


Can you explain this change?

The plasma store now returns 1 if an error occurs (before that the status was negative). This is because of the arrow status checking macros.

AmplabJenkins · 2017-05-31T22:15:31Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-31T22:15:32Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/917/
Test PASSed.

AmplabJenkins · 2017-05-31T22:20:30Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-31T22:20:31Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/918/
Test PASSed.

robertnishihara · 2017-05-31T23:36:28Z

src/plasma/plasma_common.cc

+  return std::memcmp(data(), rhs.data(), kUniqueIDSize) == 0;
+}
+
+Status plasma_error_status(int plasma_error) {


I just noticed, but this gives a compiler warning on MacOS.

/ray/src/plasma/plasma_manager.cc:59:1: warning: control may reach end of non-void function [-Wreturn-type]

pcmoritz force-pushed the arrow-2 branch 7 times, most recently from 206b928 to 9604673 Compare May 23, 2017 16:33

pcmoritz added 13 commits May 27, 2017 18:43

port plasma to arrow

9b59046

fixes

5033a16

refactor plasma client

eb9829c

more modernization

d0ab043

fix plasma manager tests

249989d

everything compiles

7618af6

fix plasma client tests

e417a53

update plasma serialization tests

df5e0ab

fix plasma manager tests

9e01842

fix bug

127018e

updates

a53eec6

fix bug

b123050

fix tests

c27d3dd

pcmoritz force-pushed the arrow-2 branch from 776a458 to c27d3dd Compare May 28, 2017 01:50

fix rebase

c30bac9

pcmoritz force-pushed the arrow-2 branch from 4f19a93 to c30bac9 Compare May 28, 2017 02:30

robertnishihara reviewed May 30, 2017

View reviewed changes

address comments

f525f68

fix travis valgrind build

e18c30e

fix linting

004b4cb

fix include order again

19d0bf0

fix linting

4e69d3a

pcmoritz changed the title ~~[WIP] Make the Plasma store ready for Arrow integration~~ Make the Plasma store ready for Arrow integration May 31, 2017

robertnishihara reviewed May 31, 2017

View reviewed changes

robertnishihara mentioned this pull request May 31, 2017

Move plasma object store into Arrow. #611

Closed

address comments

cee6ac0

pcmoritz force-pushed the arrow-2 branch from b55aa58 to cee6ac0 Compare May 31, 2017 22:07

robertnishihara merged commit b94b4a3 into ray-project:master May 31, 2017

robertnishihara deleted the arrow-2 branch May 31, 2017 23:24

robertnishihara reviewed May 31, 2017

View reviewed changes

This was referenced Jun 1, 2017

Compilation error on Amazon Linux AMI. #463

Closed

Compiler warning on MacOS in plasma_protocol.cc. #621

Closed

robertnishihara mentioned this pull request Jun 15, 2017

Fatal logging statements in plasma store no longer print backtraces. #672

Closed


		#include "status.h"

		#define RAY_PROTOCOL_VERSION 0x0000000000000000

		@@ -0,0 +1,210 @@
		// Copyright (c) 2011 The LevelDB Authors. All rights reserved.

Make the Plasma store ready for Arrow integration #579

Make the Plasma store ready for Arrow integration #579

Conversation

pcmoritz commented May 21, 2017 • edited by robertnishihara Loading

AmplabJenkins commented May 28, 2017

AmplabJenkins commented May 28, 2017

AmplabJenkins commented May 28, 2017

AmplabJenkins commented May 28, 2017

AmplabJenkins commented May 28, 2017

AmplabJenkins commented May 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented May 30, 2017

AmplabJenkins commented May 30, 2017

AmplabJenkins commented May 30, 2017

AmplabJenkins commented May 30, 2017

AmplabJenkins commented May 31, 2017

AmplabJenkins commented May 31, 2017

AmplabJenkins commented May 31, 2017

AmplabJenkins commented May 31, 2017

AmplabJenkins commented May 31, 2017

AmplabJenkins commented May 31, 2017

robertnishihara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented May 31, 2017

AmplabJenkins commented May 31, 2017

AmplabJenkins commented May 31, 2017

AmplabJenkins commented May 31, 2017

Choose a reason for hiding this comment

pcmoritz commented May 21, 2017 •

edited by robertnishihara

Loading