You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@opendal.apache.org by GitBox <gi...@apache.org> on 2023/04/11 06:57:59 UTC

[GitHub] [incubator-opendal] Ji-Xinyou created a discussion: The map construction in opendal's C binding

GitHub user Ji-Xinyou created a discussion: The map construction in opendal's C binding

In opendal, we use a `HashMap<String, String>` to support our init logic. But since `HashMap` is pretty complex to express and not a C native type, we need to be careful about it.

I currently have several ideas on how the C binding could construct the required `HashMap`, and really appreciate any suggestions or ideas on this topic.

1. Do not expose any `HashMap` thing to the user, the API will be like the following
```C
opendal_operator_ptr opendal_operator_new(char *scheme, char **map_keys, char **map_values);
```
This API will reads keys and values and make a `HashMap` inside Rust code without exposing to users, but this will require user to make sure that the `keys` and `values` are in correct order. Moreover, the parameters are with less semantics.
e.g.
```C
char keys[][] = {"k1", "k2"};
char values[][] = {"v1", "v2"};
```
is ok but
```C
char keys[][] = {"k1", "k2"};
char values[][] = {"v2", "v1"};
```
is semantically not ok and often not detectable.

And
```C
char keys[][] = {"k1", "k2", "k3"};
char values[][] = {"v1", "v2"};
```
is not grammarly ok.

2. Expose some of the `HashMap` by providing API like
```C
opendal_map m = opendal_map_new(char **keys, char **values);
```
To control the behavior of opendal_map, we can expose limited API.
The disadvantage of this will be opendal C binding will be less native, since it relies on part of the Rust std, and the advantage will be it will allow for further operation like `read` and `insert` to it.

Again, I would like to hear everyone's opinions on the things above or any other possible implementation.

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Ji-Xinyou edited a comment on the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Ji-Xinyou edited a comment on the discussion: The map construction in opendal's C binding

> > Also, different backends use different KVs, if we are using the AWS way, we accept params like `opendal_operatior_config`.
> 
> Every service will have their own builder like `opendal_service_s3_builder`. The hashmap thing is an easier way to build a `S3Builder`. And finally, we will use the builder to build the service.

So what about this way of API.

Builders:
```C
opendal_builder_fs opendal_builder_fs_new(struct opendal_builder_fs_config);
opendal_builder_memory opendal_builder_memory_new(struct opendal_builder_memory_config);
```

Operators:
```C
opendal_operator_ptr opendal_operator_new(char *scheme, void *builder);
```

And in runtime we cast the `void*` to what we want (the C way generics lol).

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5577577

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Xuanwo added a comment to the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Xuanwo added a comment to the discussion: The map construction in opendal's C binding

> which way do you think is better, the config way or the function with pointer way.

Let's use `the function with pointer` for now.

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5577828

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Xuanwo added a comment to the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Xuanwo added a comment to the discussion: The map construction in opendal's C binding

> In opendal, we use a HashMap<String, String> to support our init logic.

This conclusion is incorrect. Using a hashmap for initialization has never been considered as our primary method. `nodejs` and `python` init operator in this way because they are dynamic language.

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5577258

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Ji-Xinyou added a comment to the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Ji-Xinyou added a comment to the discussion: The map construction in opendal's C binding

> > Also, different backends use different KVs, if we are using the AWS way, we accept params like `opendal_operatior_config`.
> 
> Every service will have their own builder like `opendal_service_s3_builder`. The hashmap thing is an easier way to build a `S3Builder`. And finally, we will use the builder to build the service.

So what about this way of API.

Builders:
```C
opendal_builder_fs opendal_builder_fs_new(struct opendal_builder_fs_config);
opendal_builder_memory opendal_builder_memory_new(struct opendal_builder_memory_config);
```

Operators:
```
opendal_operator_ptr opendal_operator_new(char *scheme, void *builder);
```

And in runtime we cast the `void*` to what we want (the C way generics lol).

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5577577

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Xuanwo edited a comment on the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Xuanwo edited a comment on the discussion: The map construction in opendal's C binding

Maybe we don't need to have both `builder` and `config`. Only keep `builder` like what we do for `services` LGTM.

Builders:

```c
opendal_builder_fs opendal_builder_fs_new();
opendal_builder_fs_set_root(struct opendal_builder_fs *builder);

opendal_builder_memory opendal_builder_memory_new();
```

Operator:

```c
opendal_operator_ptr opendal_operator_new(char *scheme, void *builder);
```

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5577599

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Xuanwo added a comment to the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Xuanwo added a comment to the discussion: The map construction in opendal's C binding

Maybe we don't need to have both `builder` and `config`. Only keep `builder` like what we do for `services` LGTM.

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5577599

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Xuanwo added a comment to the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Xuanwo added a comment to the discussion: The map construction in opendal's C binding

> Also, different backends use different KVs, if we are using the AWS way, we accept params like `opendal_operatior_config`.

Every service will have their own config like `opendal_service_s3_config`.

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5577429

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Ji-Xinyou added a comment to the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Ji-Xinyou added a comment to the discussion: The map construction in opendal's C binding

I saw that in `core`, `OperatorBuilder` either uses a `HashMap` (or an iterator collected) or uses env var.

Also, different backends use different KVs, if we are using the AWS way, we accept params like `opendal_operatior_config`.
This has a problem that
* Suppose we are initing a Fs backend, we only need `root` and `atomic_write_dir`. But for other backends, we need many other keys. If we accept a unified param like `opendal_operator_config`, all these fields have to reside in this one struct.

like
```C
struct opendal_operator_config {
    char* root,
    char* atomic_write_dir,
    char* filesystem,
    char* endpoint,
    char* account_name,
    .....
}
```

I am worrying that if some fields are set, like `filesystem`, but the fields are not for the backend they are using (e.g. `filesystem` is not used by `Fs` backend), this might introduce some confusion.

WDYT?

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5577399

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Ji-Xinyou added a comment to the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Ji-Xinyou added a comment to the discussion: The map construction in opendal's C binding

That reply was before your edited comment, I got your point now, which way do you think is better, the config way or the function with pointer way.

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5577696

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Xuanwo edited a comment on the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Xuanwo edited a comment on the discussion: The map construction in opendal's C binding

> Also, different backends use different KVs, if we are using the AWS way, we accept params like `opendal_operatior_config`.

Every service will have their own builder like `opendal_service_s3_builder`. The hashmap thing is an easier way to build a `S3Builder`. And finally, we will use the builder to build the service.

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5577429

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Xuanwo edited a comment on the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Xuanwo edited a comment on the discussion: The map construction in opendal's C binding

> Also, different backends use different KVs, if we are using the AWS way, we accept params like `opendal_operatior_config`.

Every service will have their own builder like `opendal_service_s3_builder`.

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5577429

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Ji-Xinyou added a comment to the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Ji-Xinyou added a comment to the discussion: The map construction in opendal's C binding

Then the discussion could be closed.
We all agree on the `builder` pattern and `function with pointer` way of populating the `builder`.

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5601443

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Xuanwo added a comment to the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Xuanwo added a comment to the discussion: The map construction in opendal's C binding

We don't need to support map constrution for c binding. I expect to see something like AWS SDK CPP:

```cpp
Aws::Client::ClientConfiguration clientConfig;
clientConfig.proxyHost = "localhost";
clientConfig.proxyPort = 1234;
clientConfig.proxyScheme = Aws::Http::Scheme::HTTPS;
Aws::S3::S3Client(clientConfig);
```

Since we have so many binding and services to support, maybe we should do some codegen work.

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5577248

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org

[GitHub] [incubator-opendal] Ji-Xinyou added a comment to the discussion: The map construction in opendal's C binding

Posted by GitBox <gi...@apache.org>.

GitHub user Ji-Xinyou added a comment to the discussion: The map construction in opendal's C binding

> Maybe we don't need to have both `builder` and `config`. Only keep `builder` like what we do for `services` LGTM.

So you mean like
```C
opendal_builder_fs builder = opendal_builder_fs_new()
   .root(char* root)
   .atomic_write_dir(char* root);
```

This seems not possible in C since it is not Object Oriented, no `self` is provided nor encapsulation.
If you are curious about this, you could take [this](https://stackoverflow.com/questions/32347910/oop-in-c-implicitly-pass-self-as-parameter) to have a look.

Apart from the `opendal_builder_fs_config` way, the following is possible.
```C
opendal_builder_fs opendal_builder_fs_new();
void opendal_builder_fs_root(opendal_builder_fs *builder);
void opendal_builder_fs_atomic_write_dir(opendal_builder_fs *builder);
```

I think both are acceptable, but the first one is more light weight.

GitHub link: https://github.com/apache/incubator-opendal/discussions/1904#discussioncomment-5577680

----
This is an automatically sent email for commits@opendal.apache.org.
To unsubscribe, please send an email to: commits-unsubscribe@opendal.apache.org