You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "jaydoane (via GitHub)" <gi...@apache.org> on 2023/10/19 02:29:02 UTC
[PR] Config delete reload [couchdb]
jaydoane opened a new pull request, #4813:
URL: https://github.com/apache/couchdb/pull/4813
<!-- Thank you for your contribution!
Please file this form by replacing the Markdown comments
with your text. If a section needs no action - remove it.
Also remember, that CouchDB uses the Review-Then-Commit (RTC) model
of code collaboration. Positive feedback is represented +1 from committers
and negative is a -1. The -1 also means veto, and needs to be addressed
to proceed. Once there are no objections, the PR can be merged by a
CouchDB committer.
See: http://couchdb.apache.org/bylaws.html#decisions for more info. -->
## Overview
Automatically reload disk default after config deletion.
<!-- Please give a short brief for the pull request,
what problem it solves or how it makes things better. -->
## Testing recommendations
<!-- Describe how we can test your changes.
Does it provide any behaviour that the end users
could notice? -->
## Related Issues or Pull Requests
<!-- If your changes affect multiple components in different
repositories please put links to those issues or pull requests here. -->
## Checklist
- [ ] Code is written and works correctly
- [ ] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] Documentation changes were made in the `src/docs` folder
- [ ] Documentation changes were backported (separated PR) to affected branches
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1418375792
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
I think in all cases? The only effect of `Persist=false` would be that the value is not written to file. So ets:delete from then call "load" and whatever "load" does is what we end up with?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1406526840
##########
src/config/test/config_tests.erl:
##########
@@ -115,6 +111,41 @@ handle_config_terminate(Self, Reason, {Pid, State}) ->
Pid ! {config_msg, {Self, Reason, State}},
ok.
+setup_ini(IniFiles) ->
+ Chain = [write_ini(F) || F <- IniFiles],
+ {Chain, setup(Chain)}.
+
+teardown_ini(_, {Chain, Apps}) ->
+ lists:foreach(
+ fun(Path) -> ok = file:delete(Path) end,
+ Chain
+ ),
+ teardown(Apps).
+
+write_ini(FileName) ->
+ Path = filename:join([?TEMPDIR, FileName]),
+ Data = io_lib:format("[section]\nfile = ~s\n", [FileName]),
+ ok = file:write_file(Path, Data),
+ Path.
+
+config_delete_reload_restart_test_() ->
+ {
+ "Test consistency after set, delete, reload, and restart",
+ foreachx,
+ fun setup_ini/1,
+ fun teardown_ini/2,
+ [
+ {
+ ["default.ini", "local.ini"],
+ fun non_persistent_set_delete_reload_restart/2
Review Comment:
Those tests originally did just use `foreach`, and the generated `.ini` files were hardcoded in `setup_ini`. But then I was curious about whether persistent `set` and `delete` variations would also write to an `.ini` file if there was only one, so I changed it to `foreachx` in order to factor out the config chain. Turns out that because of how `write_filename` is defined:
```erlang
WriteFile =
case IniFiles of
[_ | _] -> lists:last(IniFiles);
_ -> undefined
end,
```
that it does indeed write to a single `.ini` file configuration (further evidence that `default.ini` is not a great name).
So maybe I should just change it back now that my experiment has concluded there's no real edge case for just a single `.ini` config?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1364906897
##########
src/config/src/config.erl:
##########
@@ -326,6 +326,13 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
"~p: [~s] ~s deleted for reason ~p",
[?MODULE, Sec, Key, Reason]
),
+ DiskKVs = parse_ini_files(Config#config.ini_files),
+ case dict:find({Sec, Key}, DiskKVs) of
+ {ok, DiskVal} ->
+ true = ets:insert(?MODULE, {{Sec, Key}, DiskVal});
+ error ->
+ ok
+ end,
Review Comment:
Good idea, makes sense. Fixed in https://github.com/apache/couchdb/pull/4813/commits/bb0eb8fd3064ef8eaa7fc6a633e9d825e046427e
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1372580930
##########
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:
The only one I'm not entirely sure about is when there's a value in `default.ini`, but `local.ini` has a delete marker. Shouldn't row 3 column 2 be the same as row 3 column 1, based on this paragraph in [the docs](https://docs.couchdb.org/en/stable/config/intro.html#setting-parameters-via-the-configuration-file)?
> In case when you’d like to remove some parameter from the default.ini without modifying that file, you may override in local.ini, but without any value
```
[compactions]
_default =
```
But maybe I'm misunderstanding your table and/or the docs.
FWIW, it seems like eunit and elixir tests continue to pass if we replace `delete_keys/1` with `omit_delete/1` in the `init` function.
##########
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:
The only one I'm not entirely sure about is when there's a value in `default.ini`, but `local.ini` has a delete marker. Shouldn't row 3 column 2 be the same as row 3 column 1, based on this paragraph in [the docs](https://docs.couchdb.org/en/stable/config/intro.html#setting-parameters-via-the-configuration-file)?
> In case when you’d like to remove some parameter from the default.ini without modifying that file, you may override in local.ini, but without any value
```
[compactions]
_default =
```
But maybe I'm misunderstanding your table and/or the docs.
FWIW, it seems like eunit and elixir tests continue to pass if we replace `delete_keys/1` with `omit_delete/1` in the `init` function.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1418434390
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
https://github.com/apache/couchdb/pull/4813/commits/6850044dedcfd5a9169f72c31dbf0e80c58b1164 reloads for both kinds of `config:delete`, and IMO the tests seem more consistent.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1418362758
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
> or after a delete call
But only after a persistent delete, right?
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
> or after a delete call
But only after a persistent delete, right?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1408661762
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
I wonder how this behavior would work?
If delete marker is not persisted, it seems it should only affect whether it gets written to the last file in the chain or not, otherwise the "live" value should be the same if persisted or not persisted case. With the new behavior, it seems, if `Persisted = false` we'd actually load the last non-deleted value from the .ini chain.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1408705136
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
The goal of the `Persist = false` I think would be to do everything the same as `Persist = true` except write to file. That seems like the simplest description of its behavior. The immediate effects on the "live" config value should be exactly the same, so if a user does:
`config:set("section", "key", "value", true)` or `config:set("section", "key", "value", false)`, the live value would be `"value"`. If they do `config:delete("section", "key", true)` or `config:delete("section", "key", false)` the live value would be the code default (config returns `undefined`).
After a restart or reload, the live value may change (it may be whatever was in the files) but that's something the user explicitly requested by using he `Persist = false` modifier.
Only in the `Persist = true` (the default) case, we'd want to have idempotence between any number of restarts or reloads after setting or deleting a value.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#issuecomment-1830331846
> bit concerned about some unrelated changes (the is_sensitive commit particularly) but they all seem fine. suggest updating the PR text/title to clarify this does more than currently stated, to help us in the future.
There are actually many "unrelated" changes to clean up the code, and generally make it easier to understand. The handling of sensitive values in particular took up a lot of room, and IMO obscured the core logic. I did make sure that all those changes are covered by tests.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1418375792
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
I think in all cases? The only effect of `Persist=false` would be that the value is not written to file. So ets:delete then call "load" and whatever "load" does is what we end up with?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1408113407
##########
src/config/src/config.erl:
##########
@@ -324,26 +337,15 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
{reply, Else, Config}
end;
handle_call(reload, _From, Config) ->
- DiskKVs = lists:foldl(
- fun(IniFile, DiskKVs0) ->
- {ok, ParsedIniValues} = parse_ini_file(IniFile),
- lists:foldl(
- fun({K, V}, DiskKVs1) ->
- dict:store(K, V, DiskKVs1)
- end,
- DiskKVs0,
- ParsedIniValues
- )
- end,
- dict:new(),
- Config#config.ini_files
- ),
- % Update ets with anything we just read
- % from disk
- dict:fold(
- fun({Sec, Key} = K, V, _) ->
+ IniMap = ini_map(Config#config.ini_files),
+ % Update ets with list values, and delete those with ?DELETE markers.
+ maps:foreach(
+ fun({Sec, Key} = K, V) ->
VExisting = get(Sec, Key, V),
- ets:insert(?MODULE, {K, V}),
+ case V =:= ?DELETE of
Review Comment:
Simplified to remove `=:=` in https://github.com/apache/couchdb/pull/4813/commits/44c3179bab08ff28aa07ad618944c8a2cc8a8bde
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1408677840
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
With the current behavior, since we don't reload anything, the value is `undefined` following a delete (until reload or restart ofc).
With the new behavior, in the `Persist == true` case, it's still `undefined` due to the delete marker, so the only change is for `Persist == false` where, as you point out, it reloads the last non deleted .ini value. I demonstrated that behavior in this test:
```erlang
non_persistent_set_delete_reload_restart() ->
Persist = false,
?assertEqual("local.ini", config:get("section", "file")),
?assertEqual(ok, config:set("section", "file", "memory", Persist)),
?assertEqual("memory", config:get("section", "file")),
?assertEqual(ok, config:delete("section", "file", Persist)),
?assertEqual("local.ini", config:get("section", "file")),
```
It may seem odd, but at least it's consistent now, since neither case changes after reload or restart.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1372262028
##########
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 wonder if with the new behavior the initial load logic can be the same as the reload. Could we always call parse_init_files with the omit logic being the default.
I tried to go through a few possible cases in a table:
| | | | | |
| ------------ | - | - | - | - |
| default.ini | X | D | D | X |
| local.ini | X | X | L | L |
| active value | C | D | L | L |
(C - code default, X - delete marker, D, L - default and local values)
What do you think?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1420715270
##########
src/config/src/config.erl:
##########
@@ -340,54 +331,30 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
{reply, Else, Config}
end;
handle_call(reload, _From, Config) ->
- DiskKVs = lists:foldl(
- fun(IniFile, DiskKVs0) ->
- {ok, ParsedIniValues} = parse_ini_file(IniFile),
- lists:foldl(
- fun({K, V}, DiskKVs1) ->
- dict:store(K, V, DiskKVs1)
- end,
- DiskKVs0,
- ParsedIniValues
- )
- end,
- dict:new(),
- Config#config.ini_files
- ),
- % Update ets with anything we just read
- % from disk
- dict:fold(
- fun({Sec, Key} = K, V, _) ->
+ % Update ets with ini values.
+ IniMap = ini_map(Config#config.ini_files),
+ maps:foreach(
+ fun({Sec, Key} = K, V) ->
VExisting = get(Sec, Key, V),
- ets:insert(?MODULE, {K, V}),
+ true = ets:insert(?MODULE, {K, V}),
case V =:= VExisting of
true ->
ok;
false ->
- case is_sensitive(Sec, Key) of
- false ->
- couch_log:notice(
- "Reload detected config change ~s.~s = ~p",
- [Sec, Key, V]
- );
- true ->
- couch_log:notice(
- "Reload detected config change ~s.~s = '****'",
- [Sec, Key]
- )
- end,
+ couch_log:notice(
+ "Reload detected config change ~s.~s = ~p",
+ [Sec, Key, maybe_conceal(V, is_sensitive(Sec, Key))]
+ ),
Event = {config_change, Sec, Key, V, true},
gen_event:sync_notify(config_event, Event)
end
end,
- nil,
- DiskKVs
+ IniMap
),
- % And remove anything in ets that wasn't
- % on disk.
+ % And remove anything in ets that wasn't on disk.
ets:foldl(
fun({{Sec, Key} = K, _}, _) ->
- case dict:is_key(K, DiskKVs) of
+ case maps:is_key(K, IniMap) of
Review Comment:
minor hint: there is https://www.erlang.org/doc/man/erlang#is_map_key-2 and is can be used as a guard if you think it would work better that way. it might avoid having an extra indentation level for the `case`
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1418457489
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
> It's definitely cleaner code to do it in both cases, but I thought you had previously argued against reloading .ini files in the case of `Persist=false`?
I think it depends if we stay with delete markers. If we stay with delete markers, then on `Persist=false` it doesn't matter if we reload, the effective live value should still be `undefined` (reload then ets:delete is the same as skipping reloading and just doing ets:delete).
If we get rid of delete markers, then reload/load/delete/set on delete would all run "load" to determine the live value. Persist is just a modifier whether we write to file or not before running "load". There are additional bits in case of reload to make sure to delete the ets values which now may have changed. There is the notification bit as well.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#issuecomment-1839122091
hm, that's unfortunate, but not a good enough reason not fix a bug imo.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1406958164
##########
src/config/test/config_tests.erl:
##########
@@ -115,6 +111,41 @@ handle_config_terminate(Self, Reason, {Pid, State}) ->
Pid ! {config_msg, {Self, Reason, State}},
ok.
+setup_ini(IniFiles) ->
+ Chain = [write_ini(F) || F <- IniFiles],
+ {Chain, setup(Chain)}.
+
+teardown_ini(_, {Chain, Apps}) ->
+ lists:foreach(
+ fun(Path) -> ok = file:delete(Path) end,
+ Chain
+ ),
+ teardown(Apps).
+
+write_ini(FileName) ->
+ Path = filename:join([?TEMPDIR, FileName]),
+ Data = io_lib:format("[section]\nfile = ~s\n", [FileName]),
+ ok = file:write_file(Path, Data),
+ Path.
+
+config_delete_reload_restart_test_() ->
+ {
+ "Test consistency after set, delete, reload, and restart",
+ foreachx,
+ fun setup_ini/1,
+ fun teardown_ini/2,
+ [
+ {
+ ["default.ini", "local.ini"],
+ fun non_persistent_set_delete_reload_restart/2
Review Comment:
> My main beef with `foreachx` is that it becomes another eunit quirk one has to learn to be able add add or fix failing tests.
I find it annoying that it requires `Pairs::[{X::any(), ((X::any(), R::any()) -> Tests)}]` instead of (say) an `Instantiator` that returns `Pairs`. But, yeah, I agree we can keep its use to a minimum. I've changed those tests back to `foreach`.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1406491018
##########
src/config/test/config_tests.erl:
##########
@@ -115,6 +111,41 @@ handle_config_terminate(Self, Reason, {Pid, State}) ->
Pid ! {config_msg, {Self, Reason, State}},
ok.
+setup_ini(IniFiles) ->
+ Chain = [write_ini(F) || F <- IniFiles],
+ {Chain, setup(Chain)}.
+
+teardown_ini(_, {Chain, Apps}) ->
+ lists:foreach(
+ fun(Path) -> ok = file:delete(Path) end,
+ Chain
+ ),
+ teardown(Apps).
+
+write_ini(FileName) ->
+ Path = filename:join([?TEMPDIR, FileName]),
+ Data = io_lib:format("[section]\nfile = ~s\n", [FileName]),
+ ok = file:write_file(Path, Data),
+ Path.
+
+config_delete_reload_restart_test_() ->
+ {
+ "Test consistency after set, delete, reload, and restart",
+ foreachx,
+ fun setup_ini/1,
+ fun teardown_ini/2,
+ [
+ {
+ ["default.ini", "local.ini"],
+ fun non_persistent_set_delete_reload_restart/2
Review Comment:
I tried adding these lines to `couch_eunit.hrl`:
```erlang
-ifndef(TDEF_FEX).
-define(TDEF_FEX(Name), fun(R, X) -> {atom_to_list(Name), ?_test(Name(R, X))} end).
-define(TDEF_FEX(Name, Timeout), fun(R, X) -> {atom_to_list(Name), {timeout, Timeout, ?_test(Name(R, X))}} end).
-endif.
```
and rewrote these new tests to use it.
However, as I worked on converting the _rest_ of this file to use `TDEF_FEX` in the `foreachx` tests, I realized it might take a bit more effort with tests like e.g. `should_notify_on_config_reload` which don't seem to lend themselves to a simple conversion. I will continue to ponder the best approach, and would welcome advice.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1418342451
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
With the change to eliminate delete markers, it seems that reloading any remaining .ini values is probably the most consistent thing to do.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1418457489
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
> It's definitely cleaner code to do it in both cases, but I thought you had previously argued against reloading .ini files in the case of `Persist=false`?
I think it depends if we stay with delete markers. If we stay with delete markers, then on `Persist=false` it doesn't matter if we reload, the effective live value should still be an `undefined` (reload then still ets:delete is the same as skipping reloading and just doing ets:delete).
If we get rid of delete markers, then reload/load/delete/set would all run "load" to determine the live value. Persist is just a modifier whether we write to file or not before running "load". There are additional bits in case of reload to make sure to delete the ets values which now many have changed. There is notification bit as well.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1372567049
##########
src/config/test/config_tests.erl:
##########
@@ -441,8 +437,8 @@ should_write_changes(_, _) ->
?assertEqual("5986", config:get("httpd", "port")),
?assertEqual(ok, config:set("httpd", "port", "8080")),
?assertEqual("8080", config:get("httpd", "port")),
- ?assertEqual(ok, config:delete("httpd", "bind_address", "8080")),
- ?assertEqual(undefined, config:get("httpd", "bind_address"))
+ ?assertEqual(ok, config:delete("httpd", "bind_address")),
+ ?assertEqual("127.0.0.1", config:get("httpd", "bind_address"))
Review Comment:
It was `undefined` before because the `config:delete("httpd", "bind_address")` on the previous line removed it from the `ets` table, but did not reload the default. With this change, after deletion the default gets reloaded from disk and inserted into the ets table. In short, It is the crux of this PR.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1372145507
##########
src/config/test/config_tests.erl:
##########
@@ -441,8 +437,8 @@ should_write_changes(_, _) ->
?assertEqual("5986", config:get("httpd", "port")),
?assertEqual(ok, config:set("httpd", "port", "8080")),
?assertEqual("8080", config:get("httpd", "port")),
- ?assertEqual(ok, config:delete("httpd", "bind_address", "8080")),
- ?assertEqual(undefined, config:get("httpd", "bind_address"))
+ ?assertEqual(ok, config:delete("httpd", "bind_address")),
+ ?assertEqual("127.0.0.1", config:get("httpd", "bind_address"))
Review Comment:
Why does not this remain `undefined`?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1406510649
##########
src/config/test/config_tests.erl:
##########
@@ -115,6 +111,41 @@ handle_config_terminate(Self, Reason, {Pid, State}) ->
Pid ! {config_msg, {Self, Reason, State}},
ok.
+setup_ini(IniFiles) ->
+ Chain = [write_ini(F) || F <- IniFiles],
+ {Chain, setup(Chain)}.
+
+teardown_ini(_, {Chain, Apps}) ->
+ lists:foreach(
+ fun(Path) -> ok = file:delete(Path) end,
+ Chain
+ ),
+ teardown(Apps).
+
+write_ini(FileName) ->
+ Path = filename:join([?TEMPDIR, FileName]),
+ Data = io_lib:format("[section]\nfile = ~s\n", [FileName]),
+ ok = file:write_file(Path, Data),
+ Path.
+
+config_delete_reload_restart_test_() ->
+ {
+ "Test consistency after set, delete, reload, and restart",
+ foreachx,
+ fun setup_ini/1,
+ fun teardown_ini/2,
+ [
+ {
+ ["default.ini", "local.ini"],
+ fun non_persistent_set_delete_reload_restart/2
Review Comment:
For one or two extra parameters, the complexity of a foreachx might not be worth it? For the two options just having two test setups: `setup_local_ini` and `setup_default_ini` and they can call the same setup `setup_ini/1` utility function could be simpler and could use the standard `?TDEF_FE` macro?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1408654225
##########
src/docs/src/config/intro.rst:
##########
@@ -126,9 +126,10 @@ by at least one space on each side. This might be useful in the ``[jwt_keys]``
section, where base64 encoded keys may contain some ``=`` characters.
.. note::
- In case when you'd like to remove some parameter from the `default.ini`
- without modifying that file, you may override in `local.ini`, but without
- any value::
+ In case you'd like to remove some parameter from an `.ini` file
+ (e.g. `default.ini`) without modifying that file, you may override
+ in a subsequently loaded `.ini` file (e.g. `local.ini`), but
Review Comment:
Wonder if `may override it in a...` would sound better?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1408705136
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
The goal of the `Persist = false` I think would be do everything the same as `Persist = true` except write to file. That seems like the simplest description of its behavior. The immediate effects on the "live" config value should be exactly the same, so if a user does:
`config:set("section", "key", "value", true)` or `config:set("section", "key", "value", false)`, the live value would be `"value"`. If they do `config:delete("section", "key", true)` or `config:delete("section", "key", false)` the live value would be the code default (config returns `undefined`).
After a restart or reload, the live value may change (it may be whatever was in the files) but that's something the user explicitly requested by using he `Persist = false` modifier.
Only in the `Persist = true` (the default) case, we'd want to have idempotence between any number of restarts or reloads after setting or deleting a value.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane merged PR #4813:
URL: https://github.com/apache/couchdb/pull/4813
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1364857681
##########
src/config/src/config.erl:
##########
@@ -326,6 +326,13 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
"~p: [~s] ~s deleted for reason ~p",
[?MODULE, Sec, Key, Reason]
),
+ DiskKVs = parse_ini_files(Config#config.ini_files),
+ case dict:find({Sec, Key}, DiskKVs) of
+ {ok, DiskVal} ->
+ true = ets:insert(?MODULE, {{Sec, Key}, DiskVal});
+ error ->
+ ok
+ end,
Review Comment:
To emphasize that we're aiming to get the value that we'd get effectively after a node restart or `reload()`, it might make sense to do the parsing and reading after the write. That's just so we go through the same sequence of operations: 1) parse from ini and 2) update ETS, that we'd perform when we re-start the node or when we call `reload()`.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#issuecomment-1839112672
> nice improvements here. I tried it out in remsh also and found everything working the way we discussed.
Thanks for the review, Bob.
Unfortunately, while this PR does indeed "fix" `reload` to honor delete markers, it turns out that at least one big downstream user actually _relies_ on this bug to operate their DBaaS. I've added some details in the Additional Context of this issue: https://github.com/apache/couchdb/issues/4892
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1409611579
##########
src/docs/src/config/intro.rst:
##########
@@ -126,9 +126,10 @@ by at least one space on each side. This might be useful in the ``[jwt_keys]``
section, where base64 encoded keys may contain some ``=`` characters.
.. note::
- In case when you'd like to remove some parameter from the `default.ini`
- without modifying that file, you may override in `local.ini`, but without
- any value::
+ In case you'd like to remove some parameter from an `.ini` file
+ (e.g. `default.ini`) without modifying that file, you may override
+ it in a subsequently loaded `.ini` file (e.g. `local.ini`), but
Review Comment:
Is the concept of being "subsequently loaded" clarified somewhere in the documentation? That is, is the order of processing for `.ini` files explicitly discussed somewhere above?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1421497935
##########
src/config/src/config.erl:
##########
@@ -340,54 +331,30 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
{reply, Else, Config}
end;
handle_call(reload, _From, Config) ->
- DiskKVs = lists:foldl(
- fun(IniFile, DiskKVs0) ->
- {ok, ParsedIniValues} = parse_ini_file(IniFile),
- lists:foldl(
- fun({K, V}, DiskKVs1) ->
- dict:store(K, V, DiskKVs1)
- end,
- DiskKVs0,
- ParsedIniValues
- )
- end,
- dict:new(),
- Config#config.ini_files
- ),
- % Update ets with anything we just read
- % from disk
- dict:fold(
- fun({Sec, Key} = K, V, _) ->
+ % Update ets with ini values.
+ IniMap = ini_map(Config#config.ini_files),
+ maps:foreach(
+ fun({Sec, Key} = K, V) ->
VExisting = get(Sec, Key, V),
- ets:insert(?MODULE, {K, V}),
+ true = ets:insert(?MODULE, {K, V}),
case V =:= VExisting of
true ->
ok;
false ->
- case is_sensitive(Sec, Key) of
- false ->
- couch_log:notice(
- "Reload detected config change ~s.~s = ~p",
- [Sec, Key, V]
- );
- true ->
- couch_log:notice(
- "Reload detected config change ~s.~s = '****'",
- [Sec, Key]
- )
- end,
+ couch_log:notice(
+ "Reload detected config change ~s.~s = ~p",
+ [Sec, Key, maybe_conceal(V, is_sensitive(Sec, Key))]
+ ),
Event = {config_change, Sec, Key, V, true},
gen_event:sync_notify(config_event, Event)
end
end,
- nil,
- DiskKVs
+ IniMap
),
- % And remove anything in ets that wasn't
- % on disk.
+ % And remove anything in ets that wasn't on disk.
ets:foldl(
fun({{Sec, Key} = K, _}, _) ->
- case dict:is_key(K, DiskKVs) of
+ case maps:is_key(K, IniMap) of
Review Comment:
excellent suggestion!
implemented in https://github.com/apache/couchdb/pull/4813/commits/0f727cd1ed2de5e22fbc13d5a120a2122da74d80
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1418353483
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
That makes sense. There is basically a single "load" operation now, for initial loading and reloading or after a delete call.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1418353483
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
That makes sense. There is basically a single "load" operation now.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1418434390
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
https://github.com/apache/couchdb/pull/4813/commits/d083263c17ee8002150ccfd1b0de923a0626d486 reloads for both kinds of `config:delete`, and IMO the tests seem more consistent.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1409618768
##########
src/docs/src/config/intro.rst:
##########
@@ -126,9 +126,10 @@ by at least one space on each side. This might be useful in the ``[jwt_keys]``
section, where base64 encoded keys may contain some ``=`` characters.
.. note::
- In case when you'd like to remove some parameter from the `default.ini`
- without modifying that file, you may override in `local.ini`, but without
- any value::
+ In case you'd like to remove some parameter from an `.ini` file
+ (e.g. `default.ini`) without modifying that file, you may override
+ it in a subsequently loaded `.ini` file (e.g. `local.ini`), but
Review Comment:
Yes, [here](https://docs.couchdb.org/en/stable/config/intro.html#configuration-files) is where the docs discuss loading configuration files in order. I'm very open to improved wording though...
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1408705136
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
The goal of the `Persist = false` I think would be to do everything the same as `Persist = true` except write to file. That seems like the simplest description of its behavior. The immediate effects on the "live" config value should be exactly the same, so if a user does:
`config:set("section", "key", "value", true)` or `config:set("section", "key", "value", false)`, the live value would be `"value"`. If they do `config:delete("section", "key", true)` or `config:delete("section", "key", false)` the live value would be the code default (config returns `undefined`).
After a restart or reload, the live value may change (it may be whatever was in the files) but that's something the user explicitly requested by using the `Persist = false` modifier.
Only in the `Persist = true` (the default) case, we'd want to have idempotence between any number of restarts or reloads after setting or deleting a value.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1406548807
##########
src/config/test/config_tests.erl:
##########
@@ -115,6 +111,41 @@ handle_config_terminate(Self, Reason, {Pid, State}) ->
Pid ! {config_msg, {Self, Reason, State}},
ok.
+setup_ini(IniFiles) ->
+ Chain = [write_ini(F) || F <- IniFiles],
+ {Chain, setup(Chain)}.
+
+teardown_ini(_, {Chain, Apps}) ->
+ lists:foreach(
+ fun(Path) -> ok = file:delete(Path) end,
+ Chain
+ ),
+ teardown(Apps).
+
+write_ini(FileName) ->
+ Path = filename:join([?TEMPDIR, FileName]),
+ Data = io_lib:format("[section]\nfile = ~s\n", [FileName]),
+ ok = file:write_file(Path, Data),
+ Path.
+
+config_delete_reload_restart_test_() ->
+ {
+ "Test consistency after set, delete, reload, and restart",
+ foreachx,
+ fun setup_ini/1,
+ fun teardown_ini/2,
+ [
+ {
+ ["default.ini", "local.ini"],
+ fun non_persistent_set_delete_reload_restart/2
Review Comment:
Yeah, I think the write to last is pretty obvious. The interesting corner-cases are around how the effective value changes after set/delete, with persist/non-persist and either restart or reload. So cases like "set value", then the same value should persist after restart and reload, or reload then restart.
My main beef with `foreachx` is that it becomes another eunit quirk one has to learn to be able add add or fix failing tests.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1408707799
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
That also makes sense, and has the added benefit of not requiring any behavior change to delete IIUC.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
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 could be updated to reflect the possibility of more `.ini` files. The change could be:
"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:
```
(node1@127.0.0.1)2> config:get("section", "file").
"local.ini"
(node1@127.0.0.1)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":
```
(node1@127.0.0.1)4> config:get("section", "file").
undefined
```
but for the wrong reason, whereas after a `reload`, it gives the wrong answer:
```
(node1@127.0.0.1)5> config:reload().
ok
(node1@127.0.0.1)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:
```
(node1@127.0.0.1)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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1418457489
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
> It's definitely cleaner code to do it in both cases, but I thought you had previously argued against reloading .ini files in the case of `Persist=false`?
I think it depends if we stay with delete markers. If we stay with delete markers, then on `Persist=false` it doesn't matter if we reload, the effective live value should still be `undefined` (reload then ets:delete is the same as skipping reloading and just doing ets:delete).
If we get rid of delete markers, then reload/load/delete/set on delete would all run "load" to determine the live value. Persist is just a modifier whether we write to file or not before running "load". There are additional bits in case of reload to make sure to delete the ets values which now many have changed. There is notification bit as well.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1373082694
##########
src/config/test/config_tests.erl:
##########
@@ -441,8 +437,8 @@ should_write_changes(_, _) ->
?assertEqual("5986", config:get("httpd", "port")),
?assertEqual(ok, config:set("httpd", "port", "8080")),
?assertEqual("8080", config:get("httpd", "port")),
- ?assertEqual(ok, config:delete("httpd", "bind_address", "8080")),
- ?assertEqual(undefined, config:get("httpd", "bind_address"))
+ ?assertEqual(ok, config:delete("httpd", "bind_address")),
+ ?assertEqual("127.0.0.1", config:get("httpd", "bind_address"))
Review Comment:
Okay, that is what I thought. Thanks for the confirmation.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
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:
```
(node1@127.0.0.1)2> config:get("section", "file").
"local.ini"
(node1@127.0.0.1)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":
```
(node1@127.0.0.1)4> config:get("section", "file").
undefined
```
but for the wrong reason, whereas after a `reload`, it gives the wrong answer:
```
(node1@127.0.0.1)5> config:reload().
ok
(node1@127.0.0.1)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:
```
(node1@127.0.0.1)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:
```
(node1@127.0.0.1)2> config:get("section", "file").
"local.ini"
(node1@127.0.0.1)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":
```
(node1@127.0.0.1)4> config:get("section", "file").
undefined
```
but for the wrong reason, whereas after a `reload`, it gives the wrong answer:
```
(node1@127.0.0.1)5> config:reload().
ok
(node1@127.0.0.1)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:
```
(node1@127.0.0.1)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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1408678794
##########
src/docs/src/config/intro.rst:
##########
@@ -126,9 +126,10 @@ by at least one space on each side. This might be useful in the ``[jwt_keys]``
section, where base64 encoded keys may contain some ``=`` characters.
.. note::
- In case when you'd like to remove some parameter from the `default.ini`
- without modifying that file, you may override in `local.ini`, but without
- any value::
+ In case you'd like to remove some parameter from an `.ini` file
+ (e.g. `default.ini`) without modifying that file, you may override
+ in a subsequently loaded `.ini` file (e.g. `local.ini`), but
Review Comment:
Oops, good catch. Will push a fix.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1408707799
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
That also makes sense, and has the added benefit of not requiring any behavior change to delete IIUC.
Updated in https://github.com/apache/couchdb/pull/4813/commits/9306b8a505be9dc366964f3e5a07bb5c6af3285f
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1407930221
##########
src/config/src/config.erl:
##########
@@ -324,26 +337,15 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
{reply, Else, Config}
end;
handle_call(reload, _From, Config) ->
- DiskKVs = lists:foldl(
- fun(IniFile, DiskKVs0) ->
- {ok, ParsedIniValues} = parse_ini_file(IniFile),
- lists:foldl(
- fun({K, V}, DiskKVs1) ->
- dict:store(K, V, DiskKVs1)
- end,
- DiskKVs0,
- ParsedIniValues
- )
- end,
- dict:new(),
- Config#config.ini_files
- ),
- % Update ets with anything we just read
- % from disk
- dict:fold(
- fun({Sec, Key} = K, V, _) ->
+ IniMap = ini_map(Config#config.ini_files),
+ % Update ets with list values, and delete those with ?DELETE markers.
+ maps:foreach(
+ fun({Sec, Key} = K, V) ->
VExisting = get(Sec, Key, V),
- ets:insert(?MODULE, {K, V}),
+ case V =:= ?DELETE of
Review Comment:
`=:=` looks odd here as the only difference between it and the more obvious `==` applies to comparing floats and integers. `?DELETE` is an atom, so there is no chance of that.
##########
src/config/src/config.erl:
##########
@@ -400,10 +400,21 @@ is_sensitive(Section, Key) ->
_ -> false
end.
+ini_map(IniFiles) ->
Review Comment:
nice.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve consistency of config after delete and reload, and tidy up [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1409620871
##########
src/docs/src/config/intro.rst:
##########
@@ -126,9 +126,10 @@ by at least one space on each side. This might be useful in the ``[jwt_keys]``
section, where base64 encoded keys may contain some ``=`` characters.
.. note::
- In case when you'd like to remove some parameter from the `default.ini`
- without modifying that file, you may override in `local.ini`, but without
- any value::
+ In case you'd like to remove some parameter from an `.ini` file
+ (e.g. `default.ini`) without modifying that file, you may override
+ it in a subsequently loaded `.ini` file (e.g. `local.ini`), but
Review Comment:
Ah, that is perfect, thank you. I do not think I have problems with the wording. I just wondered if this has been made explicit up to this point.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1418330777
##########
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'm resolving this conversation since we decided to take the approach of removing support for delete markers altogether.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove support for delete markers and improve config consistency [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #4813:
URL: https://github.com/apache/couchdb/pull/4813#discussion_r1418429479
##########
src/config/src/config.erl:
##########
@@ -315,6 +315,19 @@ handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
_ ->
ok
end,
+ % If persistent, there will always be a delete marker just written
+ % in the last .ini file, so only reload in the non-persistent case.
+ Persist orelse
+ begin
+ IniMap = ini_map(Config#config.ini_files),
+ case maps:find({Sec, Key}, IniMap) of
+ % ?DELETE markers not inserted
+ {ok, Val} when is_list(Val) ->
+ true = ets:insert(?MODULE, {{Sec, Key}, Val});
+ _ ->
+ ok
+ end
Review Comment:
It's definitely cleaner code to do it in both cases, but I thought you had previously argued against reloading .ini files in the case of `Persist=false`?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Config delete reload [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
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. That discrepancy seems like a bug. I think it would be nice to at least get it to state 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.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org