This is an automated email from the ASF dual-hosted git repository. vatamane pushed a commit to branch w-3-for-purge-plugin in repository https://gitbox.apache.org/repos/asf/couchdb.git
commit 22fa48bb633047a9b357c01cc5ddab994df42f04 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.
