jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1399943173
##########
src/config/src/config.erl:
##########
@@ -252,7 +251,7 @@ init(IniFiles) ->
ets:new(?MODULE, [named_table, set, protected, {read_concurrency, true}]),
lists:map(
fun(IniFile) ->
- {ok, ParsedIniValues} = parse_ini_file(IniFile),
+ {ok, ParsedIniValues} = parse_ini_file(IniFile, fun delete_keys/1),
Review Comment:
> I think the docs would apply more if we only had two config files:
`default.ini` and `local.ini`, but since we support a hierarchy of files:
`default.ini, default.d/*.ini, local.ini, local.d/*.ini` it's a bit ambiguous.
Yeah, I think the docs need to be updated to reflect the possibility of more
`.ini` files. The change could be as simple as just:
"In case when you’d like to remove some parameter from an `.ini` file
without modifying that file, you may override it in a subsequent `.ini` file,
but without supplying any value following the equal sign:"
> What if the value in `default.ini` is 'D', in `local.ini` as `L` and in
`local.ini/1.ini` is a deleted entry? In the current main code we'd have a
discrepancy:
>
> * During startup we'd end up with `C`, the code default (`undefined`)
>
> * After reload we'd end with `L`, the one higher in the hierarchy
I believe this problem exists even with just a `default.ini` and `local.ini`
file. I performed a little experiment in a dev environment on main that also
showed a discrepancy between how `init` and `reload` treat delete markers. I
started by prepending a new section and values to these files:
```diff
diff --git a/rel/overlay/etc/default.ini b/rel/overlay/etc/default.ini
index 68ae900a3..c1461662e 100644
--- a/rel/overlay/etc/default.ini
+++ b/rel/overlay/etc/default.ini
@@ -1,3 +1,6 @@
+[section]
+file = default.ini
+
; Upgrading CouchDB will overwrite this file.
[vendor]
name = {{package_author_name}}
diff --git a/rel/overlay/etc/local.ini b/rel/overlay/etc/local.ini
index ba76ebfdf..bd8c88565 100644
--- a/rel/overlay/etc/local.ini
+++ b/rel/overlay/etc/local.ini
@@ -1,3 +1,6 @@
+[section]
+file = local.ini
+
; CouchDB Configuration Settings
; Custom settings should be made in this file. They will override settings
```
then in a remsh after starting CouchDB:
```
([email protected])2> config:get("section", "file").
"local.ini"
([email protected])3> config:delete("section", "file").
ok
```
in another terminal, confirming `local.ini` was changed as expected:
```
❯ head -2 dev/lib/node1/etc/local.ini
[section]
file =
```
Now, this `undefined` below is actually "correct":
```
([email protected])4> config:get("section", "file").
undefined
```
but for the wrong reason, whereas after a `reload`, it gives the wrong
answer:
```
([email protected])5> config:reload().
ok
([email protected])6> config:get("section", "file").
"default.ini"
```
since the `file =` line in `local.ini` should have overridden the default,
and made it `undefined`.
Lastly, I copied the modified `dev/lib/node1/etc/local.ini` to
`rel/overlay/etc/local.ini` to prevent it being overwritten, and restarted
CouchDB. Evidently, `config:init/1` correctly honors the delete marker:
```
([email protected])1> config:get("section", "file").
undefined
```
> It's seems complicated to reason about having startup and reload work
differently. The discrepancy itself seems like a bug.
I completely agree that it's hard to reason about, and clearly seems buggy
based on the above.
> I think it would be nice to at least fix it to where, absent any changes
to the .ini files, a start-up followed by any number of `reload` calls
shouldn't change the active config value.
Agree 100%
> In general, trying to think what is more useful from a user's point of
view:
>
> * They have a `default.ini` and `local.ini`. They set some value in
`local.ini`, say to increase `max_dbs_open = 1000` when their `default.ini` may
have it at `500`. Then they decide they'd like to revert back to default.ini's
value so they use the `delete` operation.
I think this behavior is reasonable, but I believe it would be a breaking
change in how we interpret delete markers.
> * Always revert to code defaults from any point in the config hierarchy.
I'm not clear on what you mean here ^. Would this be the effect of ignoring
delete markers?
> Maybe in the case their default.ini is read-only or broken and code
defaults are better. In that case, perhaps they could create another
`local.d/*.ini` and write the good value there.
I think the idea is that `default.ini` should always be treated as read-only
because we reserve the right to change it with upgrades. So we heavily
discourage admins from ever modifying `default.ini`, but instead make all
overrides to `local.ini`. And a delete marker in any subsequent `.ini` file is
a signifier that any previous value for that key should be ignored.
> (But if we do it this way, reload as is would behave inconsistently so we
have to make it behave similarly to init at least - always go back to code
default).
Yes, totally agree. I have attempted to do fix the discrepancy between
`init` and `reload` in the latest update to this PR.
> To me it seems the current reload behavior is more intuitive and the
startup behavior seems surprising.
Clearly the way `init` was making `ets:delete` calls when it hit delete
markers was hard to reason about, and especially made the code hard to re-use.
The main recent changes are:
1. Implement `ini_map/1` leveraging the built in `maps:merge` to simplify
parsing and merging multiple `.ini` files.
1. Make delete markers first class concepts, and expose their existence to
any caller of `ini_map/1` so it can handle them accordingly.
1. Improve consistency of delete marker handling for `reload`, `init`, and
`delete` using `ini_map/1`.
It could use some unit tests, but manual testing seems to now be giving
consistent results.
##########
src/config/src/config.erl:
##########
@@ -252,7 +251,7 @@ init(IniFiles) ->
ets:new(?MODULE, [named_table, set, protected, {read_concurrency, true}]),
lists:map(
fun(IniFile) ->
- {ok, ParsedIniValues} = parse_ini_file(IniFile),
+ {ok, ParsedIniValues} = parse_ini_file(IniFile, fun delete_keys/1),
Review Comment:
> I think the docs would apply more if we only had two config files:
`default.ini` and `local.ini`, but since we support a hierarchy of files:
`default.ini, default.d/*.ini, local.ini, local.d/*.ini` it's a bit ambiguous.
Yeah, I think the docs need to be updated to reflect the possibility of more
`.ini` files. The change could be as simple as just:
"In case when you’d like to remove some parameter from an `.ini` file
without modifying that file, you may override it in a subsequent `.ini` file,
but without supplying any value following the equal sign:"
> What if the value in `default.ini` is 'D', in `local.ini` as `L` and in
`local.ini/1.ini` is a deleted entry? In the current main code we'd have a
discrepancy:
>
> * During startup we'd end up with `C`, the code default (`undefined`)
>
> * After reload we'd end with `L`, the one higher in the hierarchy
I believe this problem exists even with just a `default.ini` and `local.ini`
file. I performed a little experiment in a dev environment on main that also
showed a discrepancy between how `init` and `reload` treat delete markers. I
started by prepending a new section and values to these files:
```diff
diff --git a/rel/overlay/etc/default.ini b/rel/overlay/etc/default.ini
index 68ae900a3..c1461662e 100644
--- a/rel/overlay/etc/default.ini
+++ b/rel/overlay/etc/default.ini
@@ -1,3 +1,6 @@
+[section]
+file = default.ini
+
; Upgrading CouchDB will overwrite this file.
[vendor]
name = {{package_author_name}}
diff --git a/rel/overlay/etc/local.ini b/rel/overlay/etc/local.ini
index ba76ebfdf..bd8c88565 100644
--- a/rel/overlay/etc/local.ini
+++ b/rel/overlay/etc/local.ini
@@ -1,3 +1,6 @@
+[section]
+file = local.ini
+
; CouchDB Configuration Settings
; Custom settings should be made in this file. They will override settings
```
then in a remsh after starting CouchDB:
```
([email protected])2> config:get("section", "file").
"local.ini"
([email protected])3> config:delete("section", "file").
ok
```
in another terminal, confirming `local.ini` was changed as expected:
```
❯ head -2 dev/lib/node1/etc/local.ini
[section]
file =
```
Now, this `undefined` below is actually "correct":
```
([email protected])4> config:get("section", "file").
undefined
```
but for the wrong reason, whereas after a `reload`, it gives the wrong
answer:
```
([email protected])5> config:reload().
ok
([email protected])6> config:get("section", "file").
"default.ini"
```
since the `file =` line in `local.ini` should have overridden the default,
and made it `undefined`.
Lastly, I copied the modified `dev/lib/node1/etc/local.ini` to
`rel/overlay/etc/local.ini` to prevent it being overwritten, and restarted
CouchDB. Evidently, `config:init/1` correctly honors the delete marker:
```
([email protected])1> config:get("section", "file").
undefined
```
> It's seems complicated to reason about having startup and reload work
differently. The discrepancy itself seems like a bug.
I completely agree that it's hard to reason about, and clearly seems buggy
based on the above.
> I think it would be nice to at least fix it to where, absent any changes
to the .ini files, a start-up followed by any number of `reload` calls
shouldn't change the active config value.
Agree 100%
> In general, trying to think what is more useful from a user's point of
view:
>
> * They have a `default.ini` and `local.ini`. They set some value in
`local.ini`, say to increase `max_dbs_open = 1000` when their `default.ini` may
have it at `500`. Then they decide they'd like to revert back to default.ini's
value so they use the `delete` operation.
I think this behavior is reasonable, but I believe it would be a breaking
change in how we interpret delete markers.
> * Always revert to code defaults from any point in the config hierarchy.
I'm not clear on what you mean here ^. Would this be the effect of ignoring
delete markers?
> Maybe in the case their default.ini is read-only or broken and code
defaults are better. In that case, perhaps they could create another
`local.d/*.ini` and write the good value there.
I think the idea is that `default.ini` should always be treated as read-only
because we reserve the right to change it with upgrades. So we heavily
discourage admins from ever modifying `default.ini`, but instead make all
overrides to `local.ini`. And a delete marker in any subsequent `.ini` file is
a signifier that any previous value for that key should be ignored.
> (But if we do it this way, reload as is would behave inconsistently so we
have to make it behave similarly to init at least - always go back to code
default).
Yes, totally agree. I have attempted to do fix the discrepancy between
`init` and `reload` in the latest update to this PR.
> To me it seems the current reload behavior is more intuitive and the
startup behavior seems surprising.
Clearly the way `init` was making `ets:delete` calls when it hit delete
markers was hard to reason about, and especially made the code hard to re-use.
The main recent changes are:
1. Implement `ini_map/1` leveraging the built in `maps:merge` to simplify
parsing and merging multiple `.ini` files.
1. Make delete markers first class concepts, and expose their existence to
any caller of `ini_map/1` so it can handle them accordingly.
1. Improve consistency of delete marker handling for `reload`, `init`, and
`delete` using `ini_map/1`.
It could use some unit tests, but manual testing seems to now be giving
consistent results.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
