nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1372634840
##########
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:
Hmm, good point. 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. 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
It's seems complicated to reason about having startup and reload work
differently. The discrepancy itself seems like a bug. 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.
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.
* Always revert to code defaults from any point in the config hierarchy.
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. (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).
To me it seems the current reload behavior is more intuitive and the startup
behavior seems surprising.
The current behavior during init seems like a `reset` instead of a `delete`.
The behavior of reload, in fact doesn't even need the silly `delete` marker. It
could just delete the line from the file and it would end up with the same
effect. But it doesn't seem like the
[config_writer](https://github.com/apache/couchdb/blob/main/src/config/src/config_writer.erl)
has any way of actually doing that currently
--
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]
