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/18 21:18:12 UTC
[GitHub] [couchdb] iilyak commented on a change in pull request #3609: Normalize some config options
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