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(bindings/java): add layers onto ops #3392

Merged
merged 12 commits into from
Nov 2, 2023
Merged

Conversation

tisonkun
Copy link
Member

@tisonkun tisonkun commented Oct 26, 2023

While we may investigate how to add Java layer support, native layers should use different tech solutions as explained in this PR.

@tisonkun
Copy link
Member Author

As for #3363 (comment), I'm not sure if we should enable RetryLayer in BehaviorExtension for all ops in tests.

Signed-off-by: tison <[email protected]>
@Zheaoli
Copy link
Member

Zheaoli commented Oct 26, 2023

I'm not sure if we should enable RetryLayer in BehaviorExtension for all ops in tests.

I think we just need to keep the same behavior with https://github.com/apache/incubator-opendal/blob/main/core/tests/behavior/utils.rs#L88-L91

@Xuanwo
Copy link
Member

Xuanwo commented Oct 26, 2023

As for #3363 (comment), I'm not sure if we should enable RetryLayer in BehaviorExtension for all ops in tests.

This is required since it's normal that services returns errors.

bindings/java/src/layer.rs Outdated Show resolved Hide resolved
bindings/java/src/layer.rs Outdated Show resolved Hide resolved
@tisonkun tisonkun closed this Oct 26, 2023
@tisonkun tisonkun deleted the nativelayer branch October 26, 2023 16:08
@tisonkun
Copy link
Member Author

It's possible to have a native layer wrapper with API -

public abstract class NativeLayer {
    public abstract long layer(long operatorNativeHandle);
}

I implement it locally, while the visibility and clone issue is suboptimal from my perspective. I don't have too much time now to dive into the trade-off so close this PR as won't do.

@tisonkun
Copy link
Member Author

... and something like -

    public static Operator of(String schema, Map<String, String> map, List<NativeLayer> layers) {
        final long nativeHandle = constructor(schema, map);
        final OperatorInfo info = makeOperatorInfo(nativeHandle);

        Operator op = new Operator(nativeHandle, info);
        for (NativeLayer layer : layers) {
            op = new Operator(layer.layer(op.nativeHandle), info);
        }

        return op;
    }

If as @Xuanwo stated, layer to be outside of constructor API but standalone API, it can be a bit complicated. FYI, in Rust core, BlockingOperator cannot be layered but always construct from a (layered) op. I highly suspect the scenario to layer a op after it's constructed and used.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 26, 2023

FYI, in Rust core, BlockingOperator cannot be layered but always construct from a (layered) op. I highly suspect the scenario to layer a op after it's constructed and used.

BlockingOperator has the exactly same in-memory layout with Operator, we add this new sturct just for seperate APIs. It's ok to add layer API for BlockingOperator.

@tisonkun tisonkun reopened this Oct 26, 2023
@tisonkun tisonkun restored the nativelayer branch October 26, 2023 16:25
@tisonkun
Copy link
Member Author

@Xuanwo the ownership issue is the major issue here. Reopen for reference.

Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
@tisonkun tisonkun changed the title feat(bindings/java): support consturct ops with native layers feat(bindings/java): add native layers onto ops Oct 27, 2023
@tisonkun
Copy link
Member Author

Passing layer as -

protected abstract Operator layer(Operator op);

The implementation is more than awful. I don't want to push the change..

Signed-off-by: tison <[email protected]>
@tisonkun tisonkun changed the title feat(bindings/java): add native layers onto ops feat(bindings/java): add layers onto ops Nov 2, 2023
Signed-off-by: tison <[email protected]>
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.

Thanks! We can enable the retry layer for behavior tests now.

@Xuanwo Xuanwo merged commit 8364d20 into apache:main Nov 2, 2023
@tisonkun tisonkun deleted the nativelayer branch November 2, 2023 12:41
@tisonkun
Copy link
Member Author

tisonkun commented Nov 2, 2023

We can enable the retry layer for behavior tests now.

It's already enabled in this patch.

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.

bindings/java: Enable retry layer by default in behavior test Support extend layers for Java binding
3 participants