You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "suyanhanx (via GitHub)" <gi...@apache.org> on 2023/11/04 07:02:57 UTC
[PR] feat(bindings/nodejs): add retry layer [incubator-opendal]
suyanhanx opened a new pull request, #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484
(no comment)
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1387745305
##########
bindings/nodejs/index.js:
##########
@@ -20,6 +20,9 @@
/// <reference types="node" />
require('dotenv').config()
-const { Operator } = require('./generated.js')
+const { Operator, RetryLayer } = require('./generated.js')
module.exports.Operator = Operator
+module.exports.layers = {
Review Comment:
They will not be created directly by the user.
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#issuecomment-1806734150
Hi, I don't like the changes in this PR, but I can't have better ideas withou playing with it. Can I have a try based on this PR, maybe push some changes if it's ok?
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#issuecomment-1807936824
> Hi, @suyanhanx, here's my idea but I'm not well-versed in nodejs. Could you take it further and delve deeper into it?
I see. I'll finish 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/nodejs): add retry layer [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1387819626
##########
bindings/nodejs/src/lib.rs:
##########
@@ -15,6 +15,8 @@
// specific language governing permissions and limitations
// under the License.
+#![allow(clippy::not_unsafe_ptr_arg_deref)]
Review Comment:
I got the problem now.
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1387775556
##########
bindings/nodejs/src/lib.rs:
##########
@@ -686,6 +691,117 @@ impl PresignedRequest {
}
}
+pub trait NodeLayer: Send + Sync {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator;
+}
+
+struct NodeLayerWrapper {
+ inner: Box<dyn NodeLayer>,
+}
+
+#[napi]
+impl Operator {
+ /// Add a layer to this operator.
+ #[napi]
+ pub fn layer(&self, env: Env, layer: JsObject) -> Result<Self> {
+ let ctx: &mut NodeLayerWrapper = env
+ .unwrap(&layer)
+ .map_err(|e| Error::from_reason(format!("failed to unwrap layer: {}", e)))?;
+ Ok(Self(ctx.inner.layer(self.0.clone())))
+ }
+}
+
+/// A layer that will retry the request if it fails.
+/// It will retry with exponential backoff.
+///
+/// ## Parameters
+///
+/// - `jitter`<bool>: Whether to add jitter to the backoff.
+/// - `max_times`<number>: The maximum number of times to retry.
+/// - `factor`<number>: The exponential factor to use.
+/// - `max_delay`<number>: The maximum delay between retries. The unit is microsecond.
+/// - `min_delay`<number>: The minimum delay between retries. The unit is microsecond.
+#[napi]
+pub struct RetryLayer(opendal::layers::RetryLayer);
+
+impl NodeLayer for RetryLayer {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator {
+ op.layer(self.0.clone())
+ }
+}
+
+/// RetryLayer constructor.
+#[js_function(1)]
+pub fn create_retry_layer(ctx: CallContext) -> Result<JsObject> {
Review Comment:
IMO, no. I considered that every parameter is optional, and we need to deal with those unprovided parameters silently, but this get method will raise an error when trying to get an undefined property, I had to deal with those error cases one by one.
Do you have any suggestions?
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1382477957
##########
.github/workflows/bindings_nodejs.yml:
##########
@@ -68,8 +66,8 @@ jobs:
- name: Check diff
run: git diff --exit-code
- - name: Unit test
- run: cargo nextest run --no-fail-fast
Review Comment:
> Seems napi-rs error, FYI https://github.com/napi-rs/napi-rs/issues/1782
Thanks for raising the issue. 😊
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1382521576
##########
bindings/nodejs/tests/suites/index.mjs:
##########
@@ -36,7 +36,11 @@ export function runner(testName, scheme) {
config.root = generateRandomRoot(config.root)
}
- const operator = scheme ? new Operator(scheme, config) : null
+ let operator = scheme ? new Operator(scheme, config) : null
+
+ let retryLayer = new RetryLayer(true, 4)
Review Comment:
Do you have any suggestions?
Like providing params as a dic like `{jitter:ture, maxTimes: 4}`?
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1387784467
##########
bindings/nodejs/src/lib.rs:
##########
@@ -686,6 +691,117 @@ impl PresignedRequest {
}
}
+pub trait NodeLayer: Send + Sync {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator;
+}
+
+struct NodeLayerWrapper {
+ inner: Box<dyn NodeLayer>,
+}
+
+#[napi]
+impl Operator {
+ /// Add a layer to this operator.
+ #[napi]
+ pub fn layer(&self, env: Env, layer: JsObject) -> Result<Self> {
+ let ctx: &mut NodeLayerWrapper = env
+ .unwrap(&layer)
+ .map_err(|e| Error::from_reason(format!("failed to unwrap layer: {}", e)))?;
+ Ok(Self(ctx.inner.layer(self.0.clone())))
+ }
+}
+
+/// A layer that will retry the request if it fails.
+/// It will retry with exponential backoff.
+///
+/// ## Parameters
+///
+/// - `jitter`<bool>: Whether to add jitter to the backoff.
+/// - `max_times`<number>: The maximum number of times to retry.
+/// - `factor`<number>: The exponential factor to use.
+/// - `max_delay`<number>: The maximum delay between retries. The unit is microsecond.
+/// - `min_delay`<number>: The minimum delay between retries. The unit is microsecond.
+#[napi]
+pub struct RetryLayer(opendal::layers::RetryLayer);
+
+impl NodeLayer for RetryLayer {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator {
+ op.layer(self.0.clone())
+ }
+}
+
+/// RetryLayer constructor.
+#[js_function(1)]
+pub fn create_retry_layer(ctx: CallContext) -> Result<JsObject> {
+ let mut retry = opendal::layers::RetryLayer::default();
+
+ let config: Either<JsObject, JsUndefined> = ctx.try_get::<JsObject>(0)?;
Review Comment:
> register those js function in rust
What do you mean? I'm not sure about this part.
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1387782422
##########
bindings/nodejs/src/lib.rs:
##########
@@ -15,6 +15,8 @@
// specific language governing permissions and limitations
// under the License.
+#![allow(clippy::not_unsafe_ptr_arg_deref)]
Review Comment:
The `napi` macro will make sure every parameter and return value's type implements `ToNapiValue`, but a pointer (the Box<dyn layer>)can't implement it. To pass a pointer to js env, it’s necessary to use this compat mode, the `js_function`.
##########
bindings/nodejs/src/lib.rs:
##########
@@ -15,6 +15,8 @@
// specific language governing permissions and limitations
// under the License.
+#![allow(clippy::not_unsafe_ptr_arg_deref)]
Review Comment:
The `napi` macro will make sure every parameter and return value's type implements `ToNapiValue`, but a pointer (the Box<dyn layer>)can't implement it. To pass a pointer to js env, it’s necessary to use this compat mode, the `js_function`.
##########
bindings/nodejs/src/lib.rs:
##########
@@ -15,6 +15,8 @@
// specific language governing permissions and limitations
// under the License.
+#![allow(clippy::not_unsafe_ptr_arg_deref)]
Review Comment:
The `napi` macro will make sure every parameter and return value's type implements `ToNapiValue`, but a pointer (the `Box<dyn layer>`)can't implement it. To pass a pointer to js env, it’s necessary to use this compat mode, the `js_function`.
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1391319418
##########
bindings/nodejs/src/lib.rs:
##########
@@ -15,6 +15,9 @@
// specific language governing permissions and limitations
// under the License.
+#![allow(clippy::not_unsafe_ptr_arg_deref)]
+/// This line exists here because of the `js_function` macro.
Review Comment:
Maybe we can remove this?
##########
.github/workflows/bindings_nodejs.yml:
##########
@@ -65,12 +63,12 @@ jobs:
- name: Check format
run: yarn run prettier --check .
+ - name: Build
Review Comment:
Maybe we can revert this change now?
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1387772252
##########
bindings/nodejs/src/lib.rs:
##########
@@ -15,6 +15,8 @@
// specific language governing permissions and limitations
// under the License.
+#![allow(clippy::not_unsafe_ptr_arg_deref)]
Review Comment:
Maybe we can use `#[napi(constructor)]` instead like what we do for Operator?
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#issuecomment-1806794534
> Hi, I don't like the changes in this PR, but I can't have better ideas withou playing with it. Can I have a try based on this PR, maybe push some changes if it's ok?
It's ok. It's up to you.😇
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1382521497
##########
bindings/nodejs/generated.js:
##########
@@ -271,10 +271,11 @@ if (!nativeBinding) {
throw new Error(`Failed to load native binding`)
}
-const { Operator, Entry, Metadata, Lister, BlockingLister } = nativeBinding
+const { Operator, Entry, Metadata, Lister, BlockingLister, RetryLayer } = nativeBinding
module.exports.Operator = Operator
module.exports.Entry = Entry
module.exports.Metadata = Metadata
module.exports.Lister = Lister
module.exports.BlockingLister = BlockingLister
+module.exports.RetryLayer = RetryLayer
Review Comment:
It's ok to group layers in a namespace.
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1387775556
##########
bindings/nodejs/src/lib.rs:
##########
@@ -686,6 +691,117 @@ impl PresignedRequest {
}
}
+pub trait NodeLayer: Send + Sync {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator;
+}
+
+struct NodeLayerWrapper {
+ inner: Box<dyn NodeLayer>,
+}
+
+#[napi]
+impl Operator {
+ /// Add a layer to this operator.
+ #[napi]
+ pub fn layer(&self, env: Env, layer: JsObject) -> Result<Self> {
+ let ctx: &mut NodeLayerWrapper = env
+ .unwrap(&layer)
+ .map_err(|e| Error::from_reason(format!("failed to unwrap layer: {}", e)))?;
+ Ok(Self(ctx.inner.layer(self.0.clone())))
+ }
+}
+
+/// A layer that will retry the request if it fails.
+/// It will retry with exponential backoff.
+///
+/// ## Parameters
+///
+/// - `jitter`<bool>: Whether to add jitter to the backoff.
+/// - `max_times`<number>: The maximum number of times to retry.
+/// - `factor`<number>: The exponential factor to use.
+/// - `max_delay`<number>: The maximum delay between retries. The unit is microsecond.
+/// - `min_delay`<number>: The minimum delay between retries. The unit is microsecond.
+#[napi]
+pub struct RetryLayer(opendal::layers::RetryLayer);
+
+impl NodeLayer for RetryLayer {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator {
+ op.layer(self.0.clone())
+ }
+}
+
+/// RetryLayer constructor.
+#[js_function(1)]
+pub fn create_retry_layer(ctx: CallContext) -> Result<JsObject> {
Review Comment:
I considered that every parameter is optional, and we need to deal with those unprovided parameters silently, but this get method will raise an error when trying to get an undefined property, I had to deal with those error cases one by one.
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1387776134
##########
bindings/nodejs/src/lib.rs:
##########
@@ -686,6 +691,117 @@ impl PresignedRequest {
}
}
+pub trait NodeLayer: Send + Sync {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator;
+}
+
+struct NodeLayerWrapper {
+ inner: Box<dyn NodeLayer>,
+}
+
+#[napi]
+impl Operator {
+ /// Add a layer to this operator.
+ #[napi]
+ pub fn layer(&self, env: Env, layer: JsObject) -> Result<Self> {
+ let ctx: &mut NodeLayerWrapper = env
+ .unwrap(&layer)
+ .map_err(|e| Error::from_reason(format!("failed to unwrap layer: {}", e)))?;
+ Ok(Self(ctx.inner.layer(self.0.clone())))
+ }
+}
+
+/// A layer that will retry the request if it fails.
+/// It will retry with exponential backoff.
+///
+/// ## Parameters
+///
+/// - `jitter`<bool>: Whether to add jitter to the backoff.
+/// - `max_times`<number>: The maximum number of times to retry.
+/// - `factor`<number>: The exponential factor to use.
+/// - `max_delay`<number>: The maximum delay between retries. The unit is microsecond.
+/// - `min_delay`<number>: The minimum delay between retries. The unit is microsecond.
+#[napi]
+pub struct RetryLayer(opendal::layers::RetryLayer);
+
+impl NodeLayer for RetryLayer {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator {
+ op.layer(self.0.clone())
+ }
+}
+
+/// RetryLayer constructor.
+#[js_function(1)]
+pub fn create_retry_layer(ctx: CallContext) -> Result<JsObject> {
+ let mut retry = opendal::layers::RetryLayer::default();
+
+ let config: Either<JsObject, JsUndefined> = ctx.try_get::<JsObject>(0)?;
Review Comment:
I'm thining about don't register those js function in rust
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1387775556
##########
bindings/nodejs/src/lib.rs:
##########
@@ -686,6 +691,117 @@ impl PresignedRequest {
}
}
+pub trait NodeLayer: Send + Sync {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator;
+}
+
+struct NodeLayerWrapper {
+ inner: Box<dyn NodeLayer>,
+}
+
+#[napi]
+impl Operator {
+ /// Add a layer to this operator.
+ #[napi]
+ pub fn layer(&self, env: Env, layer: JsObject) -> Result<Self> {
+ let ctx: &mut NodeLayerWrapper = env
+ .unwrap(&layer)
+ .map_err(|e| Error::from_reason(format!("failed to unwrap layer: {}", e)))?;
+ Ok(Self(ctx.inner.layer(self.0.clone())))
+ }
+}
+
+/// A layer that will retry the request if it fails.
+/// It will retry with exponential backoff.
+///
+/// ## Parameters
+///
+/// - `jitter`<bool>: Whether to add jitter to the backoff.
+/// - `max_times`<number>: The maximum number of times to retry.
+/// - `factor`<number>: The exponential factor to use.
+/// - `max_delay`<number>: The maximum delay between retries. The unit is microsecond.
+/// - `min_delay`<number>: The minimum delay between retries. The unit is microsecond.
+#[napi]
+pub struct RetryLayer(opendal::layers::RetryLayer);
+
+impl NodeLayer for RetryLayer {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator {
+ op.layer(self.0.clone())
+ }
+}
+
+/// RetryLayer constructor.
+#[js_function(1)]
+pub fn create_retry_layer(ctx: CallContext) -> Result<JsObject> {
Review Comment:
OMO, no. I considered that every parameter is optional, and we need to deal with those unprovided parameters silently, but this get method will raise an error when trying to get an undefined property, I had to deal with those error cases one by one.
Do you have any suggestions?
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1387738037
##########
bindings/nodejs/src/lib.rs:
##########
@@ -686,6 +691,117 @@ impl PresignedRequest {
}
}
+pub trait NodeLayer: Send + Sync {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator;
+}
+
+struct NodeLayerWrapper {
+ inner: Box<dyn NodeLayer>,
+}
+
+#[napi]
+impl Operator {
+ /// Add a layer to this operator.
+ #[napi]
+ pub fn layer(&self, env: Env, layer: JsObject) -> Result<Self> {
+ let ctx: &mut NodeLayerWrapper = env
+ .unwrap(&layer)
+ .map_err(|e| Error::from_reason(format!("failed to unwrap layer: {}", e)))?;
+ Ok(Self(ctx.inner.layer(self.0.clone())))
+ }
+}
+
+/// A layer that will retry the request if it fails.
+/// It will retry with exponential backoff.
+///
+/// ## Parameters
+///
+/// - `jitter`<bool>: Whether to add jitter to the backoff.
+/// - `max_times`<number>: The maximum number of times to retry.
+/// - `factor`<number>: The exponential factor to use.
+/// - `max_delay`<number>: The maximum delay between retries. The unit is microsecond.
+/// - `min_delay`<number>: The minimum delay between retries. The unit is microsecond.
+#[napi]
+pub struct RetryLayer(opendal::layers::RetryLayer);
+
+impl NodeLayer for RetryLayer {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator {
+ op.layer(self.0.clone())
+ }
+}
+
+/// RetryLayer constructor.
+#[js_function(1)]
+pub fn create_retry_layer(ctx: CallContext) -> Result<JsObject> {
+ let mut retry = opendal::layers::RetryLayer::default();
+
+ let config: Either<JsObject, JsUndefined> = ctx.try_get::<JsObject>(0)?;
+
+ if let Either::A(config) = config {
+ let jitter = config.get_named_property::<JsBoolean>("jitter");
+ if let Ok(jitter) = jitter {
+ let jitter = jitter.get_value();
+ if let Ok(jitter) = jitter {
+ if jitter {
+ retry = retry.with_jitter();
+ }
+ }
+ }
+
+ let max_times = config.get_named_property::<JsNumber>("maxTimes");
+ if let Ok(max_times) = max_times {
+ let max_times = max_times.get_uint32();
+ if let Ok(max_times) = max_times {
+ retry = retry.with_max_times(max_times as usize);
+ }
+ }
+ //
Review Comment:
not used.
##########
bindings/nodejs/src/lib.rs:
##########
@@ -686,6 +691,117 @@ impl PresignedRequest {
}
}
+pub trait NodeLayer: Send + Sync {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator;
+}
+
+struct NodeLayerWrapper {
+ inner: Box<dyn NodeLayer>,
+}
+
+#[napi]
+impl Operator {
+ /// Add a layer to this operator.
+ #[napi]
+ pub fn layer(&self, env: Env, layer: JsObject) -> Result<Self> {
+ let ctx: &mut NodeLayerWrapper = env
+ .unwrap(&layer)
+ .map_err(|e| Error::from_reason(format!("failed to unwrap layer: {}", e)))?;
+ Ok(Self(ctx.inner.layer(self.0.clone())))
+ }
+}
+
+/// A layer that will retry the request if it fails.
+/// It will retry with exponential backoff.
+///
+/// ## Parameters
+///
+/// - `jitter`<bool>: Whether to add jitter to the backoff.
+/// - `max_times`<number>: The maximum number of times to retry.
+/// - `factor`<number>: The exponential factor to use.
+/// - `max_delay`<number>: The maximum delay between retries. The unit is microsecond.
+/// - `min_delay`<number>: The minimum delay between retries. The unit is microsecond.
+#[napi]
+pub struct RetryLayer(opendal::layers::RetryLayer);
+
+impl NodeLayer for RetryLayer {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator {
+ op.layer(self.0.clone())
+ }
+}
+
+/// RetryLayer constructor.
+#[js_function(1)]
+pub fn create_retry_layer(ctx: CallContext) -> Result<JsObject> {
+ let mut retry = opendal::layers::RetryLayer::default();
+
+ let config: Either<JsObject, JsUndefined> = ctx.try_get::<JsObject>(0)?;
Review Comment:
Do we have better way to handle the default value?
##########
bindings/nodejs/src/lib.rs:
##########
@@ -15,6 +15,8 @@
// specific language governing permissions and limitations
// under the License.
+#![allow(clippy::not_unsafe_ptr_arg_deref)]
Review Comment:
Please mark them as unsafe instead of allow clippy.
##########
bindings/nodejs/src/lib.rs:
##########
@@ -686,6 +691,117 @@ impl PresignedRequest {
}
}
+pub trait NodeLayer: Send + Sync {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator;
+}
+
+struct NodeLayerWrapper {
+ inner: Box<dyn NodeLayer>,
+}
+
+#[napi]
+impl Operator {
+ /// Add a layer to this operator.
+ #[napi]
+ pub fn layer(&self, env: Env, layer: JsObject) -> Result<Self> {
+ let ctx: &mut NodeLayerWrapper = env
+ .unwrap(&layer)
+ .map_err(|e| Error::from_reason(format!("failed to unwrap layer: {}", e)))?;
+ Ok(Self(ctx.inner.layer(self.0.clone())))
+ }
+}
+
+/// A layer that will retry the request if it fails.
+/// It will retry with exponential backoff.
+///
+/// ## Parameters
+///
+/// - `jitter`<bool>: Whether to add jitter to the backoff.
+/// - `max_times`<number>: The maximum number of times to retry.
+/// - `factor`<number>: The exponential factor to use.
+/// - `max_delay`<number>: The maximum delay between retries. The unit is microsecond.
+/// - `min_delay`<number>: The minimum delay between retries. The unit is microsecond.
+#[napi]
+pub struct RetryLayer(opendal::layers::RetryLayer);
+
+impl NodeLayer for RetryLayer {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator {
+ op.layer(self.0.clone())
+ }
+}
+
+/// RetryLayer constructor.
+#[js_function(1)]
+pub fn create_retry_layer(ctx: CallContext) -> Result<JsObject> {
Review Comment:
The code in this function seems a bit verbose. Can we make it better?
##########
bindings/nodejs/index.js:
##########
@@ -20,6 +20,9 @@
/// <reference types="node" />
require('dotenv').config()
-const { Operator } = require('./generated.js')
+const { Operator, RetryLayer } = require('./generated.js')
module.exports.Operator = Operator
+module.exports.layers = {
Review Comment:
We don't need expose `Entry`, `Metadata` and `Lister`?
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1382513640
##########
bindings/nodejs/generated.js:
##########
@@ -271,10 +271,11 @@ if (!nativeBinding) {
throw new Error(`Failed to load native binding`)
}
-const { Operator, Entry, Metadata, Lister, BlockingLister } = nativeBinding
+const { Operator, Entry, Metadata, Lister, BlockingLister, RetryLayer } = nativeBinding
module.exports.Operator = Operator
module.exports.Entry = Entry
module.exports.Metadata = Metadata
module.exports.Lister = Lister
module.exports.BlockingLister = BlockingLister
+module.exports.RetryLayer = RetryLayer
Review Comment:
We will expose `RetryLayer` directly as `opendal`? Do we need a `opendal.layers`'s namespace?
##########
bindings/nodejs/tests/suites/index.mjs:
##########
@@ -36,7 +36,11 @@ export function runner(testName, scheme) {
config.root = generateRandomRoot(config.root)
}
- const operator = scheme ? new Operator(scheme, config) : null
+ let operator = scheme ? new Operator(scheme, config) : null
+
+ let retryLayer = new RetryLayer(true, 4)
Review Comment:
The API doesn't look good to me.
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1382451152
##########
.github/workflows/bindings_nodejs.yml:
##########
@@ -68,8 +66,8 @@ jobs:
- name: Check diff
run: git diff --exit-code
- - name: Unit test
- run: cargo nextest run --no-fail-fast
Review Comment:
Seems napi-rs error, FYI https://github.com/napi-rs/napi-rs/issues/1782
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#issuecomment-1807915733
Hi, @suyanhanx, here's my idea but I'm not well-versed in nodejs. Could you take it further and delve deeper into it?
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1392325189
##########
.github/workflows/bindings_nodejs.yml:
##########
@@ -65,12 +63,12 @@ jobs:
- name: Check format
run: yarn run prettier --check .
+ - name: Build
Review Comment:
Reverted to unit test.
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1392322776
##########
bindings/nodejs/src/lib.rs:
##########
@@ -15,6 +15,9 @@
// specific language governing permissions and limitations
// under the License.
+#![allow(clippy::not_unsafe_ptr_arg_deref)]
+/// This line exists here because of the `js_function` macro.
Review Comment:
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/nodejs): add retry layer [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#issuecomment-1807940264
> I see. I'll finish this.
Thanks a lot!
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1387764386
##########
bindings/nodejs/src/lib.rs:
##########
@@ -15,6 +15,8 @@
// specific language governing permissions and limitations
// under the License.
+#![allow(clippy::not_unsafe_ptr_arg_deref)]
Review Comment:
This unsafe warning is from a macro [here](https://github.com/apache/incubator-opendal/pull/3484/files#diff-ca4184f1ae4e7a2a05946c3af8bceb7ecb303597fa60ed20047820cacb707b85R734). I don't how to mark a macro as unsafe. And seems that it's impossible to mark a macro as unsafe now.
Marking the function as unsafe doesn't work. 🥲
##########
bindings/nodejs/src/lib.rs:
##########
@@ -15,6 +15,8 @@
// specific language governing permissions and limitations
// under the License.
+#![allow(clippy::not_unsafe_ptr_arg_deref)]
Review Comment:
This unsafe warning is from a macro [here](https://github.com/apache/incubator-opendal/pull/3484/files#diff-ca4184f1ae4e7a2a05946c3af8bceb7ecb303597fa60ed20047820cacb707b85R734). I don't how to mark a macro as unsafe. And seems that it's impossible to mark a macro as unsafe now.
Marking the function as unsafe doesn't work. 🥲
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1387771715
##########
bindings/nodejs/src/lib.rs:
##########
@@ -686,6 +691,117 @@ impl PresignedRequest {
}
}
+pub trait NodeLayer: Send + Sync {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator;
+}
+
+struct NodeLayerWrapper {
+ inner: Box<dyn NodeLayer>,
+}
+
+#[napi]
+impl Operator {
+ /// Add a layer to this operator.
+ #[napi]
+ pub fn layer(&self, env: Env, layer: JsObject) -> Result<Self> {
+ let ctx: &mut NodeLayerWrapper = env
+ .unwrap(&layer)
+ .map_err(|e| Error::from_reason(format!("failed to unwrap layer: {}", e)))?;
+ Ok(Self(ctx.inner.layer(self.0.clone())))
+ }
+}
+
+/// A layer that will retry the request if it fails.
+/// It will retry with exponential backoff.
+///
+/// ## Parameters
+///
+/// - `jitter`<bool>: Whether to add jitter to the backoff.
+/// - `max_times`<number>: The maximum number of times to retry.
+/// - `factor`<number>: The exponential factor to use.
+/// - `max_delay`<number>: The maximum delay between retries. The unit is microsecond.
+/// - `min_delay`<number>: The minimum delay between retries. The unit is microsecond.
+#[napi]
+pub struct RetryLayer(opendal::layers::RetryLayer);
+
+impl NodeLayer for RetryLayer {
+ fn layer(&self, op: opendal::Operator) -> opendal::Operator {
+ op.layer(self.0.clone())
+ }
+}
+
+/// RetryLayer constructor.
+#[js_function(1)]
+pub fn create_retry_layer(ctx: CallContext) -> Result<JsObject> {
+ let mut retry = opendal::layers::RetryLayer::default();
+
+ let config: Either<JsObject, JsUndefined> = ctx.try_get::<JsObject>(0)?;
Review Comment:
No. Since this is possible for users that they do not to pass parameters, object/undefined, these two types are incompatible, we have to handle them separately.
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#discussion_r1382354089
##########
.github/workflows/bindings_nodejs.yml:
##########
@@ -68,8 +66,8 @@ jobs:
- name: Check diff
run: git diff --exit-code
- - name: Unit test
- run: cargo nextest run --no-fail-fast
Review Comment:
https://github.com/apache/incubator-opendal/actions/runs/6753573761/job/18360177973?pr=3484
Can you please help me to find out what configuration is missing from the compilation?
I don't have an environment to be able to test it.
It's wired that running a command test will result in missing libs that fail to compile, but building it directly won't.
cc @messense
--
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/nodejs): add retry layer [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on PR #3484:
URL: https://github.com/apache/incubator-opendal/pull/3484#issuecomment-1793368003
It looks like irrelevant tests were triggered.
--
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