You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2021/06/07 14:31:46 UTC
[GitHub] [couchdb] jiahuili430 opened a new pull request #3609: Normalise all config options
jiahuili430 opened a new pull request #3609:
URL: https://github.com/apache/couchdb/pull/3609
## Overview
Normalise all config options so default.ini is 100% commented out.
## Testing recommendations
<!-- Describe how we can test your changes.
Does it provides any behaviour that the end users
could notice? -->
## Related Issues or Pull Requests
This fixes #3473
## Checklist
- [x] Code is written and works correctly
- [ ] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3609: Normalize some config options
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3609:
URL: https://github.com/apache/couchdb/pull/3609#discussion_r652112636
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -141,7 +141,12 @@ start_link(Name, Options) ->
end.
get_server_options(Module) ->
- ServerOptsCfg = config:get(Module, "server_options", "[]"),
+ ServerOptsCfg =
+ case Module of
+ "chttpd" -> config:get(Module,
+ "server_options", "[{recbuf, undefined}]");
Review comment:
Maybe we could `-define(...)` a macro at the top to make it more readable. `DEFAULT_SERVER_OPTIONS` ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3609: Normalize some config options
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3609:
URL: https://github.com/apache/couchdb/pull/3609#discussion_r652113397
##########
File path: src/chttpd/src/chttpd_prefer_header.erl
##########
@@ -47,7 +47,9 @@ filter_headers(Headers, IncludeList) ->
get_header_list() ->
- SectionStr = config:get("chttpd", "prefer_minimal", ""),
+ SectionStr = config:get("chttpd", "prefer_minimal",
+ "Cache-Control, Content-Length, Content-Range,
Review comment:
Let's define a macro here too `?DEFAULT_PREFERRED_HEADERS`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak merged pull request #3609: Normalize some config options
Posted by GitBox <gi...@apache.org>.
iilyak merged pull request #3609:
URL: https://github.com/apache/couchdb/pull/3609
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3609: Normalize some config options
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3609:
URL: https://github.com/apache/couchdb/pull/3609#discussion_r654410441
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -141,7 +144,12 @@ start_link(Name, Options) ->
end.
get_server_options(Module) ->
- ServerOptsCfg = config:get(Module, "server_options", "[]"),
+ ServerOptsCfg =
+ case Module of
+ "chttpd" -> config:get(Module,
+ "server_options", ?DEFAULT_SERVER_OPTIONS);
+ _ -> config:get(Module, "server_options", "[]")
Review comment:
You've commented out [`httpd->server_options = [{sndbuf, 262144}]`](https://github.com/apache/couchdb/pull/3609/files#diff-38acf80292ea264190d7a08646c46d571d7ddcb37a749a58fb79c009030dd115L218).
Should it be `config:get(Module, "server_options", "[{sndbuf, 262144}]")`. If that's the case we should define one more macro constant.
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -141,7 +144,12 @@ start_link(Name, Options) ->
end.
get_server_options(Module) ->
- ServerOptsCfg = config:get(Module, "server_options", "[]"),
+ ServerOptsCfg =
+ case Module of
+ "chttpd" -> config:get(Module,
+ "server_options", ?DEFAULT_SERVER_OPTIONS);
+ _ -> config:get(Module, "server_options", "[]")
Review comment:
Never mind the other type of Module is [`httpsd`](https://github.com/apache/couchdb/blob/3.x/src/chttpd/src/chttpd.erl#L115)
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -159,7 +167,7 @@ handle_request(MochiReq0) ->
handle_request_int(MochiReq) ->
Begin = os:timestamp(),
- case config:get("chttpd", "socket_options") of
+ case config:get("chttpd", "socket_options", ?DEFAULT_SOCKET_OPTIONS) of
Review comment:
When you use `config:get/3` you cannot get `undefined` atom. I believe it should be written as:
```
SocketOptsCfg = config:get("chttpd", "socket_options", ?DEFAULT_SOCKET_OPTIONS),
{ok, SocketOpts} = couch_util:parse_term(SocketOptsCfg),
ok = mochiweb_socket:setopts(MochiReq:get(socket), SocketOpts)
```
##########
File path: src/chttpd/src/chttpd_prefer_header.erl
##########
@@ -22,6 +22,11 @@
-include_lib("couch/include/couch_db.hrl").
+-define(DEFAULT_PREFER_MINIMAL,
Review comment:
This works in this particular case.
```
1> S = "Cache-Control, Content-Length, Content-Range,
1> Content-Type, ETag, Server, Transfer-Encoding, Vary".
"Cache-Control, Content-Length, Content-Range, \n Content-Type, ETag, Server, Transfer-Encoding, Vary"
2> re:split(S, "\\s*,\\s*", [trim, {return, list}]).
["Cache-Control","Content-Length","Content-Range",
"Content-Type","ETag","Server","Transfer-Encoding","Vary"]
```
However usually splitting long strings done differently:
```
6> S2 = "Cache-Control, Content-Length, Content-Range, "
6> "Content-Type, ETag, Server, Transfer-Encoding, Vary".
"Cache-Control, Content-Length, Content-Range, Content-Type, ETag, Server, Transfer-Encoding, Vary"
```
##########
File path: src/chttpd/src/chttpd_prefer_header.erl
##########
@@ -22,6 +22,11 @@
-include_lib("couch/include/couch_db.hrl").
+-define(DEFAULT_PREFER_MINIMAL,
Review comment:
Just to clarify the difference is "\n" embedded into the string.
##########
File path: src/chttpd/src/chttpd_sup.erl
##########
@@ -75,8 +77,9 @@ settings() ->
[
{bind_address, config:get("chttpd", "bind_address")},
{port, config:get("chttpd", "port")},
- {backlog, config:get("chttpd", "backlog")},
- {server_options, config:get("chttpd", "server_options")}
+ {backlog, config:get("chttpd", "backlog", ?DEFAULT_BACKLOG)},
Review comment:
Why not to convert it into `config:get_integer/3`? Is it because we use `undefined` somewhere?
##########
File path: src/couch/src/couch_httpd.erl
##########
@@ -44,6 +44,10 @@
-define(HANDLER_NAME_IN_MODULE_POS, 6).
-define(MAX_DRAIN_BYTES, 1048576).
-define(MAX_DRAIN_TIME_MSEC, 1000).
+-define(DEFAULT_SOCKET_OPTIONS, "[{sndbuf, 262144}]").
+-define(DEFAULT_AUTHENTICATION_HANDLERS,
Review comment:
See another comment about splitting strings into multiple lines.
##########
File path: src/couch/src/couch_httpd.erl
##########
@@ -44,6 +44,10 @@
-define(HANDLER_NAME_IN_MODULE_POS, 6).
-define(MAX_DRAIN_BYTES, 1048576).
-define(MAX_DRAIN_TIME_MSEC, 1000).
+-define(DEFAULT_SOCKET_OPTIONS, "[{sndbuf, 262144}]").
+-define(DEFAULT_AUTHENTICATION_HANDLERS,
Review comment:
However due to the way `couch_util:parse_term/1` your way also works:
```
Eshell V9.3.3.15 (abort with ^G)
1> couch_util:parse_term("{couch_httpd_auth, cookie_authentication_handler},
1> {couch_httpd_auth, default_authentication_handler}").
{error,{2,erl_parse,"bad term"}}
```
##########
File path: src/couch/src/couch_server.erl
##########
@@ -823,7 +823,7 @@ get_default_engine(Server, DbName) ->
engines = Engines
} = Server,
Default = {couch_bt_engine, make_filepath(RootDir, DbName, "couch")},
- case config:get("couchdb", "default_engine") of
+ case config:get("couchdb", "default_engine", ?DEFAULT_ENGINE) of
Review comment:
You don't need case when you use `config:get/3` you would always receive a string (assuming default value is string, which is true in your case).
##########
File path: src/couch_replicator/src/couch_replicator_docs.erl
##########
@@ -456,26 +456,26 @@ maybe_add_trailing_slash(Url) ->
make_options(Props) ->
Options0 = lists:ukeysort(1, convert_options(Props)),
Options = check_options(Options0),
- DefWorkers = config:get("replicator", "worker_processes", "4"),
- DefBatchSize = config:get("replicator", "worker_batch_size", "500"),
- DefConns = config:get("replicator", "http_connections", "20"),
- DefTimeout = config:get("replicator", "connection_timeout", "30000"),
- DefRetries = config:get("replicator", "retries_per_request", "5"),
+ DefWorkers = config:get_integer("replicator", "worker_processes", 4),
+ DefBatchSize = config:get_integer("replicator", "worker_batch_size", 500),
+ DefConns = config:get_integer("replicator", "http_connections", 20),
+ DefTimeout = config:get_integer("replicator", "connection_timeout", 30000),
+ DefRetries = config:get_integer("replicator", "retries_per_request", 5),
UseCheckpoints = config:get("replicator", "use_checkpoints", "true"),
Review comment:
Do you want to convert this into `config:get_boolean/3` and get rid of `list_to_existing_atom(UseCheckpoints)`?
##########
File path: src/ioq/src/ioq.erl
##########
@@ -55,7 +55,15 @@ get_queue_lengths() ->
gen_server:call(?MODULE, get_queue_lengths).
bypass(Priority) ->
- config:get("ioq.bypass", atom_to_list(Priority)) =:= "true".
+ case Priority of
Review comment:
Very good idea!
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -1714,7 +1714,7 @@ parse_doc_query(Req) ->
parse_shards_opt(Req) ->
[
{n, parse_shards_opt("n", Req, config:get("cluster", "n", "3"))},
- {q, parse_shards_opt("q", Req, config:get("cluster", "q", "8"))},
+ {q, parse_shards_opt("q", Req, config:get("cluster", "q", "2"))},
Review comment:
`config:get_integer/3`?
##########
File path: src/mem3/src/mem3.erl
##########
@@ -204,7 +204,7 @@ choose_shards(DbName, Nodes, Options) ->
true -> ok
end,
Q = mem3_util:q_val(couch_util:get_value(q, Options,
- config:get("cluster", "q", "8"))),
+ config:get("cluster", "q", "2"))),
Review comment:
`config:get_integer/3`?
##########
File path: src/setup/src/setup_httpd.erl
##########
@@ -36,7 +36,7 @@ handle_setup_req(#httpd{method='GET'}=Req) ->
true ->
chttpd:send_json(Req, 200, {[{state, single_node_enabled}]});
_ ->
- case config:get("cluster", "n", undefined) of
+ case config:get("cluster", "n", "3") of
Review comment:
`config:get_integer/3`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3609: Normalize some config options
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3609:
URL: https://github.com/apache/couchdb/pull/3609#discussion_r652112933
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -159,7 +164,8 @@ handle_request(MochiReq0) ->
handle_request_int(MochiReq) ->
Begin = os:timestamp(),
- case config:get("chttpd", "socket_options") of
+ case config:get("chttpd", "socket_options",
+ "[{sndbuf, 262144}, {nodelay, true}]") of
Review comment:
Let's use a define here as well, `?DEFAULT_SOCKET_OPTIONS` maybe?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jiahuili430 commented on a change in pull request #3609: Normalize some config options
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3609:
URL: https://github.com/apache/couchdb/pull/3609#discussion_r654436421
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -159,7 +167,7 @@ handle_request(MochiReq0) ->
handle_request_int(MochiReq) ->
Begin = os:timestamp(),
- case config:get("chttpd", "socket_options") of
+ case config:get("chttpd", "socket_options", ?DEFAULT_SOCKET_OPTIONS) of
Review comment:
Changed, Thanks for the review @iilyak
##########
File path: src/chttpd/src/chttpd_prefer_header.erl
##########
@@ -22,6 +22,11 @@
-include_lib("couch/include/couch_db.hrl").
+-define(DEFAULT_PREFER_MINIMAL,
Review comment:
Thanks for the clarification, changed.
##########
File path: src/couch/src/couch_httpd.erl
##########
@@ -44,6 +44,10 @@
-define(HANDLER_NAME_IN_MODULE_POS, 6).
-define(MAX_DRAIN_BYTES, 1048576).
-define(MAX_DRAIN_TIME_MSEC, 1000).
+-define(DEFAULT_SOCKET_OPTIONS, "[{sndbuf, 262144}]").
+-define(DEFAULT_AUTHENTICATION_HANDLERS,
Review comment:
Changed.
##########
File path: src/couch/src/couch_server.erl
##########
@@ -823,7 +823,7 @@ get_default_engine(Server, DbName) ->
engines = Engines
} = Server,
Default = {couch_bt_engine, make_filepath(RootDir, DbName, "couch")},
- case config:get("couchdb", "default_engine") of
+ case config:get("couchdb", "default_engine", ?DEFAULT_ENGINE) of
Review comment:
Changed.
##########
File path: src/chttpd/src/chttpd_sup.erl
##########
@@ -75,8 +77,9 @@ settings() ->
[
{bind_address, config:get("chttpd", "bind_address")},
{port, config:get("chttpd", "port")},
- {backlog, config:get("chttpd", "backlog")},
- {server_options, config:get("chttpd", "server_options")}
+ {backlog, config:get("chttpd", "backlog", ?DEFAULT_BACKLOG)},
Review comment:
Changed.
##########
File path: src/couch_replicator/src/couch_replicator_docs.erl
##########
@@ -456,26 +456,26 @@ maybe_add_trailing_slash(Url) ->
make_options(Props) ->
Options0 = lists:ukeysort(1, convert_options(Props)),
Options = check_options(Options0),
- DefWorkers = config:get("replicator", "worker_processes", "4"),
- DefBatchSize = config:get("replicator", "worker_batch_size", "500"),
- DefConns = config:get("replicator", "http_connections", "20"),
- DefTimeout = config:get("replicator", "connection_timeout", "30000"),
- DefRetries = config:get("replicator", "retries_per_request", "5"),
+ DefWorkers = config:get_integer("replicator", "worker_processes", 4),
+ DefBatchSize = config:get_integer("replicator", "worker_batch_size", 500),
+ DefConns = config:get_integer("replicator", "http_connections", 20),
+ DefTimeout = config:get_integer("replicator", "connection_timeout", 30000),
+ DefRetries = config:get_integer("replicator", "retries_per_request", 5),
UseCheckpoints = config:get("replicator", "use_checkpoints", "true"),
Review comment:
Changed.
##########
File path: src/ioq/src/ioq.erl
##########
@@ -55,7 +55,15 @@ get_queue_lengths() ->
gen_server:call(?MODULE, get_queue_lengths).
bypass(Priority) ->
- config:get("ioq.bypass", atom_to_list(Priority)) =:= "true".
+ case Priority of
Review comment:
Thanks
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -1714,7 +1714,7 @@ parse_doc_query(Req) ->
parse_shards_opt(Req) ->
[
{n, parse_shards_opt("n", Req, config:get("cluster", "n", "3"))},
- {q, parse_shards_opt("q", Req, config:get("cluster", "q", "8"))},
+ {q, parse_shards_opt("q", Req, config:get("cluster", "q", "2"))},
Review comment:
Changed.
##########
File path: src/mem3/src/mem3.erl
##########
@@ -204,7 +204,7 @@ choose_shards(DbName, Nodes, Options) ->
true -> ok
end,
Q = mem3_util:q_val(couch_util:get_value(q, Options,
- config:get("cluster", "q", "8"))),
+ config:get("cluster", "q", "2"))),
Review comment:
Changed.
##########
File path: src/setup/src/setup_httpd.erl
##########
@@ -36,7 +36,7 @@ handle_setup_req(#httpd{method='GET'}=Req) ->
true ->
chttpd:send_json(Req, 200, {[{state, single_node_enabled}]});
_ ->
- case config:get("cluster", "n", undefined) of
+ case config:get("cluster", "n", "3") of
Review comment:
Changed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jiahuili430 commented on a change in pull request #3609: Normalize some config options
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3609:
URL: https://github.com/apache/couchdb/pull/3609#discussion_r652130254
##########
File path: src/couch/src/couch_httpd.erl
##########
@@ -110,7 +110,7 @@ start_link(Name, Options) ->
{ok, ServerOptions} = couch_util:parse_term(
config:get("httpd", "server_options", "[]")),
{ok, SocketOptions} = couch_util:parse_term(
- config:get("httpd", "socket_options", "[]")),
+ config:get("httpd", "socket_options", "[{sndbuf, 262144}]")),
Review comment:
In default.ini, ServerOptions was commented out before, so it should get `[]`;
However, `socket_options = [{sndbuf, 262144}]`, so I changed it here.
Thanks for the review. @nickva
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak merged pull request #3609: Normalize some config options
Posted by GitBox <gi...@apache.org>.
iilyak merged pull request #3609:
URL: https://github.com/apache/couchdb/pull/3609
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3609: Normalize some config options
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3609:
URL: https://github.com/apache/couchdb/pull/3609#discussion_r652100445
##########
File path: src/chttpd/test/eunit/chttpd_util_test.erl
##########
@@ -86,40 +78,6 @@ test_with_undefined_option(_) ->
?assertNot(chttpd_util:get_chttpd_config("undefined_option", false)).
-test_with_httpd_option(_) ->
- ?assertEqual("{couch_httpd_auth, cookie_authentication_handler}, " ++
- "{couch_httpd_auth, default_authentication_handler}",
- chttpd_util:get_chttpd_config("authentication_handlers")).
-
-
-test_with_chttpd_option(_) ->
- ?assertEqual("512", chttpd_util:get_chttpd_config("backlog")),
- ?assertEqual("512", chttpd_util:get_chttpd_config("backlog", 123)),
- ?assertEqual(512, chttpd_util:get_chttpd_config_integer("backlog", 123)),
- ?assertEqual("false",
- chttpd_util:get_chttpd_config("require_valid_user")),
- ?assertEqual("false",
- chttpd_util:get_chttpd_config("require_valid_user", "true")),
- ?assertEqual(false,
- chttpd_util:get_chttpd_config_boolean("require_valid_user", true)).
-
-
-test_with_chttpd_option_which_moved_from_httpd(_) ->
- ?assertEqual(undefined, chttpd_util:get_chttpd_config("max_uri_length")),
- ?assertEqual(8000, chttpd_util:get_chttpd_config("max_uri_length", 8000)),
- ?assertEqual(undefined, chttpd_util:get_chttpd_config("WWW-Authenticate")),
- ?assert(chttpd_util:get_chttpd_config("enable_cors", true)).
-
-
-test_get_chttpd_config_integer(_) ->
- ?assertEqual(123,
- chttpd_util:get_chttpd_config_integer("max_http_request_size", 123)).
-
-
-test_get_chttpd_config_boolean(_) ->
- ?assert(chttpd_util:get_chttpd_config_boolean("allow_jsonp", true)).
-
-
Review comment:
I can see why these were removed however I think we dropped some coverage too. I noticed in setup we do set a bunch of test related value in chttpd / httpd, perhaps we'd want to have a test_chttpd_behavior(_) function which actually those and tests various overriding behavior just like we do for test_auth_behavior
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3609: Normalize some config options
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3609:
URL: https://github.com/apache/couchdb/pull/3609#discussion_r652067543
##########
File path: src/couch/src/couch_httpd.erl
##########
@@ -110,7 +110,7 @@ start_link(Name, Options) ->
{ok, ServerOptions} = couch_util:parse_term(
config:get("httpd", "server_options", "[]")),
{ok, SocketOptions} = couch_util:parse_term(
- config:get("httpd", "socket_options", "[]")),
+ config:get("httpd", "socket_options", "[{sndbuf, 262144}]")),
Review comment:
Hmm it seems this bit of code was duplicated with one right above it, we do `{ok, ServerOptions} =` twice in a row. Wonder what happened there. I think we'd want to do it once only and then apply the new defaults.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org