ARROW-4418 [Plasma] replace event loop with boost::asio for plasma store #3704

suquark · 2019-02-19T14:58:12Z

This is replacing the socket based IO with boost asio so plasma can be ported to windows.

pcmoritz · 2019-02-19T22:47:00Z

@suquark Can you remove the asio commit? We don't want to commit asio into here, let me know if you need help to integrate asio into the build system!

suquark · 2019-02-20T02:45:06Z

@pcmoritz Yes, I would like to know how to integrate asio. Should we download the source when building plasma?

suquark · 2019-02-20T03:44:00Z

I have rebased the branch to resolve conflicts.

zhijunfu · 2019-02-21T03:26:23Z

@suquark Just FYI. If I understand correctly boost::asio::local::stream_protocol is only supported on POSIX systems, on windows you might need boost::asio::windows::stream_handle or local TCP? Also on linux it's reported that AF_UNIX is has better performance over tcp, thus it might not be very desirable to use local TCP on all platforms.

suquark · 2019-03-10T11:45:40Z

@robertnishihara @pcmoritz It is ready for review. The CI failure comes from the master branch.

suquark · 2019-03-10T11:50:24Z

I have removed most unix-only headers for cross-platform compiling, but we still cannot build it on Windows because the shared memory operation is not windows-compatible. I would like to fix this in a following PR.

pcmoritz · 2019-03-11T02:06:30Z

cpp/src/plasma/CMakeLists.txt

This should be formatted more like the following to be consistent with ThirdpartyToolchain.cmake. Maybe even consider to add it there?

ExternalProject_Add(asio_ep URL https://github.com/chriskohlhoff/asio/archive/asio-1-12-2.zip CONFIGURE_COMMAND "" # No autogen since we use asio in header-only way. BUILD_IN_SOURCE 1 BUILD_COMMAND "" INSTALL_COMMAND cmake -E copy asio/include/asio.hpp ${OUTPUT_DIR}/.. && cmake -E copy_directory asio/include/asio ${OUTPUT_DIR}/../asio/)

It was formatted by the cmake format checker. I can move it into ThirdpartyToolchain.cmake to keep the currect format.

Can you hang on until the CMake refactor is merged to make sure this doesn't conflict (easier to fix this PR than to fix that)?

Sure. Which PR is about CMake refactor?

pcmoritz · 2019-03-11T02:17:48Z

cpp/src/plasma/io/basic_connection.cc

This is not currently used, right? Let's remove it!

I created this because we can use it under windows. Windows do not support unix domain sockets, so we can use tcp socket instead (or we can use UDP, since it would be more efficient).
And do you know how to build it with the CI under windows?

pcmoritz · 2019-03-11T02:20:55Z

cpp/src/plasma/io/plasma_connection.cc

An if condition would be more idiomatic here I think

pcmoritz · 2019-03-11T02:22:08Z

cpp/src/plasma/io/basic_connection.h

Let's rename this to PlasmaStream (UPPER_CASE should be reserved for macros)

pcmoritz · 2019-03-11T02:22:30Z

cpp/src/plasma/io/basic_connection.h

Let's rename this to PlasmaAcceptor

cpp/src/plasma/store.h

cpp/src/plasma/store.cc

cpp/src/plasma/io/client_connection.h

cpp/src/plasma/io/plasma_connection.cc

cpp/src/plasma/io/plasma_connection.h

cpp/src/plasma/io/basic_connection.h

robertnishihara · 2019-03-11T18:22:12Z

@zhijunfu would you be interested in reviewing this PR?

…elegate class. This provides less "pluggability" but I think still offers a clean model for extension (subsystems can wrap the constructor for there purposes, and provide external static methods to check for particular types of errors). Author: Micah Kornfield <emkornfield@gmail.com> Author: Antoine Pitrou <antoine@python.org> Closes apache#4484 from emkornfield/status_code_proposal and squashes the following commits: 4d1ab8d <Micah Kornfield> don't import plasma errors directly into top level pyarrow module a66f999 <Micah Kornfield> make format 040216d <Micah Kornfield> fixes for comments outside python 729bba1 <Antoine Pitrou> Fix Py2 issues (hopefully) ea56d1e <Antoine Pitrou> Fix PythonErrorDetail to store Python error state (and restore it in check_status()) 21e1b95 <Micah Kornfield> fix compilation 9c905b0 <Micah Kornfield> fix lint 74d563c <Micah Kornfield> fixes 85786ef <Micah Kornfield> change messages 3626a90 <Micah Kornfield> try removing message a4e6a1f <Micah Kornfield> add logging for debug 4586fd1 <Micah Kornfield> fix typo 8f011b3 <Micah Kornfield> fix status propagation 317ea9c <Micah Kornfield> fix complie 9f59160 <Micah Kornfield> don't make_shared inline 484b3a2 <Micah Kornfield> style fix 14e3467 <Micah Kornfield> dont rely on rtti cd22df6 <Micah Kornfield> format dec4585 <Micah Kornfield> not-quite pluggable error codes

suquark · 2019-07-15T06:44:49Z

@pcmoritz I have seen you removing the independent asio headers. Why doing so?

pcmoritz · 2019-07-15T07:35:44Z

@suquark This way it is easier to integrate with Ray's build system (we already have boost as a dependency in both arrow's and Ray's build system so it seems logical to use them). I did it with minimal modification so it's easy to change, but I don't see a clear advantage of using the standalone one right now, do you? :)

suquark · 2019-07-15T12:20:57Z

@pcmoritz Oh, that makes sense then. Thx for doing that.

emkornfield · 2019-08-29T05:25:58Z

What are the next steps with this PR?

suquark · 2019-08-29T22:50:26Z

@emkornfield I am going to fix these conflicts, and hopefully we can get it merged in the next few weeks.

bkietz

@suquark this looks like a good start on an ambitious change. Could you rebase please?

bkietz · 2019-09-11T16:44:07Z

cpp/src/plasma/io/basic_connection.h

+
+#include "arrow/status.h"
+
+namespace asio = boost::asio;


Global namespace aliases in non-internal headers are against our coding style. Please either remove these aliases or confine them to the plasma::io namespace

bkietz · 2019-09-11T16:44:54Z

LICENSE.txt


 --------------------------------------------------------------------------------

-This project includes code from the Boost project


why is this notation necessary?

bkietz · 2019-09-11T16:48:07Z

cpp/src/plasma/io/connection.h

+// under the License.
+
+#ifndef PLASMA_IO_CONNECTION_H
+#define PLASMA_IO_CONNECTION_H


Suggested change

#define PLASMA_IO_CONNECTION_H

#pragma once

bkietz · 2019-09-11T16:58:03Z

cpp/src/plasma/io/basic_connection.cc

+}
+
+// We have to fill the template of all possible types.
+template class Connection<PlasmaStream>;


An extern template like this is extremely difficult to get working under MSVC, and you only instantiate this template once anyway. Why is this a template instead of just defining PlasmaConnection directly in connection.h?

bkietz · 2019-09-11T16:59:33Z

cpp/src/plasma/store.h

+  /// Disconnect a client from the PlasmaStore.
+  ///
+  /// \param client The client that is disconnected.
+  void ProcessDisconnectClient(const std::shared_ptr<ClientConnection>& client);


Why the Process prefix?

bkietz · 2019-09-11T17:03:50Z

dev/release/verify-apt.sh

 if [ "${have_gandiva}" = "yes" ]; then
  apt install -y -V libgandiva-glib-dev=${deb_version}
-  apt install -y -V libgandiva-glib-doc=${deb_version}
+  # apt install -y -V libgandiva-glib-doc=${deb_version}


what's this change about?

bkietz · 2019-09-11T17:07:53Z

cpp/src/plasma/store.cc


 struct GetRequest {
-  GetRequest(Client* client, const std::vector<ObjectID>& object_ids);
+  GetRequest(asio::io_context& io_context,


Although it's not the asio pattern, it may be necessary to pass these around by pointer to appease cpplint

bkietz · 2019-09-11T17:21:14Z

cpp/src/plasma/store.cc

+    }
+
+    // Send the get reply to the client.
+    Status s = SendGetReply(client, &object_ids[0], objects, object_ids.size(), store_fds,


Nit: this looks like it could segfault if object_ids is empty. Could be made more clear with:

Suggested change

Status s = SendGetReply(client, &object_ids[0], objects, object_ids.size(), store_fds,

Status s = SendGetReply(client, object_ids.data(), objects, object_ids.size(), store_fds,

emkornfield · 2019-10-16T05:59:52Z

@suquark do you think you will have time to rebase and address the feedback?

emkornfield · 2019-10-24T03:50:07Z

@suquark thank you for the PR if you have time to update it, please reopen. closing for now do to lack of response.

suquark · 2019-10-25T18:35:43Z

Sorry for my late response. Let me try to address these issues.

suquark force-pushed the plasma branch from d05d986 to 0f4dd05 Compare February 19, 2019 15:03

suquark force-pushed the plasma branch 2 times, most recently from a45202c to 37fb5d2 Compare February 20, 2019 03:42

suquark force-pushed the plasma branch 2 times, most recently from 9967441 to 14b25ea Compare February 22, 2019 10:38

suquark force-pushed the plasma branch 5 times, most recently from 565ceda to 10f28fd Compare March 7, 2019 14:30

pcmoritz reviewed Mar 11, 2019

View reviewed changes

cpp/src/plasma/io/plasma_connection.cc Outdated

Copy link

Contributor

pcmoritz Mar 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An if condition would be more idiomatic here I think

pcmoritz reviewed Mar 11, 2019

View reviewed changes

cpp/src/plasma/io/basic_connection.h Outdated

Copy link

Contributor

pcmoritz Mar 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to PlasmaStream (UPPER_CASE should be reserved for macros)

pcmoritz reviewed Mar 11, 2019

View reviewed changes

cpp/src/plasma/io/basic_connection.h Outdated

Copy link

Contributor

pcmoritz Mar 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to PlasmaAcceptor

pcmoritz reviewed Mar 11, 2019

View reviewed changes

cpp/src/plasma/store.h Outdated Show resolved Hide resolved

pcmoritz reviewed Mar 11, 2019

View reviewed changes

cpp/src/plasma/store.h Outdated Show resolved Hide resolved

pcmoritz reviewed Mar 11, 2019

View reviewed changes

cpp/src/plasma/store.cc Outdated Show resolved Hide resolved

robertnishihara reviewed Mar 11, 2019

View reviewed changes

cpp/src/plasma/io/client_connection.h Outdated Show resolved Hide resolved

robertnishihara reviewed Mar 11, 2019

View reviewed changes

cpp/src/plasma/io/plasma_connection.cc Outdated Show resolved Hide resolved

robertnishihara reviewed Mar 11, 2019

View reviewed changes

cpp/src/plasma/io/plasma_connection.h Outdated Show resolved Hide resolved

pcmoritz reviewed Mar 11, 2019

View reviewed changes

cpp/src/plasma/io/basic_connection.h Outdated Show resolved Hide resolved

suquark force-pushed the plasma branch from 656ee9e to 8e2a48b Compare March 14, 2019 14:12

emkornfield and others added 4 commits July 3, 2019 12:14

[Website] Update Fletcher link and description (apache#4794)

d030749

Merge branch 'master' into plasma

533ef53

fix merge

4e099a8

kou force-pushed the master branch from e3ae1b0 to 03576af Compare July 4, 2019 05:51

pcmoritz added 11 commits July 5, 2019 17:59

Merge branch 'master' into plasma

21c945c

fix

c8bbf05

update

d60f69f

update

19f0a35

update

fd3a73d

update

b6d7461

fix

df06510

update

fb0e411

fix

57bfd64

update

4cad602

update

11871a2

flrs mentioned this pull request Jul 19, 2019

ModuleNotFoundError: No module named 'pyarrow._plasma' nalepae/pandarallel#15

Closed

kszucs force-pushed the master branch 2 times, most recently from ed180da to 85fe336 Compare July 22, 2019 19:29

bkietz requested changes Sep 11, 2019

View reviewed changes

emkornfield closed this Oct 24, 2019

suquark mentioned this pull request Oct 25, 2019

[WIP] ARROW-4418 [Plasma] replace event loop with boost::asio for plasma store #5736

Closed


		--------------------------------------------------------------------------------

		This project includes code from the Boost project

	Status s = SendGetReply(client, &object_ids[0], objects, object_ids.size(), store_fds,
	Status s = SendGetReply(client, object_ids.data(), objects, object_ids.size(), store_fds,

ARROW-4418 [Plasma] replace event loop with boost::asio for plasma store #3704

ARROW-4418 [Plasma] replace event loop with boost::asio for plasma store #3704

Uh oh!

Conversation

suquark commented Feb 19, 2019 • edited by pcmoritz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pcmoritz commented Feb 19, 2019

Uh oh!

suquark commented Feb 20, 2019

Uh oh!

suquark commented Feb 20, 2019

Uh oh!

zhijunfu commented Feb 21, 2019

Uh oh!

suquark commented Mar 10, 2019

Uh oh!

suquark commented Mar 10, 2019

Uh oh!

pcmoritz Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcmoritz Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertnishihara commented Mar 11, 2019

Uh oh!

suquark commented Jul 15, 2019

Uh oh!

pcmoritz commented Jul 15, 2019

Uh oh!

suquark commented Jul 15, 2019

Uh oh!

emkornfield commented Aug 29, 2019

Uh oh!

suquark commented Aug 29, 2019

Uh oh!

bkietz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suquark commented Feb 19, 2019 •

edited by pcmoritz

Loading

pcmoritz Mar 11, 2019 •

edited

Loading

pcmoritz Mar 11, 2019 •

edited

Loading

bkietz left a comment •

edited

Loading