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/03/16 11:09:10 UTC

[GitHub] [incubator-opendal] suyanhanx opened a new pull request, #1646: feat(bindings/nodejs): Support Layer

suyanhanx opened a new pull request, #1646:
URL: https://github.com/apache/incubator-opendal/pull/1646

   Add Layer support


-- 
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


[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1646: feat(bindings/nodejs): Support Layer

Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1646:
URL: https://github.com/apache/incubator-opendal/pull/1646#discussion_r1138533180


##########
bindings/nodejs/src/layers.rs:
##########
@@ -0,0 +1,142 @@
+use napi::bindgen_prelude::*;
+use std::collections::HashMap;
+use std::str::FromStr;
+use std::time::Duration;
+use std::usize;
+
+/// Layer is the mechanism to intercept operations.
+#[derive(PartialEq, Eq)]
+#[napi]
+pub enum Layer {
+    /// Add concurrent request limit.
+    ///
+    /// Users can control how many concurrent connections could be established between OpenDAL and underlying storage services.
+    ///
+    /// - @param {number} permits
+    ///
+    /// Create a new ConcurrentLimitLayer will specify permits
+    ConcurrentLimit,
+
+    /// Add an immutable in-memory index for underlying storage services.
+    ///
+    /// Especially useful for services without list capability like HTTP.
+    ///
+    /// - @param {Array<string>} keys
+    ///
+    /// The keys which will be inserted into the index.
+    ///
+    /// Please use `JSON.stringfy` to serialize because of type restrictions.
+    ImmutableIndex,

Review Comment:
   I don't think this layer is worth export. Let's remove 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


[GitHub] [incubator-opendal] suyanhanx commented on a diff in pull request #1646: feat(bindings/nodejs): Support Layer

Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #1646:
URL: https://github.com/apache/incubator-opendal/pull/1646#discussion_r1138688489


##########
bindings/nodejs/src/layers.rs:
##########
@@ -0,0 +1,142 @@
+use napi::bindgen_prelude::*;
+use std::collections::HashMap;
+use std::str::FromStr;
+use std::time::Duration;
+use std::usize;
+
+/// Layer is the mechanism to intercept operations.
+#[derive(PartialEq, Eq)]
+#[napi]
+pub enum Layer {
+    /// Add concurrent request limit.
+    ///
+    /// Users can control how many concurrent connections could be established between OpenDAL and underlying storage services.
+    ///
+    /// - @param {number} permits
+    ///
+    /// Create a new ConcurrentLimitLayer will specify permits
+    ConcurrentLimit,
+
+    /// Add an immutable in-memory index for underlying storage services.
+    ///
+    /// Especially useful for services without list capability like HTTP.
+    ///
+    /// - @param {Array<string>} keys
+    ///
+    /// The keys which will be inserted into the index.
+    ///
+    /// Please use `JSON.stringfy` to serialize because of type restrictions.
+    ImmutableIndex,

Review Comment:
   Currently, it is not possible to directly export a struct containing a layer to be used by the JavaScript side. 
   I am exporting this enum in order to differentiate between layers. 
   Using other types, such as string, to specify the layer type doesn't seem very elegant. After all, the name of the layer type is quite long.🤔 



-- 
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


[GitHub] [incubator-opendal] Xuanwo commented on pull request #1646: feat(bindings/nodejs): Support Layer

Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #1646:
URL: https://github.com/apache/incubator-opendal/pull/1646#issuecomment-1473636795

   Since there have been no requests for layer support in the JS binding, I would rather take a step back and reconsider implementing layer support at the binding level.
   
   I will temporarily close this PR until we have a clear plan for improving 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


[GitHub] [incubator-opendal] Xuanwo closed pull request #1646: feat(bindings/nodejs): Support Layer

Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo closed pull request #1646: feat(bindings/nodejs): Support Layer
URL: https://github.com/apache/incubator-opendal/pull/1646


-- 
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