You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "tisonkun (via GitHub)" <gi...@apache.org> on 2023/05/20 10:06:40 UTC
[GitHub] [incubator-opendal] tisonkun commented on a diff in pull request #2276: refactor(binding/java): build, async and docs
tisonkun commented on code in PR #2276:
URL: https://github.com/apache/incubator-opendal/pull/2276#discussion_r1199590583
##########
bindings/java/README.md:
##########
@@ -1,8 +1,18 @@
+# OpenDAL Java Bindings
+
+## Build and test
Review Comment:
Add basic build instruction.
##########
bindings/java/src/main/java/org/apache/opendal/Metadata.java:
##########
@@ -19,27 +19,21 @@
package org.apache.opendal;
-public class Metadata extends OpenDALObject {
- public Metadata(long ptr) {
- this.ptr = ptr;
+public class Metadata extends NativeObject {
+ public Metadata(long nativeHandle) {
+ super(nativeHandle);
}
public boolean isFile() {
- return isFile(this.ptr);
+ return isFile(nativeHandle);
}
public long getContentLength() {
- return getContentLength(this.ptr);
+ return getContentLength(nativeHandle);
}
@Override
- public void close() {
- freeMetadata(this.ptr);
- }
-
- private native void freeMetadata(long statPtr);
-
- private native boolean isFile(long statPtr);
-
- private native long getContentLength(long statPtr);
+ protected native void disposeInternal(long handle);
Review Comment:
See 7b9b65e5c1a7f98de4a09cbab61e9ca336ef877d
##########
bindings/java/src/main/java/org/apache/opendal/Metadata.java:
##########
@@ -19,27 +19,21 @@
package org.apache.opendal;
-public class Metadata extends OpenDALObject {
- public Metadata(long ptr) {
- this.ptr = ptr;
+public class Metadata extends NativeObject {
+ public Metadata(long nativeHandle) {
+ super(nativeHandle);
}
public boolean isFile() {
- return isFile(this.ptr);
+ return isFile(nativeHandle);
}
public long getContentLength() {
- return getContentLength(this.ptr);
+ return getContentLength(nativeHandle);
}
@Override
- public void close() {
- freeMetadata(this.ptr);
- }
-
- private native void freeMetadata(long statPtr);
-
- private native boolean isFile(long statPtr);
-
- private native long getContentLength(long statPtr);
+ protected native void disposeInternal(long handle);
+ private static native boolean isFile(long nativeHandle);
Review Comment:
Most native methods can be static.
##########
bindings/java/src/lib.rs:
##########
@@ -15,71 +15,49 @@
// specific language governing permissions and limitations
// under the License.
-use std::cell::RefCell;
use std::collections::HashMap;
use std::ffi::c_void;
use std::str::FromStr;
-use std::sync::Arc;
use jni::objects::JClass;
use jni::objects::JMap;
use jni::objects::JObject;
use jni::objects::JString;
use jni::objects::JThrowable;
use jni::objects::JValue;
-use jni::sys::jboolean;
-use jni::sys::jint;
-use jni::sys::jlong;
-use jni::sys::JNI_VERSION_1_8;
-use jni::JNIEnv;
-use jni::JavaVM;
+use jni::sys::{jboolean, jobject, JNI_VERSION_1_8};
+use jni::sys::{jint, jlong};
+use jni::{JNIEnv, JavaVM};
use once_cell::sync::OnceCell;
+use tokio::runtime::Builder;
+use tokio::runtime::Runtime;
+
use opendal::BlockingOperator;
use opendal::Operator;
use opendal::Scheme;
-use tokio::runtime::Builder;
-use tokio::runtime::Runtime;
static mut RUNTIME: OnceCell<Runtime> = OnceCell::new();
-
-thread_local! {
Review Comment:
We don't need thread-locals. Because:
1. JavaVM is globally the same one.
2. ENV can be retrieved locally while attaching the current thread as a daemon is idempotent so that we don't increase cost.
##########
bindings/java/src/lib.rs:
##########
@@ -131,53 +112,60 @@ pub unsafe extern "system" fn Java_org_apache_opendal_Operator_writeAsync(
ptr: *mut Operator,
file: JString,
content: JString,
- future: JObject,
-) {
+) -> jobject {
let op = &mut *ptr;
let file: String = env.get_string(&file).unwrap().into();
let content: String = env.get_string(&content).unwrap().into();
+
+ let class = "java/util/concurrent/CompletableFuture";
+ let f = env.new_object(class, "()V", &[]).unwrap();
Review Comment:
Return an object instead of using "out" parameter.
##########
bindings/java/src/main/java/org/apache/opendal/exception/ODException.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.opendal.exception;
+
+public class ODException extends RuntimeException {
+ private final Code code;
+
+ public ODException(byte code, String message) {
Review Comment:
So that we don't have to pass strings from Rust to Java.
##########
bindings/java/src/main/java/org/apache/opendal/NativeObject.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.opendal;
+
+import io.questdb.jar.jni.JarJniLoader;
+import java.util.concurrent.atomic.AtomicReference;
+
+public abstract class NativeObject implements AutoCloseable {
+
+ private enum LibraryState {
+ NOT_LOADED,
+ LOADING,
+ LOADED
+ }
+
+ private static final AtomicReference<LibraryState> libraryLoaded =
+ new AtomicReference<>(LibraryState.NOT_LOADED);
+
+ static {
+ NativeObject.loadLibrary();
+ }
+
+ public static void loadLibrary() {
Review Comment:
Avoid concurrent loading overhead.
##########
bindings/java/src/main/java/org/apache/opendal/Metadata.java:
##########
@@ -19,27 +19,21 @@
package org.apache.opendal;
-public class Metadata extends OpenDALObject {
- public Metadata(long ptr) {
- this.ptr = ptr;
+public class Metadata extends NativeObject {
Review Comment:
Factor out loading and free staff to super class `NativeObject`
##########
bindings/java/pom.xml:
##########
@@ -58,56 +62,53 @@
<dependency>
<groupId>org.questdb</groupId>
<artifactId>jar-jni</artifactId>
- <version>${rust-maven-plugin.version}</version>
+ <version>${questdb.version}</version>
</dependency>
+
<dependency>
<groupId>io.cucumber</groupId>
<artifactId>cucumber-java</artifactId>
<scope>test</scope>
</dependency>
-
<dependency>
<groupId>io.cucumber</groupId>
<artifactId>cucumber-junit-platform-engine</artifactId>
<scope>test</scope>
</dependency>
-
<dependency>
<groupId>org.junit.platform</groupId>
<artifactId>junit-platform-suite</artifactId>
<scope>test</scope>
</dependency>
-
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
<scope>test</scope>
</dependency>
-
-
</dependencies>
<build>
+ <testResources>
Review Comment:
Always follow symbol link, so that `bindings.feature` always work instead of suffering from relative path.
##########
bindings/java/src/main/java/org/apache/opendal/exception/ODException.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.opendal.exception;
+
+public class ODException extends RuntimeException {
+ private final Code code;
+
+ public ODException(byte code, String message) {
+ this(Code.getCode(code), message);
+ }
+
+ public ODException(Code code, String message) {
+ super(message);
+ this.code = code;
+ }
+
+ public Code getCode() {
+ return code;
+ }
+
+ public enum Code {
Review Comment:
Simplify a bit class naming. No `OpenDALException` and `OpenDALErrorCode`. Also, avoid constructing Code via JNI.
##########
bindings/java/pom.xml:
##########
@@ -25,14 +25,18 @@
<modelVersion>4.0.0</modelVersion>
<groupId>org.apache.opendal</groupId>
- <artifactId>java-binding</artifactId>
+ <artifactId>opendaljni</artifactId>
<version>1.0-SNAPSHOT</version>
<properties>
- <maven.compiler.source>8</maven.compiler.source>
- <maven.compiler.target>8</maven.compiler.target>
+ <maven.compiler.source>1.8</maven.compiler.source>
Review Comment:
Reduce manually configured `maven-compiler-plugin`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org