GSP-700: Config Features and DefaultPairs via Connetion String #700

JinnyYi · 2021-08-04T13:27:04Z

previous discussion: #680
demo: #699

codecov · 2021-08-04T13:27:55Z

Codecov Report

Merging #700 (cec079b) into master (de387ba) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #700   +/-   ##
=======================================
  Coverage   12.35%   12.35%           
=======================================
  Files          22       22           
  Lines        1441     1441           
=======================================
  Hits          178      178           
  Misses       1256     1256           
  Partials        7        7

Flag	Coverage Δ
unittests	`12.35% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de387ba...cec079b. Read the comment docs.

Xuanwo · 2021-08-04T15:49:01Z

Nice number！

Xuanwo · 2021-08-05T04:23:03Z

docs/rfcs/700-config-features-and-defaultpairs-via-connection-string.md

+
+`*Features` are types defined in service.
+
+**Feature Pairs Registry**


We already have features field in service.toml:

features = ["virtual_dir"]

Maybe we can reuse it?

Template that uses features can be found here: https://github.com/beyondstorage/go-storage/blob/master/cmd/definitions/tmpl/service.tmpl#L135-L146

Does it mean we can insert map["virtual_dir"] = "bool" into pairMap instead of generate *FeaturesPairMap?

https://github.com/beyondstorage/go-storage/blob/master/cmd/definitions/type.go#L283

Maybe we can append into New functions Optional directly?

Sorry for not understanding what you meant.
Maybe we still need to insert feature pairs into pairMap for registry, otherwise the feature name will not be recognized during parsing connection string?

Ok, I got it, we are discussing different things.

There are two things we need to support:

DefaultStorageClass pairs

default_storage_class in connection string.

For the first, we can add pairs into Optional, and for the other, we can add into pairMap.

Finally, we can reduce the usage of Feature Pairs Registry or Default Pairs Registry.

docs/rfcs/700-config-features-and-defaultpairs-via-connection-string.md

xxchan · 2021-08-05T05:43:39Z

docs/rfcs/700-config-features-and-defaultpairs-via-connection-string.md

+Also, we generate the following function for default pairs to support configure the default pair during initialization in the way of `WithXxx()`:
+
+Take `default_storage_class` for example:
+
+```go
+func WithDefaultStorageClass(v string) Pair {
+	reutrn Pair{
+		Key:   "default_storage_class",
+		Value: v,
+	}
+}
+```


What's its relationship with the old DefaultStoragePairs?

I think DefaultStoragePairs is more fine-grained compaired to WithDefaultStorageClass.
DefaultStoragePairs is an embeded struct contains a set of []Pair for supported operations. So users need to assign value to pairs of each operation one by one, even when different operations have the same pair.
For WithDefaultStorageClass, pairs that have the same name will share the same default value，users needn't to specify the operation.

The two kind of pairs can be used together (not orthogonal). Will it make users confused? Will they conflict with each other?

I found that there's no need to generate WithDefaultXxx() functions. Maybe we can just use the existing functions WithXxx() to set the default paris?

Will they conflict with each other?

When both WithDefault*Pairs() and WithXxx() are used, I think the values passed in by WithDefault*Pairs() should be picked (the priority is: pairs from args > WithDefault*Pairs() > WithXxx), and this can be and should be generated.

Maybe we can just use the existing functions WithXxx() to set the default paris?

I don't think so.

They have different semantic meanings.

WithStorageClass set current operation's storage class

WithDefaultStorageClass set current storage's default storage class.

So WithDefaultStorageClass is not a valid pair for Write operation, only valid for NewStorage operation. And WithStorageClass is valid for Write but not for NewStorage.

NOTES: there is no direct conversion between WithDefaultStorageClass and WithStorageClass. WithDefaultStorageClass configure the DefaultPairs and DefaultPairs will be expand in corresponding operations.

If loose_pair is enabled, they will be ignored, otherwise, we will raise not support pairs if used incorrectly.

And to the next question: How about using WithDefaultPairs and WithDefaultStorageClass at the same time?

We will pick DefaultPairs first, and append pairs that parsed from DefaultStorageClass.

Do you think it is necessary to explain the rationale why we need to add new pairs? Since the RFC title is about connection string.

docs/rfcs/700-config-features-and-defaultpairs-via-connection-string.md

Co-authored-by: xxchan <[email protected]>

Xuanwo · 2021-08-09T03:37:35Z

Let service specify default_xxx may be a huge burden. Think about s3, we have to write default_storage_class, default_excepted_bucket_owner and so on.

Maybe we can introduce new fields to help services simplify the work? I camp up to two ways:

Add new fields in operations

Like we discussed in #680, we can add a new field defaultable in operations. Paris that is listed in defaultable will be treated as allowing users to set default values. We can parse the toml files while generating and get a full list of all defaultable pairs, and then, generate related pairs.

[namespace.storage.op.write]
optional = ["content_md5", "content_type", "io_callback", "storage_class", "excepted_bucket_owner", "server_side_encryption_bucket_key_enabled", "server_side_encryption_customer_algorithm", "server_side_encryption_customer_key", "server_side_encryption_aws_kms_key_id", "server_side_encryption_context", "server_side_encryption"]
defaultable = ["content_type", "storage_class", "excepted_bucket_owner", ...]

Add new fields in the namespace

Add new fields in operations could still be too verbose. Think about the same pair in different operations:

[namespace.storage.op.read]
optional = ["offset", "io_callback", "size", "excepted_bucket_owner"]
defaultable = ["excepted_bucket_owner"]

[namespace.storage.op.write]
optional = ["content_md5", "content_type", "io_callback", "storage_class", "excepted_bucket_owner"]
defaultable = ["excepted_bucket_owner"]

So maybe we can add them in the namespace, alongside features and implement.

[namespace.storage]
features = ["virtual_dir"]
implement = ["direr", "multiparter"]
defaultable = ["excepted_bucket_owner"]

[namespace.storage.op.read]
optional = ["offset", "io_callback", "size", "excepted_bucket_owner"]

[namespace.storage.op.write]
optional = ["content_md5", "content_type", "io_callback", "storage_class", "excepted_bucket_owner"]

No matter which way we choose, services only need to specify once instead of list all pairs in the New function.

Xuanwo · 2021-08-09T03:41:14Z

docs/rfcs/700-config-features-and-defaultpairs-via-connection-string.md

+
+**Parse default pairs in `parsePair*New`**
+
+We can add private fields correspond to default pairs into the generated `Default*Pairs` to carry the shared values, like:


Maybe we can convert default_storage_class=xxxx/WithStoragClass("xxx") to pairs in DefaultStoragePairs directly?

We know storage_class is a defaultable pair

We know which operations support storage_class

JinnyYi · 2021-08-09T05:19:51Z

Maybe we can introduce new fields to help services simplify the work

Yes, and introduce new fields can specify the defaultable pairs more explicitly and visually.

For add new fields in operations, it's easy to check the validity of the defaultable pairs. We can even specify the defaultable paris on the operation level, other than the namespace level(all operations affected). But we need to traverse the operations' pair list once to collect all the defaultable paris.

For add new fields in the namespace, we can get all the defaultable pairs easily. But we also need to traverse the operations' pair list to check the validity of the pairs on namespace level.

I’ll try to add new fields in the namespace.

Xuanwo · 2021-08-10T04:35:24Z

docs/rfcs/700-config-features-and-defaultpairs-via-connection-string.md


 **Parse features in `parsePair*New`**

-We can handle the optional defaultable pairs with prefix `enable_`, assign value to the corresponding field of `*Features` in `ParsePair*New()`.
+We can handle the defaultable pairs for features with prefix `enable_`, assign value to the corresponding field of `*Features` in `ParsePair*New()`.


features will have WithEnableVirtualDir?

Maybe WithEnableVirtualDir() is not necessary？I intended to add default paris for features to reduce the usage of Feature Pairs Registry for connection string, so WithEnableVirtualDir() will be generated if no additional controls.

What does add default paris for features mean?

Like pairs for defaultable pairs. Names of these pairs are prefixed with enable_. Then the keys enable_xxx will be added into pairMap and can be parsed when using connection string.

OK, mostly LGTM.

docs/rfcs/700-config-features-and-defaultpairs-via-connection-string.md

…string.md

Xuanwo · 2021-08-10T08:38:53Z

I approve of this GSP mostly. Let's explain the rationale why we need to add new pairs as described in #700 (comment)

Xuanwo · 2021-08-10T09:34:02Z

Nice work!

GSP: Config Features and DefaultPairs via Connetion String

a5b8e31

assign num

f442c20

JinnyYi changed the title ~~GSP: Config Features and DefaultPairs via Connetion String~~ GSP-700: Config Features and DefaultPairs via Connetion String Aug 4, 2021

Xuanwo reviewed Aug 5, 2021

View reviewed changes

xxchan reviewed Aug 5, 2021

View reviewed changes

docs/rfcs/700-config-features-and-defaultpairs-via-connection-string.md Outdated Show resolved Hide resolved

xxchan reviewed Aug 5, 2021

View reviewed changes

JinnyYi commented Aug 5, 2021

View reviewed changes

docs/rfcs/700-config-features-and-defaultpairs-via-connection-string.md Show resolved Hide resolved

JinnyYi and others added 4 commits August 5, 2021 23:08

Update implementation

26b4ab4

Update Default*Pairs

295d8d4

Update default pairs format and implementation

cb8ef5e

Correct assignment

3b5ab31

Co-authored-by: xxchan <[email protected]>

Xuanwo mentioned this pull request Aug 9, 2021

Weekly report since 2021-08-02 beyondstorage/community#30

Closed

Xuanwo reviewed Aug 9, 2021

View reviewed changes

JinnyYi added 2 commits August 9, 2021 20:48

Add defaultable field to update implementation

d2c375e

Merge master into conn-string-for-feature-pair

6ae32ef

Xuanwo reviewed Aug 10, 2021

View reviewed changes

Xuanwo mentioned this pull request Aug 10, 2021

Tracking issues of GSP-700: Config Features and DefaultPairs via Connetion String #704

Closed

7 tasks

Xuanwo reviewed Aug 10, 2021

View reviewed changes

docs/rfcs/700-config-features-and-defaultpairs-via-connection-string.md Outdated Show resolved Hide resolved

Update docs/rfcs/700-config-features-and-defaultpairs-via-connection-…

df6ae67

…string.md

Add notes for new pairs

cec079b

Xuanwo approved these changes Aug 10, 2021

View reviewed changes

Xuanwo merged commit 3b28b1f into master Aug 10, 2021

Xuanwo deleted the conn-string-for-feature-pair branch August 10, 2021 09:33

Xuanwo mentioned this pull request Aug 16, 2021

Weekly report since 2021-08-09 beyondstorage/community#32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GSP-700: Config Features and DefaultPairs via Connetion String #700

GSP-700: Config Features and DefaultPairs via Connetion String #700

JinnyYi commented Aug 4, 2021

codecov bot commented Aug 4, 2021 •

edited

Loading

Xuanwo commented Aug 4, 2021

Xuanwo Aug 5, 2021

JinnyYi Aug 5, 2021

Xuanwo Aug 5, 2021

JinnyYi Aug 5, 2021 •

edited

Loading

Xuanwo Aug 6, 2021

xxchan Aug 5, 2021

JinnyYi Aug 5, 2021

xxchan Aug 5, 2021

JinnyYi Aug 5, 2021 •

edited

Loading

JinnyYi Aug 5, 2021

Xuanwo Aug 6, 2021

xxchan Aug 10, 2021

Xuanwo Aug 10, 2021

Xuanwo commented Aug 9, 2021

Xuanwo Aug 9, 2021

JinnyYi commented Aug 9, 2021

Xuanwo Aug 10, 2021

JinnyYi Aug 10, 2021

Xuanwo Aug 10, 2021

JinnyYi Aug 10, 2021

Xuanwo Aug 10, 2021

Xuanwo commented Aug 10, 2021

Xuanwo commented Aug 10, 2021


		`*Features` are types defined in service.

		Feature Pairs Registry


		*Parse default pairs in `parsePairNew`**

		We can add private fields correspond to default pairs into the generated `Default*Pairs` to carry the shared values, like:

GSP-700: Config Features and DefaultPairs via Connetion String #700

GSP-700: Config Features and DefaultPairs via Connetion String #700

Conversation

JinnyYi commented Aug 4, 2021

codecov bot commented Aug 4, 2021 • edited Loading

Codecov Report

Xuanwo commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinnyYi Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinnyYi Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xuanwo commented Aug 9, 2021

Add new fields in operations

Add new fields in the namespace

Choose a reason for hiding this comment

JinnyYi commented Aug 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xuanwo commented Aug 10, 2021

Xuanwo commented Aug 10, 2021

codecov bot commented Aug 4, 2021 •

edited

Loading

JinnyYi Aug 5, 2021 •

edited

Loading

JinnyYi Aug 5, 2021 •

edited

Loading