Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(binding/java): add list & remove_all support #3333

Merged
merged 23 commits into from
Oct 25, 2023
Merged

feat(binding/java): add list & remove_all support #3333

merged 23 commits into from
Oct 25, 2023

Conversation

G-XD
Copy link
Contributor

@G-XD G-XD commented Oct 18, 2023

Related: #3169

@tisonkun
Copy link
Member

tisonkun commented Oct 18, 2023

Let's wait a bit for the refactor PR #3332 and do a rebase. I assume that rebase this PR is simpler than rebase that one (for behavior tests). Sorry to make such conflicts.

@tisonkun
Copy link
Member

Now you can do the rebase and I'll review this PR in about days - I'm a bit busy in today and tomorrow.

# Conflicts:
#	bindings/java/src/test/java/org/apache/opendal/test/behavior/AbstractBehaviorTest.java
.gitattributes Outdated Show resolved Hide resolved
@G-XD
Copy link
Contributor Author

G-XD commented Oct 20, 2023

Hi @tisonkun @Xuanwo , PTAL

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

I support the changes for:

  • remove all
  • list without args

Perhaps we can have another round to think of how to pass over args via JNI. But as long as we don't populate a large amount methods in this flavor, I can support to give it a try (as a baseline to improve).

BTW, how to convert list is another issue, you can take a look at this PR for another flavor. I don't have a strong preference for this point but we may align the pattern.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 23, 2023

BTW, how to convert list is another issue, you can take a look at this PR for another flavor. I don't have a strong preference for this point but we may align the pattern.

I'm not sure if builder pattern is a good idea in java like:

op.list_with()
    .limit(5)
    .startAfter(someElement)
    .join();

But anyway, I do agree with @tisonkun's comment that we can merge remove_all and list support first. We can try and discuss the api for list_with in following PRs.

@G-XD
Copy link
Contributor Author

G-XD commented Oct 23, 2023

BTW, how to convert list is another issue, you can take a look at this PR for another flavor. I don't have a strong preference for this point but we may align the pattern.

Since the List is not returned directly in Operator, but rather CompletableFuture, I modified loadEnabledServices().

@tisonkun
Copy link
Member

@G-XD One things to be verified:

  • set_object_array_element is a native JNI call;
  • List with put via JNI is crossing the language boundary.

Do you check which one is more efficient?

I prefer set_object_array_element and construct List with a preset array to avoid too many cross boundary invocations. But I don't do a performance test yet.

@G-XD
Copy link
Contributor Author

G-XD commented Oct 23, 2023

Do you check which one is more efficient?

I didn't do a performance comparison test, but I think minimize marshalling of resources across the JNI layer is correct.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 23, 2023

What's the difference of List and Entry[]?

@tisonkun
Copy link
Member

tisonkun commented Oct 24, 2023

op.list_with()
    .limit(5)
    .startAfter(someElement)
    .join();

This is exactly what I expected to implement in Java API. Let's investigate it in a following PR.

Maybe

class Lister {
  private final Operator op;
  private int limit;
  public Lister limit(int limit) {
    this.limit = limit;
    return this;
  }

  public CompletableFuture<T> execute() {
   return executeNative(op, limit, <other args>); 
  }

  private static native executeNative( ... );
}

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Generally looks good. Comments inline.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Other LGTM from my side.

.github/workflows/bindings_java.yml Outdated Show resolved Hide resolved
@tisonkun
Copy link
Member

Merging...

Thanks for your excellent work!

@tisonkun tisonkun merged commit c5333ea into apache:main Oct 25, 2023
44 checks passed
@G-XD G-XD deleted the java_binding_list branch October 25, 2023 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants