Skip to content

Conversation

@Yeolar
Copy link
Contributor

@Yeolar Yeolar commented Jul 23, 2017

No description provided.

@pcmoritz
Copy link
Contributor

+1 for the change

@wesm
Copy link
Member

wesm commented Jul 23, 2017

Could this error have caused any problems? Any way to test for it (e.g. a DCHECK someplace?)?

@pcmoritz
Copy link
Contributor

Good question! The method is tested here:
https://github.com/apache/arrow/blob/master/cpp/src/plasma/test/serialization_tests.cc#L176

What we could do is injecting flatbuffer verifier calls in all the methods in plasma_protocol (only in debug mode); I don't think flatbuffers::GetRoot has a way of detecting that the wrong message was read. What do you think?

@Yeolar
Copy link
Contributor Author

Yeolar commented Jul 24, 2017

PlasmaSealRequest covers PlasmaReleaseRequest, in plasma.fbs. (For "object_id: string;" is also the first field in PlasmaSealRequest.) I think that's why no error occured, flatbuffers is just casting types from void*. Agree with @pcmoritz, maybe can use the Verify method for checking in test.

@pcmoritz
Copy link
Contributor

pcmoritz commented Jul 24, 2017

Ok, I created a JIRA here: https://issues.apache.org/jira/browse/ARROW-1255

@Yeolar: Do you want to tackle it as part of this PR? We can probably do this with minimal code duplication (and effort) by making a new templated method that replaces flatbuffers::GetRoot and checks with the verifier. Maybe such a method already exists in flatbuffers.

@Yeolar
Copy link
Contributor Author

Yeolar commented Jul 25, 2017

@pcmoritz , I've submitted a new PR #887, please have a look if it's apposite. By the way, when compiling arrow, it seems plasma not compiled, how to include?

@pcmoritz
Copy link
Contributor

Thanks, I'll have a look! Here is a draft of the documentation on how to make Plasma work: https://github.com/pcmoritz/arrow/blob/724fd2f4dbd2655bcd33d8e672198fd1b704c75a/python/doc/source/plasma.rst

The gist of it is that you need to pass -DARROW_PLASMA=on into CMake when compiling the C++ part of arrow and then set the environment variable PYARROW_WITH_PLASMA=1 when running setup.py.

The documentation is work in progress (see #881), let us know if you have ideas on how to improve it. The Python documentation should already work, the C++ one I need to fix first, I'll do that in the next couple of days.

@Yeolar Yeolar closed this Jul 26, 2017
asfgit pushed a commit that referenced this pull request Jul 26, 2017
…XXX in plasma protocol.

Related to #878, add DCHECK for ReadXXX.

Author: Yeolar <[email protected]>

Closes #887 from Yeolar/fixtypo_plasma_and_add_DCHECK and squashes the following commits:

4df63bc [Yeolar] clang-format for too long lines.
143d254 [Yeolar] Update, compile passed.
09ff103 [Yeolar] Fix conflicts.
b951d8d [Yeolar] Merge pull request #1 from apache/master
ebae611 [Yeolar] Fix typo in plasma protocol; add DCHECK for ReadXXX in plasma protocol.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants