ARROW-1163: [Java] Java client support for plasma #2065

salah-man · 2018-05-18T05:44:18Z

This commit includes the support of java client for plasma, which is part of the java worker support of Ray.
In addition to some minor changes in build system, it consists of the following modules:

java/plasma: java client support for plasma
cpp/src/plasma/lib/java: JNI support for plasma client
java/plasma/Readme.md: Readme.md for help document
java/plasma/test.sh: test.sh for test

-------------------------- This commit includes the support of java client for plasma, which is part of the java worker support of Ray. In addition to some minor changes in build system, it consists of the following modules: - java/plasma: java client support for plasma - cpp/src/plasma/lib/java: JNI support for plasma client

robertnishihara · 2018-05-18T07:08:50Z

Related to ray-project/ray#2095.

jacques-n · 2018-05-18T15:37:45Z

I think this should integrate better with other Arrow Java APIs. Right now, the apis here don't interact with ValueVector nor ArrowBuf, the key components of the Java API. We should make sure to integrate at that level so that our pieces work well together.

jacques-n · 2018-05-18T15:38:38Z

Also, would be good to have a jira ticket tracking this discussion/approach.

pcmoritz

Thanks for the contribution! This looks really great :)

Before it can be merged, we should have a few things:

Fix all the linting/test errors on Travis (e.g. org_apache_arrow_plasma_PlasmaClientJNI.cc needs a license header, <jni.h> is not found in the C++ build, so we need to install it, there is probably some linting error to be fixed once the license is there
Add some tests to make sure the Java PlasmaClient is working
Provide instructions on how to set it up and compile it for developers

I think the things @jacques-n brings up can be addressed in a follow-up PR, let's make sure the low-level APIs are working well in this PR.

salah-man · 2018-05-20T00:21:56Z

Thanks for your attention @jacques-n. I think the comment from @jacques-n is great, and the java client of plasma better not depend on the DomainDO itself such as ObjectBuffer and ObjectId. And the scenes here is simple, so i just delete the data structure of ObjectBuffer and ObjectId and use the byte[] for the input and output of the api. The code is ready and will commit with other changes comment by @pcmoritz later.

salah-man · 2018-05-20T00:29:43Z

Thank you for your time @pcmoritz. The comment from you really helpful.

I will fix all the linting/test errors on Travis.
I think it is really needed for the test case, and for the java client of plasma, maybe we need to start the standalone process of plasma store to do the test which i will take a moment to try. And do you have any suggestion here?
I will add the Readme.md under the package of plasma for the usage guide

pcmoritz · 2018-05-20T01:05:27Z

Thanks, that sounds good!

For 2, we can do the setup similar to how we do it in C++ (see https://github.com/apache/arrow/blob/master/cpp/src/plasma/test/client_tests.cc#L61) and start an instance of the store from the Java tests (and shut it down after the tests are done).

jacques-n · 2018-05-20T01:16:23Z

The public interface (which is fine in a separate patch), should be ValueVector & ArrowBuf based, not byte[] based. Before releasing this functionality we should be making sure that it works in the context of the rest of the Arrow Java code.

robertnishihara · 2018-05-20T06:00:43Z

In Python, we expose two different object store client APIs.

An API for creating/sealing/getting a writeable buffer (I think it's called PlasmaBuffer).
An API for storing/retrieving a Python object.

In Java, I think it makes sense to have analogs of these. The more important one for now is the first one, which could be implemented using Arrow buffers (but the stored data would be blobs and not necessarily need to be data serialized using the Arrow format). The second would use the Arrow format under the hood.

salah-man · 2018-05-20T12:28:21Z

Yes, i think it is make sense to hava analogs of these the same as python, and we could try to use the Arrow buffers to instead the byte[] in the follow-up PR, which could make sure that it works in the context of the rest of the Arrow Java code. Thank you for your time again, @pcmoritz @jacques-n @robertnishihara

…output of plasma java client api from custem type to byte[]

-------------------------- It consists of the following changes: add java/plasma/README.md for help document add java/plasma/test.sh for plasma java client test case fix some ci error delete the useless parameters of connect api from jni

codecov-io · 2018-05-22T02:33:20Z

Codecov Report

Merging #2065 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2065      +/-   ##
==========================================
- Coverage   86.33%   86.33%   -0.01%     
==========================================
  Files         242      242              
  Lines       41103    41103              
==========================================
- Hits        35487    35486       -1     
- Misses       5616     5617       +1

Impacted Files	Coverage Δ
cpp/src/arrow/util/thread-pool-test.cc	`98.87% <0%> (-0.57%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d9d893...be067e8. Read the comment docs.

salah-man · 2018-05-22T06:43:02Z

As comment, we just fix the problem you have suggest, could you review them again? Thank you very much, @pcmoritz

pcmoritz · 2018-05-23T03:20:53Z

java/plasma/test.sh

@@ -35,7 +36,7 @@ pushd ../../cpp
            -DCMAKE_CXX_FLAGS="-g -O3" \
            -DARROW_BUILD_TESTS=off \
            -DARROW_HDFS=on \
-            -DARROW_BOOST_USE_SHARED=off \
+            -DARROW_BOOST_USE_SHARED=on \


I had to do this to make it compile with Ubuntu 16.04, since the boost static libraries that ship with it are compiled without -fPIC.

pcmoritz · 2018-05-23T03:21:52Z

cpp/src/plasma/CMakeLists.txt

+    ${PLASMA_LIBRARY_EXT_java_SRC})
+
+  if(APPLE)
+    target_link_libraries(plasma_java plasma_static ${PLASMA_LINK_LIBS} "-undefined dynamic_lookup" -Wl,-force_load,${FLATBUFFERS_STATIC_LIB} ${FLATBUFFERS_STATIC_LIB} ${PTHREAD_LIBRARY})


Why do we need the -Wl,-force_load, part here?

-Wl,-force_load for add runtime library path here

pcmoritz · 2018-05-23T03:23:40Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one


Let's add this file to the C++ linting/clang-tidy

I think this file has already in the C++ linting process now, because i just fix many linting errors which is feedback by the ci.

pcmoritz · 2018-05-23T03:24:38Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+
+#include "plasma/client.h"
+
+const jsize LEN_OF_OBJECTID = sizeof(plasma::ObjectID) / sizeof(jbyte);


should this be a constexpr?

yes, it will not be changed, and worked as constant

Thanks! Also, naming it OBJECT_ID_SIZE would be more consistent with the rest of the code (but up to you, since it's private in this file)

Thanks, i could rename it by the way.

constexpr is preferable (since the compiler can optimize more)

pcmoritz · 2018-05-23T03:25:44Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+
+JNIEXPORT void JNICALL Java_org_apache_arrow_plasma_PlasmaClientJNI_disconnect(
+    JNIEnv* env, jclass cls, jlong conn) {
+  plasma::PlasmaClient* client = (plasma::PlasmaClient*)conn;


Let's do a reinterpret_cast here, we shouldn't have old style casts

pcmoritz · 2018-05-23T03:26:04Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+JNIEXPORT jobject JNICALL Java_org_apache_arrow_plasma_PlasmaClientJNI_create(
+    JNIEnv* env, jclass cls, jlong conn, jbyteArray object_id, jint size,
+    jbyteArray metadata) {
+  plasma::PlasmaClient* client = (plasma::PlasmaClient*)conn;


pcmoritz · 2018-05-23T03:27:00Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+  std::shared_ptr<Buffer> data;
+  Status s = client->Create(oid, size, md, md_size, &data);
+  if (s.IsPlasmaObjectExists()) {
+    jclass Exception = env->FindClass("org/ray/spi/impl/PlasmaObjectExistsException");


This class needs to be integrated into this PR, right? Can we also add a test that this works?

I know what you mean now, i will use the normal Exception(java.lang.Exception) instead, thanks.

robertnishihara · 2018-05-23T05:34:26Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+
+JNIEXPORT void JNICALL Java_org_apache_arrow_plasma_PlasmaClientJNI_release(
+    JNIEnv* env, jclass cls, jlong conn, jbyteArray object_id) {
+  plasma::PlasmaClient* client = (plasma::PlasmaClient*)conn;


c-style cast

robertnishihara · 2018-05-23T05:34:47Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+
+JNIEXPORT jobjectArray JNICALL Java_org_apache_arrow_plasma_PlasmaClientJNI_get(
+    JNIEnv* env, jclass cls, jlong conn, jobjectArray object_ids, jint timeout_ms) {
+  plasma::PlasmaClient* client = (plasma::PlasmaClient*)conn;


c-style cast

robertnishihara · 2018-05-23T05:34:51Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+  std::vector<plasma::ObjectID> oids(num_oids);
+  std::vector<plasma::ObjectBuffer> obufs(num_oids);
+  for (int i = 0; i < num_oids; ++i) {
+    jbyteArray_to_object_id(env, (jbyteArray)env->GetObjectArrayElement(object_ids, i),


c-style cast

robertnishihara · 2018-05-23T05:35:56Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+  jclass clsByteBufferArray = env->FindClass("[Ljava/nio/ByteBuffer;");
+
+  jobjectArray ret = env->NewObjectArray(num_oids, clsByteBufferArray, NULL);
+  jobjectArray o = NULL;


Should these NULLs be nullptr? I'm not sure.

It is point here, maybe nullptr better, i will fix them.

robertnishihara · 2018-05-23T05:36:03Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+
+JNIEXPORT jboolean JNICALL Java_org_apache_arrow_plasma_PlasmaClientJNI_contains(
+    JNIEnv* env, jclass cls, jlong conn, jbyteArray object_id) {
+  plasma::PlasmaClient* client = (plasma::PlasmaClient*)conn;


c-style cast

robertnishihara · 2018-05-23T05:36:21Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+  bool has_object;
+  ARROW_CHECK_OK(client->Contains(oid, &has_object));
+
+  if (has_object) {


maybe just return has_object

I know it's equivalent, I mean I think the code would be clearer if you replace

if (has_object) { return true; } else { return false; }

with

return has_object;

Yes, you are right, i am changing it to return has_object.

robertnishihara · 2018-05-23T05:37:11Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+  plasma::PlasmaClient* client = (plasma::PlasmaClient*)conn;
+  jsize num_oids = env->GetArrayLength(object_ids);
+
+  /*


remove unused code

If this is something that needs to be handled in the future, then add a comment with a TODO

robertnishihara · 2018-05-23T05:37:20Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+
+  std::vector<plasma::ObjectID> oids(num_oids);
+  for (int i = 0; i < num_oids; ++i) {
+    jbyteArray_to_object_id(env, (jbyteArray)env->GetObjectArrayElement(object_ids, i),


robertnishihara · 2018-05-23T05:37:32Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+JNIEXPORT jobjectArray JNICALL Java_org_apache_arrow_plasma_PlasmaClientJNI_wait(
+    JNIEnv* env, jclass cls, jlong conn, jobjectArray object_ids, jint timeout_ms,
+    jint num_returns) {
+  plasma::PlasmaClient* client = (plasma::PlasmaClient*)conn;


robertnishihara · 2018-05-23T05:37:50Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+  int num_return_objects;
+  // TODO: may be blocked. consider to add the thread support
+  ARROW_CHECK_OK(client->Wait(static_cast<int>(num_oids), oreqs.data(), num_returns,
+                              (uint64_t)timeout_ms, &num_return_objects));


robertnishihara · 2018-05-23T05:38:17Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+      env->SetObjectArrayElement(ret, num_returned, oid);
+      num_returned++;
+    } else {
+      // ARROW_CHECK(oreqs[i].status == ObjectStatus_Nonexistent);


why is this commented out?

robertnishihara · 2018-05-23T05:38:22Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+
+JNIEXPORT jlong JNICALL Java_org_apache_arrow_plasma_PlasmaClientJNI_evict(
+    JNIEnv* env, jclass cls, jlong conn, jlong num_bytes) {
+  plasma::PlasmaClient* client = (plasma::PlasmaClient*)conn;


robertnishihara · 2018-05-23T05:38:27Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+  int64_t evicted_bytes;
+  ARROW_CHECK_OK(client->Evict((int64_t)num_bytes, evicted_bytes));
+
+  return (jlong)evicted_bytes;


robertnishihara · 2018-05-23T05:38:30Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

+  plasma::PlasmaClient* client = (plasma::PlasmaClient*)conn;
+
+  int64_t evicted_bytes;
+  ARROW_CHECK_OK(client->Evict((int64_t)num_bytes, evicted_bytes));


robertnishihara · 2018-05-23T05:39:20Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.h

+ * Class:     org_apache_arrow_plasma_PlasmaClientJNI
+ * Method:    connect
+ * Signature: (Ljava/lang/String;Ljava/lang/String;I)J
+ */


@pcmoritz What should the comment style be in this file? For C++ files we've been doing // everywhere else, and using Doxygen style /// for documentation.

However, should JNI files be handled differently?

Oh, it is generated by javah automatically

Does this PR include instructions for how to generate the files?

That is only the org_apache_arrow_plasma_PlasmaClientJNI.h will by generated by javah offline, and usually we do not need to add the instructions for how to generate, because we have to write the code of org_apache_arrow_plasma_PlasmaClientJNI.cc, and just check in the org_apache_arrow_plasma_PlasmaClientJNI.h as well.

pcmoritz

+1 This looks good to me now. I'll merge unless there are more objections.

pcmoritz · 2018-07-21T05:50:07Z

@salah-man Are you or somebody from your team willing to take a stab at https://issues.apache.org/jira/browse/ARROW-2892? It is important and would be high impact I think (and you can probably do it much faster than me because I have limited experience with Java).

wesm · 2018-07-27T19:16:52Z

@salah-man can you let us know your JIRA id so we can assign this issue to you?

raulchen · 2018-08-02T12:16:13Z

hi, @wesm, this is Hao Chen from Ant Financial. Sorry for the delay, we have limited bandwidth currently.
Can you elaborate on what needs to be done? I can find someone from my team to work on this.

pcmoritz · 2018-08-02T19:21:06Z

That's great, thanks for your help!

At a high level, it should be possible to store arrow tables and tensors in python (which is implemented here: https://arrow.apache.org/docs/python/plasma.html#using-arrow-and-pandas-with-plasma) and then read them out using Java with a similar API, using zero-copy if possible.

I'm not very familiar with the arrow Java API, but it will probably need integration with https://github.com/apache/arrow/blob/543808d5b2b64df008b57efec2ab9057a9bdc723/java/memory/src/main/java/io/netty/buffer/ArrowBuf.java. We can discuss more about it in https://issues.apache.org/jira/browse/ARROW-2892.

The overall goal for us should be to make it possible to exchange data between Java and Python Ray tasks via Plasma I think. If we can achieve that, this will not only help Ray a lot but also provide the right capabilities for people who only use plasma and not Ray.

raulchen · 2018-08-03T08:28:11Z

@pcmoritz thanks for explaining.

Regarding integrating Java API with ArrowBuf, I'm new to Arrow and still don't fully understand that. Do we want something like python plasma client's get_buffers? The python get_buffers method returns a list of PlasmaBuffer, which is simply a wrapper around c++ ObjectBuffer. However, the java ArrowBuf class is much more complex and doesn't use c++ code under the hood. Does that mean we have to implement the java plasma client from scratch, without re-using c++ code?

Also, do we have any doc for Arrow Java API (ArrowBuf and related stuff)?

Thanks.

jacques-n · 2018-08-07T15:14:32Z

@salah-man, let's take this conversation to https://issues.apache.org/jira/browse/ARROW-2892

pcmoritz changed the title ~~[JavaWorker]Java client support for plasma~~ ARROW-1163: [Java] Java client support for plasma May 19, 2018

pcmoritz reviewed May 19, 2018

View reviewed changes

salah-man added 2 commits May 21, 2018 18:45

Delete the ObjectId.java and ObjectBuffer.java, change the input and …

9da40d0

…output of plasma java client api from custem type to byte[]

Changes for the feedback from pr

3e9ba0d

-------------------------- It consists of the following changes: add java/plasma/README.md for help document add java/plasma/test.sh for plasma java client test case fix some ci error delete the useless parameters of connect api from jni

salah-man force-pushed the java_lib branch from 7238a7a to 451c2c7 Compare May 21, 2018 10:58

fix the ci problem

14393f5

salah-man force-pushed the java_lib branch from 451c2c7 to 14393f5 Compare May 21, 2018 11:17

add ci of plasma java client

1c327a7

cleanups

2f3f129

pcmoritz force-pushed the java_lib branch from 295eccc to 2f3f129 Compare May 23, 2018 03:19

pcmoritz reviewed May 23, 2018

View reviewed changes

Google C++ convention

52b26e7

pcmoritz reviewed May 23, 2018

View reviewed changes

robertnishihara reviewed May 23, 2018

View reviewed changes

salah-man added 3 commits May 23, 2018 13:52

fix the problems after the feedback

b745465

small change for feedback

80e9580

fix ci lint error

b7f5e94

salah-man force-pushed the java_lib branch from 58298b1 to b7f5e94 Compare May 23, 2018 09:54

add the test case of contains and hash of plasma java client

be067e8

pcmoritz approved these changes May 24, 2018

View reviewed changes

pcmoritz closed this in ce23c06 May 25, 2018

pcmoritz mentioned this pull request May 26, 2018

[JavaWorker] Enable java worker support ray-project/ray#2094

Merged

asfimport mentioned this pull request Feb 19, 2021

[Plasma] Implement interface to get Java arrow objects from Plasma #19263

Closed

		@@ -0,0 +1,311 @@
		// Licensed to the Apache Software Foundation (ASF) under one


		#include "plasma/client.h"

		const jsize LEN_OF_OBJECTID = sizeof(plasma::ObjectID) / sizeof(jbyte);

ARROW-1163: [Java] Java client support for plasma #2065

ARROW-1163: [Java] Java client support for plasma #2065

Conversation

salah-man commented May 18, 2018 • edited Loading

robertnishihara commented May 18, 2018

jacques-n commented May 18, 2018 • edited Loading

jacques-n commented May 18, 2018

pcmoritz left a comment

Choose a reason for hiding this comment

salah-man commented May 20, 2018 • edited Loading

salah-man commented May 20, 2018

pcmoritz commented May 20, 2018

jacques-n commented May 20, 2018

robertnishihara commented May 20, 2018

salah-man commented May 20, 2018

codecov-io commented May 22, 2018 • edited Loading

Codecov Report

salah-man commented May 22, 2018

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoritz left a comment

Choose a reason for hiding this comment

pcmoritz commented Jul 21, 2018

wesm commented Jul 27, 2018

raulchen commented Aug 2, 2018

pcmoritz commented Aug 2, 2018

raulchen commented Aug 3, 2018

jacques-n commented Aug 7, 2018

salah-man commented May 18, 2018 •

edited

Loading

jacques-n commented May 18, 2018 •

edited

Loading

salah-man commented May 20, 2018 •

edited

Loading

codecov-io commented May 22, 2018 •

edited

Loading