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/10/26 13:41:36 UTC
[PR] feat(bindings/java): support consturct ops with native layers [incubator-opendal]
tisonkun opened a new pull request, #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392
This closes https://github.com/apache/incubator-opendal/issues/3367.
While we may investigate how to add Java layer support, native layers should use different tech solutions as explained in this PR.
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1374192886
##########
bindings/java/src/main/java/org/apache/opendal/layer/RetryNativeLayer.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.layer;
+
+import java.time.Duration;
+import lombok.Builder;
+import org.apache.opendal.NativeLayer;
+
+@Builder
+public class RetryNativeLayer extends NativeLayer {
+
+ private final boolean jitter;
Review Comment:
> This causes `RetryNativeLayer` to hold a native handle to release.
Can we implement something like `Drop` for `RetryNativeLayer`? I'm guessing java can do something while the class is not referred. By the way, the memory of `layerHandle` should be freed after `doLayer` has been called.
> Still I'm thinking of a layer concept to intercept in Java level
It's ok to keep to have different layer types.
But users don't need to know about them.
For example, users could call:
```java
op.layer(RetryLayer).layer(TracingLayer).layer(MyLayer)
```
without knowing it's `native` or not.
--
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
Re: [PR] feat(bindings/java): support consturct ops with native layers [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#issuecomment-1781425023
It's possible to have a native layer wrapper with API -
```java
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.
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1374170378
##########
bindings/java/src/test/java/org/apache/opendal/test/NativeLayerTest.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import java.util.HashMap;
+import java.util.Map;
+import lombok.Cleanup;
+import org.apache.opendal.NativeLayer;
+import org.apache.opendal.Operator;
+import org.apache.opendal.layer.RetryNativeLayer;
+import org.junit.jupiter.api.Test;
+
+public class NativeLayerTest {
+ @Test
+ void testOperatorWithRetryLayer() {
+ final Map<String, String> conf = new HashMap<>();
+ conf.put("root", "/opendal/");
+ final NativeLayer retryLayer = RetryNativeLayer.builder().build();
Review Comment:
But it's better for users partically configure the layer manner. I think of it and do not find a better solution, please raise an alternative if you can work it out.
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1379616479
##########
bindings/java/src/test/java/org/apache/opendal/test/NativeLayerTest.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import java.util.HashMap;
+import java.util.Map;
+import lombok.Cleanup;
+import org.apache.opendal.NativeLayer;
+import org.apache.opendal.Operator;
+import org.apache.opendal.layer.RetryNativeLayer;
+import org.junit.jupiter.api.Test;
+
+public class NativeLayerTest {
+ @Test
+ void testOperatorWithRetryLayer() {
+ final Map<String, String> conf = new HashMap<>();
+ conf.put("root", "/opendal/");
+ final NativeLayer retryLayer = RetryNativeLayer.builder().build();
+ @Cleanup final Operator op = Operator.of("memory", conf);
Review Comment:
I attempted but failed to improve the code. You're right in your assessment!
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1374196855
##########
bindings/java/src/test/java/org/apache/opendal/test/NativeLayerTest.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import java.util.HashMap;
+import java.util.Map;
+import lombok.Cleanup;
+import org.apache.opendal.NativeLayer;
+import org.apache.opendal.Operator;
+import org.apache.opendal.layer.RetryNativeLayer;
+import org.junit.jupiter.api.Test;
+
+public class NativeLayerTest {
+ @Test
+ void testOperatorWithRetryLayer() {
+ final Map<String, String> conf = new HashMap<>();
+ conf.put("root", "/opendal/");
+ final NativeLayer retryLayer = RetryNativeLayer.builder().build();
+ @Cleanup final Operator op = Operator.of("memory", conf);
Review Comment:
Let me have a try on this.
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1379618522
##########
bindings/java/src/main/java/org/apache/opendal/layer/RetryNativeLayer.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.layer;
+
+import java.time.Duration;
+import lombok.Builder;
+import org.apache.opendal.NativeLayer;
+
+@Builder
+public class RetryNativeLayer extends NativeLayer {
Review Comment:
The only thing left this is chaning the name of `RetryNativeLayer` into `RetryLayer`?
--
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
Re: [PR] feat(bindings/java): add layers onto ops [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392
--
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
Re: [PR] feat(bindings/java): support consturct ops with native layers [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#issuecomment-1781274504
> As for [#3363 (comment)](https://github.com/apache/incubator-opendal/pull/3363#discussion_r1372882657), 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.
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#issuecomment-1782566888
Passing layer as -
```java
protected abstract Operator layer(Operator op);
```
The implementation is more than awful. I don't want to push the change..
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1374225255
##########
bindings/java/src/main/java/org/apache/opendal/layer/RetryNativeLayer.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.layer;
+
+import java.time.Duration;
+import lombok.Builder;
+import org.apache.opendal.NativeLayer;
+
+@Builder
+public class RetryNativeLayer extends NativeLayer {
+
+ private final boolean jitter;
Review Comment:
> Can we implement something like Drop for RetryNativeLayer? I'm guessing java can do something while the class is not referred. By the way, the memory of layerHandle should be freed after doLayer has been called.
No. This is what try-with-resource and AutoCloseable does, as I implement it in `NativeObject`. This is not the way to go especially you refer to "the memory of layerHandle should be freed after doLayer has been called". It's vague and unclear to handle the ownership and lifetime properly, so I tend to use a POJO as we refactor Metadata as a POJO rather than a NativeObject.
> I'm guessing java can do something
Previously, it's "finalize", while the method is deprecated to be removed.
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1374172539
##########
bindings/java/src/main/java/org/apache/opendal/layer/RetryNativeLayer.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.layer;
+
+import java.time.Duration;
+import lombok.Builder;
+import org.apache.opendal.NativeLayer;
+
+@Builder
+public class RetryNativeLayer extends NativeLayer {
+
+ private final boolean jitter;
Review Comment:
> Is it a good idea to store a nativeHandle
No. This causes `RetryNativeLayer` to hold a native handle to release. I'd prefer to avoid this pattern as much as possible. Otherwise, Java developers should always remember to release the native handle, which is quite unfamiliar with the normal programming paradigm.
--
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
Re: [PR] feat(bindings/java): support consturct ops with native layers [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#issuecomment-1781155217
As for https://github.com/apache/incubator-opendal/pull/3363#discussion_r1372882657, I'm not sure if we should enable RetryLayer in BehaviorExtension for all ops in tests.
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1374041772
##########
bindings/java/src/main/java/org/apache/opendal/layer/RetryNativeLayer.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.layer;
+
+import java.time.Duration;
+import lombok.Builder;
+import org.apache.opendal.NativeLayer;
+
+@Builder
+public class RetryNativeLayer extends NativeLayer {
+
+ private final boolean jitter;
Review Comment:
Is it a good idea to store a `nativeHandle` (the `RetryLayer` in rust core) in `RetryNativeLayer` class and forward all functions to it? So the `doLayer` can be `doLayer(long nativeHandle, long layerHandle)`.
Besides, I have two naming things:
- How about renaming `nativeHandle` to `nativeOperator` across the whole package?
- Do we need to keep `Native` in `RetryNativeLayer`? How about just `RetryLayer`? I feel like users don't need to know about this concept.
--
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
Re: [PR] feat(bindings/java): add layers onto ops [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1379785408
##########
bindings/java/src/main/java/org/apache/opendal/layer/RetryNativeLayer.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.layer;
+
+import java.time.Duration;
+import lombok.Builder;
+import org.apache.opendal.NativeLayer;
+
+@Builder
+public class RetryNativeLayer extends NativeLayer {
Review Comment:
Updated.
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1374168377
##########
bindings/java/src/main/java/org/apache/opendal/NativeLayer.java:
##########
@@ -0,0 +1,24 @@
+/*
+ * 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;
+
+public abstract class NativeLayer {
Review Comment:
The `op.layer` method accept `NativeLayer` param. I tend to leave it as `public`.
But yes I have the same concern that some public classes or methods are limited to Java's visibility system. I tend to investigate two possibility to improve the situation:
1. Use Java's module system. But it requires JDK 9+, while it doesn't cause an error in JDK 1.8.
2. Use Apache Yetus' [interface audience](https://yetus.apache.org/documentation/in-progress/javadocs/org/apache/yetus/audience/package-summary.html) and other tools to maintain the interface stability and visibility module.
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1374177243
##########
bindings/java/src/main/java/org/apache/opendal/layer/RetryNativeLayer.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.layer;
+
+import java.time.Duration;
+import lombok.Builder;
+import org.apache.opendal.NativeLayer;
+
+@Builder
+public class RetryNativeLayer extends NativeLayer {
+
+ private final boolean jitter;
Review Comment:
> Let me think of it. This can be reasonable.
Let's keep it as is for now. Still I'm thinking of a layer concept to intercept in Java level, so keep NativeLayers named as NativeLayer leave the room for such possibility.
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1374042845
##########
bindings/java/src/test/java/org/apache/opendal/test/NativeLayerTest.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import java.util.HashMap;
+import java.util.Map;
+import lombok.Cleanup;
+import org.apache.opendal.NativeLayer;
+import org.apache.opendal.Operator;
+import org.apache.opendal.layer.RetryNativeLayer;
+import org.junit.jupiter.api.Test;
+
+public class NativeLayerTest {
+ @Test
+ void testOperatorWithRetryLayer() {
+ final Map<String, String> conf = new HashMap<>();
+ conf.put("root", "/opendal/");
+ final NativeLayer retryLayer = RetryNativeLayer.builder().build();
Review Comment:
The `builder().build()` pattern looks like a bit verbose.
##########
bindings/java/src/main/java/org/apache/opendal/layer/RetryNativeLayer.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.layer;
+
+import java.time.Duration;
+import lombok.Builder;
+import org.apache.opendal.NativeLayer;
+
+@Builder
+public class RetryNativeLayer extends NativeLayer {
+
+ private final boolean jitter;
Review Comment:
Is it a good idea to store a `nativeHandle` (the `RetryLayer` in rust core) in `RetryNativeLayer` class and forward all functions to it? So the `doLayer` can be `doLayer(long nativeHandle, long layerHandle)`.
Besides, I have two naming things:
- How renaming `nativeHandle` to `nativeOperator`?
- Do we need to keep `Native` in `RetryNativeLayer`? How about just `RetryLayer`? I feel like users don't need to know about this concept.
##########
bindings/java/src/main/java/org/apache/opendal/NativeLayer.java:
##########
@@ -0,0 +1,24 @@
+/*
+ * 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;
+
+public abstract class NativeLayer {
Review Comment:
Can we make `NativeLayer` private and only used inside opendal package?
##########
bindings/java/src/test/java/org/apache/opendal/test/NativeLayerTest.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import java.util.HashMap;
+import java.util.Map;
+import lombok.Cleanup;
+import org.apache.opendal.NativeLayer;
+import org.apache.opendal.Operator;
+import org.apache.opendal.layer.RetryNativeLayer;
+import org.junit.jupiter.api.Test;
+
+public class NativeLayerTest {
+ @Test
+ void testOperatorWithRetryLayer() {
+ final Map<String, String> conf = new HashMap<>();
+ conf.put("root", "/opendal/");
+ final NativeLayer retryLayer = RetryNativeLayer.builder().build();
+ @Cleanup final Operator op = Operator.of("memory", conf);
Review Comment:
Can we have `Operator.of("memory", conf).layer(retryLayer)`?
--
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
Re: [PR] feat(bindings/java): support consturct ops with native layers [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#issuecomment-1781446003
> 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`.
--
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
Re: [PR] feat(bindings/java): support consturct ops with native layers [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#issuecomment-1781451555
@Xuanwo the ownership issue is the major issue here. Reopen for reference.
--
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
Re: [PR] feat(bindings/java): add layers onto ops [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#issuecomment-1790656437
> We can enable the retry layer for behavior tests now.
It's already enabled in this patch.
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1374173244
##########
bindings/java/src/main/java/org/apache/opendal/layer/RetryNativeLayer.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.layer;
+
+import java.time.Duration;
+import lombok.Builder;
+import org.apache.opendal.NativeLayer;
+
+@Builder
+public class RetryNativeLayer extends NativeLayer {
+
+ private final boolean jitter;
Review Comment:
> How about renaming nativeHandle to nativeOperator across the whole package?
It's possible. Let's do it in a follow-up PR. It changes only parameter name so it won't break API also.
--
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
Re: [PR] feat(bindings/java): support consturct ops with native layers [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun closed pull request #3392: feat(bindings/java): support consturct ops with native layers
URL: https://github.com/apache/incubator-opendal/pull/3392
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1374169561
##########
bindings/java/src/test/java/org/apache/opendal/test/NativeLayerTest.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import java.util.HashMap;
+import java.util.Map;
+import lombok.Cleanup;
+import org.apache.opendal.NativeLayer;
+import org.apache.opendal.Operator;
+import org.apache.opendal.layer.RetryNativeLayer;
+import org.junit.jupiter.api.Test;
+
+public class NativeLayerTest {
+ @Test
+ void testOperatorWithRetryLayer() {
+ final Map<String, String> conf = new HashMap<>();
+ conf.put("root", "/opendal/");
+ final NativeLayer retryLayer = RetryNativeLayer.builder().build();
+ @Cleanup final Operator op = Operator.of("memory", conf);
Review Comment:
This is the issue I talked with you about the ownership. Since we use clone in the rust side:
```rust
Box::into_raw(Box::new(op.clone().layer(retry))) as jlong
```
If we writes `Operator.of("memory", conf).layer(retryLayer)`, the temp var `Operator.of("memory", conf)` cannot properly release its underneath resource.
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1374174045
##########
bindings/java/src/main/java/org/apache/opendal/layer/RetryNativeLayer.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.layer;
+
+import java.time.Duration;
+import lombok.Builder;
+import org.apache.opendal.NativeLayer;
+
+@Builder
+public class RetryNativeLayer extends NativeLayer {
+
+ private final boolean jitter;
Review Comment:
> Do we need to keep Native in RetryNativeLayer? How about just RetryLayer? I feel like users don't need to know about this concept.
Let me think of it. This can be reasonable.
--
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
Re: [PR] feat(bindings/java): add native layers onto ops [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1374220618
##########
bindings/java/src/main/java/org/apache/opendal/layer/RetryNativeLayer.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.layer;
+
+import java.time.Duration;
+import lombok.Builder;
+import org.apache.opendal.NativeLayer;
+
+@Builder
+public class RetryNativeLayer extends NativeLayer {
+
+ private final boolean jitter;
Review Comment:
> users could call
I have another round for this and agree with it. Let me modify the interface.
--
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
Re: [PR] feat(bindings/java): support consturct ops with native layers [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#discussion_r1373304966
##########
bindings/java/src/layer.rs:
##########
@@ -0,0 +1,60 @@
+// 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.
+
+use std::time::Duration;
+
+use jni::objects::JClass;
+use jni::sys::{jboolean, jfloat, jlong};
+use jni::JNIEnv;
+
+use opendal::layers::RetryLayer;
+use opendal::raw::{FusedAccessor, Layer};
+
+pub(crate) enum NativeLayer {
Review Comment:
Can we remove `enum`? I wish we can have a simple class to rust layer mapping instead. For example, holding a pointer of `RetryLayer` in a class. Is that possible?
##########
bindings/java/src/layer.rs:
##########
@@ -0,0 +1,60 @@
+// 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.
+
+use std::time::Duration;
+
+use jni::objects::JClass;
+use jni::sys::{jboolean, jfloat, jlong};
+use jni::JNIEnv;
+
+use opendal::layers::RetryLayer;
+use opendal::raw::{FusedAccessor, Layer};
+
+pub(crate) enum NativeLayer {
+ Retry(RetryLayer),
+}
+
+impl NativeLayer {
Review Comment:
We use `NativeXxx` to handle Rust-related types. However, the term `native` often reminds me of `java's native xxx`.
Do you think it's a good idea to use `RawAbc` or `CoreAbc` instead?
--
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
Re: [PR] feat(bindings/java): support consturct ops with native layers [incubator-opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#issuecomment-1781436319
... and something like -
```java
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.
--
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
Re: [PR] feat(bindings/java): support consturct ops with native layers [incubator-opendal]
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on PR #3392:
URL: https://github.com/apache/incubator-opendal/pull/3392#issuecomment-1781274422
> 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
--
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