This is an automated email from the ASF dual-hosted git repository.
vatamane pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/couchdb.git
The following commit(s) were added to refs/heads/main by this push:
new 61c4c4d16 Cleanup fabric r/w parameter handling
61c4c4d16 is described below
commit 61c4c4d162366ffa979cdf39baf67487fdfd9643
Author: Nick Vatamaniuc <[email protected]>
AuthorDate: Mon Oct 27 12:14:36 2025 -0400
Cleanup fabric r/w parameter handling
For some odd reasons we parse the request `r` and `w` parameters as lists in
chttpd. Then, in fabric, we jump through hoops switching them to integers
(we
switch get_value defaults to lists, then parse the result back to an
integer).
Add a helper function in fabric_util go DRY-up the quorum option fetching
and
make it accept both list and integers. For now chttpd still sends the
values as
lists but in the future we can gradually switch to using integers.
---
src/fabric/src/fabric_doc_open.erl | 4 +-
src/fabric/src/fabric_doc_open_revs.erl | 3 +-
src/fabric/src/fabric_doc_purge.erl | 10 +--
src/fabric/src/fabric_doc_update.erl | 3 +-
src/fabric/src/fabric_open_revs.erl | 3 +-
src/fabric/src/fabric_util.erl | 122 +++++++++++++++++++++++---------
6 files changed, 95 insertions(+), 50 deletions(-)
diff --git a/src/fabric/src/fabric_doc_open.erl
b/src/fabric/src/fabric_doc_open.erl
index 37cb699d6..138a3f2bb 100644
--- a/src/fabric/src/fabric_doc_open.erl
+++ b/src/fabric/src/fabric_doc_open.erl
@@ -41,11 +41,11 @@ go(DbName, Id, Options) ->
),
SuppressDeletedDoc = not lists:member(deleted, Options),
N = mem3:n(DbName),
- R = couch_util:get_value(r, Options, integer_to_list(mem3:quorum(DbName))),
+ R = fabric_util:r_from_opts(DbName, Options),
Acc0 = #acc{
dbname = DbName,
workers = Workers,
- r = min(N, list_to_integer(R)),
+ r = min(N, R),
state = r_not_met,
replies = []
},
diff --git a/src/fabric/src/fabric_doc_open_revs.erl
b/src/fabric/src/fabric_doc_open_revs.erl
index ae3f72420..93ec7e71e 100644
--- a/src/fabric/src/fabric_doc_open_revs.erl
+++ b/src/fabric/src/fabric_doc_open_revs.erl
@@ -37,12 +37,11 @@ go(DbName, Id, Revs, Options) ->
open_revs,
[Id, Revs, Options]
),
- R = couch_util:get_value(r, Options, integer_to_list(mem3:quorum(DbName))),
State = #state{
dbname = DbName,
worker_count = length(Workers),
workers = Workers,
- r = list_to_integer(R),
+ r = fabric_util:r_from_opts(DbName, Options),
revs = Revs,
latest = lists:member(latest, Options),
replies = []
diff --git a/src/fabric/src/fabric_doc_purge.erl
b/src/fabric/src/fabric_doc_purge.erl
index bc96430eb..a4fc9e76f 100644
--- a/src/fabric/src/fabric_doc_purge.erl
+++ b/src/fabric/src/fabric_doc_purge.erl
@@ -62,7 +62,7 @@ go(DbName, IdsRevs, Options) ->
worker_uuids = WorkerUUIDs,
resps = Responses,
uuid_counts = UUIDCounts,
- w = w(DbName, Options)
+ w = fabric_util:w_from_opts(DbName, Options)
},
Callback = fun handle_message/3,
Acc2 =
@@ -128,14 +128,6 @@ group_reqs_by_shard(DbName, Reqs) ->
end,
lists:foldl(ReqFoldFun, #{}, Reqs).
-w(DbName, Options) ->
- try
- list_to_integer(couch_util:get_value(w, Options))
- catch
- _:_ ->
- mem3:quorum(DbName)
- end.
-
% Failed WorkerUUIDs = #{#shard{} => [UUIDs, ...]}
% Resps = #{UUID => [{ok, ...} | {error, ...}]
append_errors(Type, #{} = WorkerUUIDs, #{} = Resps) ->
diff --git a/src/fabric/src/fabric_doc_update.erl
b/src/fabric/src/fabric_doc_update.erl
index 1f5755de0..a977180bc 100644
--- a/src/fabric/src/fabric_doc_update.erl
+++ b/src/fabric/src/fabric_doc_update.erl
@@ -42,11 +42,10 @@ go(DbName, AllDocs0, Opts) ->
),
{Workers, _} = lists:unzip(GroupedDocs),
RexiMon = fabric_util:create_monitors(Workers),
- W = couch_util:get_value(w, Options, integer_to_list(mem3:quorum(DbName))),
Acc0 = #acc{
waiting_count = length(Workers),
doc_count = length(AllDocs),
- w = list_to_integer(W),
+ w = fabric_util:w_from_opts(DbName, Options),
grouped_docs = GroupedDocs,
reply = dict:new()
},
diff --git a/src/fabric/src/fabric_open_revs.erl
b/src/fabric/src/fabric_open_revs.erl
index b0a54645a..b4f95df8a 100644
--- a/src/fabric/src/fabric_open_revs.erl
+++ b/src/fabric/src/fabric_open_revs.erl
@@ -83,8 +83,7 @@ handle_message(Reason, Worker, #st{} = St) ->
handle_error(Reason, St#st{workers = Workers1, reqs = Reqs1}).
init_state(DbName, IdsRevsOpts, Options) ->
- DefaultR = integer_to_list(mem3:quorum(DbName)),
- R = list_to_integer(couch_util:get_value(r, Options, DefaultR)),
+ R = fabric_util:r_from_opts(DbName, Options),
{ArgRefs, Reqs0} = build_req_map(IdsRevsOpts),
ShardMap = build_worker_map(DbName, Reqs0),
{Workers, Reqs} = spawn_workers(Reqs0, ShardMap, Options),
diff --git a/src/fabric/src/fabric_util.erl b/src/fabric/src/fabric_util.erl
index b10f20a63..1da214f7f 100644
--- a/src/fabric/src/fabric_util.erl
+++ b/src/fabric/src/fabric_util.erl
@@ -37,13 +37,13 @@
-export([get_uuid_prefix_len/0]).
-export([isolate/1, isolate/2]).
-export([get_design_doc_records/1]).
+-export([w_from_opts/2, r_from_opts/2]).
-compile({inline, [{doc_id_and_rev, 1}]}).
-include_lib("mem3/include/mem3.hrl").
-include_lib("couch/include/couch_db.hrl").
-include_lib("couch_mrview/include/couch_mrview.hrl").
--include_lib("eunit/include/eunit.hrl").
remove_down_workers(Workers, BadNode) ->
remove_down_workers(Workers, BadNode, []).
@@ -261,38 +261,6 @@ create_monitors(Shards) ->
MonRefs = lists:usort([rexi_utils:server_pid(N) || #shard{node = N} <-
Shards]),
rexi_monitor:start(MonRefs).
-%% verify only id and rev are used in key.
-update_counter_test() ->
- Reply =
- {ok, #doc{
- id = <<"id">>,
- revs = <<"rev">>,
- body = <<"body">>,
- atts = <<"atts">>
- }},
- ?assertEqual(
- [{{<<"id">>, <<"rev">>}, {Reply, 1}}],
- update_counter(Reply, 1, [])
- ).
-
-remove_ancestors_test() ->
- Foo1 = {ok, #doc{revs = {1, [<<"foo">>]}}},
- Foo2 = {ok, #doc{revs = {2, [<<"foo2">>, <<"foo">>]}}},
- Bar1 = {ok, #doc{revs = {1, [<<"bar">>]}}},
- Bar2 = {not_found, {1, <<"bar">>}},
- ?assertEqual(
- [kv(Bar1, 1), kv(Foo1, 1)],
- remove_ancestors([kv(Bar1, 1), kv(Foo1, 1)], [])
- ),
- ?assertEqual(
- [kv(Bar1, 1), kv(Foo2, 2)],
- remove_ancestors([kv(Bar1, 1), kv(Foo1, 1), kv(Foo2, 1)], [])
- ),
- ?assertEqual(
- [kv(Bar1, 2)],
- remove_ancestors([kv(Bar2, 1), kv(Bar1, 1)], [])
- ).
-
is_replicator_db(DbName) ->
path_ends_with(DbName, <<"_replicator">>).
@@ -415,6 +383,30 @@ get_design_doc_records(DbName) ->
Else
end
end).
+
+w_from_opts(Db, Options) ->
+ quorum_from_opts(Db, couch_util:get_value(w, Options)).
+
+r_from_opts(Db, Options) ->
+ quorum_from_opts(Db, couch_util:get_value(r, Options)).
+
+quorum_from_opts(Db, Val) ->
+ try
+ if
+ is_integer(Val) ->
+ Val;
+ is_list(Val) ->
+ % Compatibility clause. Keep as long as chttpd parses r and w
+ % request parameters as lists (strings).
+ list_to_integer(Val);
+ true ->
+ mem3:quorum(Db)
+ end
+ catch
+ _:_ ->
+ mem3:quorum(Db)
+ end.
+
% If we issue multiple fabric calls from the same process we have to isolate
% them so in case of error they don't pollute the processes dictionary or the
% mailbox
@@ -443,6 +435,41 @@ do_isolate(Fun) ->
{'$isolerr', Tag, Reason, Stack}
end.
+-ifdef(TEST).
+-include_lib("couch/include/couch_eunit.hrl").
+
+%% verify only id and rev are used in key.
+update_counter_test() ->
+ Reply =
+ {ok, #doc{
+ id = <<"id">>,
+ revs = <<"rev">>,
+ body = <<"body">>,
+ atts = <<"atts">>
+ }},
+ ?assertEqual(
+ [{{<<"id">>, <<"rev">>}, {Reply, 1}}],
+ update_counter(Reply, 1, [])
+ ).
+
+remove_ancestors_test() ->
+ Foo1 = {ok, #doc{revs = {1, [<<"foo">>]}}},
+ Foo2 = {ok, #doc{revs = {2, [<<"foo2">>, <<"foo">>]}}},
+ Bar1 = {ok, #doc{revs = {1, [<<"bar">>]}}},
+ Bar2 = {not_found, {1, <<"bar">>}},
+ ?assertEqual(
+ [kv(Bar1, 1), kv(Foo1, 1)],
+ remove_ancestors([kv(Bar1, 1), kv(Foo1, 1)], [])
+ ),
+ ?assertEqual(
+ [kv(Bar1, 1), kv(Foo2, 2)],
+ remove_ancestors([kv(Bar1, 1), kv(Foo1, 1), kv(Foo2, 1)], [])
+ ),
+ ?assertEqual(
+ [kv(Bar1, 2)],
+ remove_ancestors([kv(Bar2, 1), kv(Bar1, 1)], [])
+ ).
+
get_db_timeout_test() ->
% Q=1, N=1
?assertEqual(20000, get_db_timeout(1, 2, 100, 60000)),
@@ -486,3 +513,32 @@ get_db_timeout_test() ->
% request_timeout was set to infinity, with enough shards it still gets to
% 100 min timeout at the start from the exponential logic
?assertEqual(100, get_db_timeout(64, 2, 100, infinity)).
+
+rw_opts_test_() ->
+ {
+ foreach,
+ fun() -> meck:new(mem3, [passthrough]) end,
+ fun(_) -> meck:unload() end,
+ [
+ ?TDEF_FE(t_w_opts_get),
+ ?TDEF_FE(t_r_opts_get)
+ ]
+ }.
+
+t_w_opts_get(_) ->
+ meck:expect(mem3, quorum, 1, 3),
+ ?assertEqual(5, w_from_opts(any_db, [{w, 5}])),
+ ?assertEqual(5, w_from_opts(any_db, [{w, "5"}])),
+ ?assertEqual(3, w_from_opts(any_db, [{w, some_other_type}])),
+ ?assertEqual(3, w_from_opts(any_db, [{w, "five"}])),
+ ?assertEqual(3, w_from_opts(any_db, [])).
+
+t_r_opts_get(_) ->
+ meck:expect(mem3, quorum, 1, 3),
+ ?assertEqual(5, r_from_opts(any_db, [{other_opt, 42}, {r, 5}])),
+ ?assertEqual(5, r_from_opts(any_db, [{r, "5"}, {something_else, "xyz"}])),
+ ?assertEqual(3, r_from_opts(any_db, [{r, some_other_type}])),
+ ?assertEqual(3, r_from_opts(any_db, [{r, "five"}])),
+ ?assertEqual(3, r_from_opts(any_db, [])).
+
+-endif.
