Couch server sharding #3366

rnewson · 2021-02-10T18:20:01Z

Overview

Break up the monolithic couch_server to avoid a bottleneck.

Testing recommendations

Heavy usage.

Related Issues or Pull Requests

N/A

Checklist

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

rnewson · 2021-02-10T18:22:47Z

src/couch/src/couch_server.erl

+    Fun = fun(N, {TimeAcc, OpenAcc}) ->
+        {ok, #server{start_time=Time,dbs_open=Open}} =
+            gen_server:call(couch_server(N), get_server),
+        {max(Time, TimeAcc), Open + OpenAcc} end,


"Time" here is a formatted datetime string, so 'max' is a bit arbitrary. all the couch_server_X's are started at the same time, not sure we care?

rnewson · 2021-02-10T18:23:09Z

src/couch/src/couch_server.erl

                true -> ok
            end;
        false ->
            ok
    end.

-close_lru() ->


No calls to this from outside the module

wohali · 2021-02-10T18:32:11Z

Huh. Can you explain the failure mode in more detail, @rnewson ?

rnewson · 2021-02-10T18:33:30Z

couch_server as a single gen_server has a finite throughput that, in busy clusters, is easily breached, causing a sizeable backlog in the message queue, ultimately leading to failure and errors.

rnewson · 2021-02-10T18:34:29Z

timeouts, memory blow ups, cats and dogs living together.

rnewson · 2021-02-10T18:37:46Z

src/couch/src/couch_server.erl

@@ -343,7 +348,7 @@ all_databases() ->
    {ok, lists:usort(DbList)}.

 all_databases(Fun, Acc0) ->
-    {ok, #server{root_dir=Root}} = gen_server:call(couch_server, get_server),
+    {ok, #server{root_dir=Root}} = gen_server:call(couch_server_1, get_server),


(because root_dir is the same in all of them)

chewbranca

Overall looks good to me. I left a few minor comments, nothing major. We'll want to make sure this is well tested before bringing it in, but overall the structure looks solid. Nice one!

chewbranca · 2021-02-10T20:06:14Z

src/couch/src/couch_server.erl

+handle_config_change("couchdb", "max_dbs_open", _, _, N) ->
+    {ok, gen_server:call(couch_server(N),{set_max_dbs_open,?MAX_DBS_OPEN})};
+handle_config_change("couchdb_engines", _, _, _, N) ->
+    {ok, gen_server:call(couch_server(N), reload_engines)};


Hmmm... do we want to trigger the config change callbacks N times now? The current approach keeps the code simpler, and I'm being a bit pedantic here, but it might be worth making a dedicated config listener pid that loops over the couch_server pids to trigger updates.

Each of the couch_servers get the callback and do this once each.

Sure, I just mean it's N numbers of config listeners for the same config changes, as each couch_server subscribes to the changes. Like I said, being pedantic here, but we could make a couch_server_config_listener that subscribes once and then updates all the couch_servers in a loop.

chewbranca · 2021-02-10T20:11:12Z

src/couch/src/couch_server.erl

+    name(BaseName, N);
+
+name(BaseName, #server{} = Srv) ->
+    name(BaseName, Srv#server.n);


If we're caching N, should we also just cache the relevant atoms so we're not doing list_to_atom on concatenated strings every time? Probably won't have a major impact given this approach will have parallelize couch_server pids, but it's certainly more work to do for them to do.

I thought about it but there are a fair few paths where we're not inside the couch_server gen process, but I'll look to see if we can do it tidily.

nickva · 2021-02-10T20:15:41Z

src/couch/src/couch_server.erl

+
+
+name(BaseName, DbName) when is_binary(DbName) ->
+    N = 1 + (erlang:crc32(DbName) rem num_servers()),


1 + erlang:phash2(DbName, num_servers()) might be cleaner here since we are not really computing a checksum but want to do hashing

May even be a tiny bit faster...

> DbName = crypto:strong_rand_bytes(100). <<23,241,236,53,92,151,12,217,246,88,65,153,224,214,236, 240,126,243,188,248,53,187,206,165,235,96,24,5,38,...>> > f(USec), {USec, ok} = timer:tc(fun() -> [erlang:crc32(DbName) rem 10 || I <- lists:seq(1, 1000000)], ok end), USec/1.0e6. 1.499477 > f(USec), {USec, ok} = timer:tc(fun() -> [erlang:phash2(DbName, 10) || I <- lists:seq(1, 1000000)], ok end), USec/1.0e6. 1.331179

hm, that looks nice, sure. I don't think checksum vs hash is a strong argument though, they're both means to an end.

@nickva I'm having trouble finding a reference, but I've been under the impression that timer:tc is not to be trusted on anonymous functions in the shell and should be tested on a compiled module.

@chewbranca I think they'd both be hit by the same slowness...

I tried a module mostly out of curiosity:

-module(test_hash). -export([test_crc32/2, test_phash/2]). test_crc32(N, Val) -> T0 = erlang:system_time(), do_crc32(N, Val), usec_per_cycle(erlang:system_time() - T0, N). test_phash(N, Val) -> T0 = erlang:system_time(), do_phash2(N, Val), usec_per_cycle(erlang:system_time() - T0, N). do_crc32(0, _) -> ok; do_crc32(N, Val) -> _ = erlang:crc32(Val) rem 10, do_crc32(N - 1, Val). do_phash2(0, _) -> ok; do_phash2(N, Val) -> _ = erlang:phash2(Val, 10), do_phash2(N - 1, Val). usec_per_cycle(Native, N) -> erlang:convert_time_unit(Native, native, microsecond) / N.

18> test_hash:test_crc32(10000000, DbName). 0.0722587 19> test_hash:test_phash(10000000, DbName). 0.070593

wohali · 2021-02-10T22:34:19Z

How will this affect the output of the message_queues key in _system stats?

This line calls registered() and I don't know how that works for multiple gen_servers that all appear to have the same name.

I suspect it'd be useful to be able to generate a (new?) stat that showed message queues by couch_server shard, identifying by db/shard serviced by that gen_server. But I might be assuming something here that isn't real... 🙃

rnewson · 2021-02-10T23:25:29Z

tests pass locally. getting random 1 or 2 failures each time on our CI chain. I would like to disable them if they aren't going to run reliably.

wohali · 2021-02-11T00:46:44Z

tests pass locally. getting random 1 or 2 failures each time on our CI chain. I would like to disable them if they aren't going to run reliably.

If the development team is unable to put in the time to maintain test suites properly, then you have my vote.

I'd bring this up on dev@, but I have, many times over many years, and nothing ever changes.

nickva · 2021-02-11T19:33:16Z

I noticed one of the failed tests in continuous-integration/jenkins/pr-merge a couch_server failure in it. I ran the test suite locally and it passed so could be just a flaky test and the new code just highlights the flakiness, but since couch_server showed up it was a bit suspicious so thought I'd mention it here:

[2021-02-11T18:42:04.494Z]     Multiple database create/delete tests

[2021-02-11T18:42:04.853Z]       couch_db_tests:111: should_create_multiple_dbs...[0.090 s] ok

[2021-02-11T18:42:04.853Z]       couch_db_tests:122: should_delete_multiple_dbs...*failed*

[2021-02-11T18:42:04.853Z] in function gen_server:call/3 (gen_server.erl, line 223)

[2021-02-11T18:42:04.853Z] in call from couch_server:create_int/2 (src/couch_server.erl, line 143)

[2021-02-11T18:42:04.853Z] in call from couch_server:create/2 (src/couch_server.erl, line 135)

[2021-02-11T18:42:04.853Z] in call from couch_db_tests:create_db/2 (test/eunit/couch_db_tests.erl, line 193)

[2021-02-11T18:42:04.853Z] in call from couch_db_tests:'-should_delete_multiple_dbs/1-fun-1-'/1 (test/eunit/couch_db_tests.erl, line 123)

[2021-02-11T18:42:04.853Z] in call from couch_db_tests:'-should_delete_multiple_dbs/1-lc$^0/1-0-'/1 (test/eunit/couch_db_tests.erl, line 123)

[2021-02-11T18:42:04.853Z] in call from couch_db_tests:'-should_delete_multiple_dbs/1-fun-8-'/1 (test/eunit/couch_db_tests.erl, line 123)

[2021-02-11T18:42:04.853Z] in call from eunit_test:run_testfun/1 (eunit_test.erl, line 71)

[2021-02-11T18:42:04.853Z] **exit:{{badarg,[{ets,update_element,

[2021-02-11T18:42:04.853Z]                [couch_server_4,

[2021-02-11T18:42:04.853Z]                 <<"eunit-test-db-cd448fbae5748e8885319d3a0d"...>>,

[2021-02-11T18:42:04.853Z]                 {5,locked}],

[2021-02-11T18:42:04.853Z]                []},

[2021-02-11T18:42:04.853Z]           {couch_lru,close_int,2,[{file,"src/couch_lru.erl"},{line,49}]},

[2021-02-11T18:42:04.853Z]           {couch_server,maybe_close_lru_db,1,

[2021-02-11T18:42:04.853Z]                         [{file,"src/couch_server.erl"},{line,405}]},

[2021-02-11T18:42:04.853Z]           {couch_server,handle_call,3,

[2021-02-11T18:42:04.853Z]                         [{file,"src/couch_server.erl"},{line,575}]},

[2021-02-11T18:42:04.853Z]           {gen_server,try_handle_call,4,[{file,"gen_server.erl"},{line,661}]},

[2021-02-11T18:42:04.853Z]           {gen_server,handle_msg,6,[{file,"gen_server.erl"},{line,690}]},

[2021-02-11T18:42:04.853Z]           {proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,249}]}]},

[2021-02-11T18:42:04.853Z]  {gen_server,call,

[2021-02-11T18:42:04.853Z]              [couch_server_4,

[2021-02-11T18:42:04.853Z]               {create,<<"eunit-test-db-9849e822ba346e3d33d5f388c3"...>>,[]},

[2021-02-11T18:42:04.853Z]               infinity]}}

[2021-02-11T18:42:04.853Z]   output:<<"">>

rnewson · 2021-02-12T13:17:19Z

took me way to long to see my mistake. I called couch_server:couch_server(DbName) in couch_lru.erl instead of couch_server:couch_dbs(DbName). So the new mystery is why these tests pass locally...

rnewson · 2021-02-12T13:51:20Z

@wohali the processes (and ets tables) have different names. I checked _system output and its as expected

   "mem3_reshard_sup": 0,
    "mem3_reshard_job_sup": 0,
    "erl_prim_loader": 0,
    "couch_server_16": 0,
    "mem3_reshard_dbdoc": 0,
    "couch_server_15": 0,
    "couch_epi_sup": 0,
    "couch_server_14": 0,
    "mem3_reshard": 0,
    "couch_server_13": 0,
    "init": 0,
    "couch_server_12": 0,
    "couch_server_11": 0,
    "mem3_nodes": 0,
    "couch_server_10": 0,
    "couch_server_9": 0,
    "couch_server_8": 0,
    "couch_server_7": 0,
    "couch_server_6": 0,
    "code_server": 0,
    "couch_server_5": 0,
    "couch_server_4": 0,
    "application_controller": 0,
    "httpc_sup": 0,
...

Consumers of this will need to adjust expectations for sure, though the _stats endpoint is fine, each couch_server_X process increments the same named stats as before.

rnewson · 2021-02-12T13:52:27Z

The only remaining question before merging is whether we redefine what max_dbs_open to be per scheduler, or whether I should reduce max_dbs_open by the num_servers() factor internal to each couch_server. My preference is for the latter but it's an important point to get opinions on.

rnewson · 2021-02-12T14:18:22Z

I've added the division at https://github.com/apache/couchdb/pull/3366/files#diff-2a7cfe9afa75338cd45f2ab010896e7cf24ea59b4355404f5412000cd9c7d934R296.

The idea is that instead of 1 gen_server/ets tables handling all $max_dbs_open items, we have N gen_server/ets tables handling $max_dbs_open/N items.

This is the least surprising behaviour. The alternative would be a silent multiplying of max_dbs_open by the number of schedulers, a number that is usually not consciously chosen and varies by host.

janl · 2021-02-12T14:44:32Z

If the development team is unable to put in the time to maintain test suites properly, then you have my vote.

turns out they were real bugs in this PR :)

janl

nicely done

wohali · 2021-02-12T16:43:31Z

@rnewson so no way to determine which couch_server is which without a remsh? Boo. How would I go about that anyway?

davisp · 2021-02-12T18:11:29Z

@wohali What do you mean which is which? Those message queues are the registered names. Thus, couch_server_1 literally translates to a process registered under that name that can be inspected via process_info/1 and friends.

wohali · 2021-02-12T21:05:24Z

@davisp which shard is couch_server_14 serving out?

rnewson · 2021-02-12T21:17:51Z

You can call couch_server:couch_server(DbName) from a remsh if you need to know which couch_server_X process a specific database is handled by. I'm not sure how useful that is. Typically at Cloudant we'd kill couch_server if it gained such a large message queue that it wasn't going to heal on its own, but I don't recall needing to look inside its state. What's your use case?

wohali · 2021-02-12T21:37:30Z

@rnewson Talking people through problem solving remotely, people who are not generally facile with (or are frightened by) remsh.

rnewson · 2021-02-12T21:51:20Z

Hm, ok, I'm still lost. A problem that requires looking into the state of couch_server is going to need remsh access anyway. Can you give me an example?

wohali · 2021-02-13T00:50:37Z

Never mind. Thanks for the good work on this PR.

The `fabric_rpc_tests` pollutes the state of `shards_db` which causes flakiness of other tests. This PR fixes the problem by configuring temporary `database_dir`. The important implementation detail is that we need to wait for all `couch_server` processes to restart. Before initroduction of sharded couch server in the apache#3366 this could be done as: ```erlang test_util:with_process_restart(couch_server, fun() -> config:set("couchdb", "database_dir", NewDatabaseDir) end), ``` This method has to be updated to support sharded couch_server. Following auxilary functions where added: - `couch_server:names/0` - returns list of registered names of each `couch_server` process - `test_util:with_processes_restart/{2,4}` - waits all process to be restarted returns `{Pids :: #{} | timeout, Res :: term()}` - `test_util:with_couch_server_restart/1` - waits for all `couch_server` processes to finish restart The new way of configuring `database_dir` in test suites is: ```erlang test_util:with_couch_server_restart(fun() -> config:set("couchdb", "database_dir", NewDatabaseDir) end), ```

In #3860 and #3366 we added sharding to `couch_index_server` and `couch_server`. The `_system` endpoint surfaces a "fake" message queue for each of these contining the aggregated queue size across all shards. This commit adds the same for the `_prometheus` endpoint. Originally I had thought to just filter out the per-shard queue lengths as we've not found these to be useful in Cloudant, but I'll leave them in for now for consistency with the `_system` endpoint. Arguably, we should filter in both places if there's agreement that the per-shard queue lengths are just noise.

rnewson requested review from davisp, chewbranca and janl February 10, 2021 18:20

rnewson commented Feb 10, 2021

View reviewed changes

src/couch/src/couch_server.erl

true -> ok

end;

false ->

ok

end.

close_lru() ->

Copy link

Member Author

rnewson Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No calls to this from outside the module

rnewson force-pushed the couch_server_sharding branch from 79d68cc to 20135cb Compare February 10, 2021 18:32

rnewson commented Feb 10, 2021

View reviewed changes

chewbranca approved these changes Feb 10, 2021

View reviewed changes

nickva reviewed Feb 10, 2021

View reviewed changes

rnewson force-pushed the couch_server_sharding branch 2 times, most recently from 8a4c4f0 to b815b4f Compare February 10, 2021 22:06

rnewson force-pushed the couch_server_sharding branch 2 times, most recently from 31e6379 to 7fe1d1e Compare February 10, 2021 23:13

rnewson force-pushed the couch_server_sharding branch 3 times, most recently from 7bd861e to 7fb0106 Compare February 11, 2021 18:35

rnewson force-pushed the couch_server_sharding branch 4 times, most recently from 4ac6e7d to c4b254f Compare February 12, 2021 13:13

encapsulate db_updated call in a function

d10eda2

rnewson force-pushed the couch_server_sharding branch from c4b254f to 7de2502 Compare February 12, 2021 14:15

rnewson force-pushed the couch_server_sharding branch from 7de2502 to 62b507f Compare February 12, 2021 14:38

janl approved these changes Feb 12, 2021

View reviewed changes

Shard couch_server for performance

ef1c902

rnewson force-pushed the couch_server_sharding branch from 62b507f to ef1c902 Compare February 12, 2021 15:09

rnewson merged commit be2898d into 3.x Feb 12, 2021

rnewson deleted the couch_server_sharding branch February 12, 2021 15:40

rnewson mentioned this pull request Feb 15, 2021

Add "couch_server" aggregate to _system output #3370

Merged

4 tasks

rnewson mentioned this pull request Mar 24, 2021

Couch server shard simplification #3455

Closed

4 tasks

rnewson mentioned this pull request Dec 3, 2021

Sharded couch index server #3860

Merged

4 tasks

iilyak mentioned this pull request Jan 31, 2022

Execute fabric_rpc_tests in clean database_dir #3921

Merged

4 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Couch server sharding #3366

Couch server sharding #3366

rnewson commented Feb 10, 2021 •

edited

Loading

rnewson Feb 10, 2021

rnewson Feb 10, 2021

wohali commented Feb 10, 2021

rnewson commented Feb 10, 2021

rnewson commented Feb 10, 2021

rnewson Feb 10, 2021

chewbranca left a comment

chewbranca Feb 10, 2021

rnewson Feb 10, 2021

chewbranca Feb 10, 2021

chewbranca Feb 10, 2021

rnewson Feb 10, 2021

nickva Feb 10, 2021

nickva Feb 10, 2021

rnewson Feb 10, 2021

chewbranca Feb 10, 2021

nickva Feb 10, 2021

wohali commented Feb 10, 2021 •

edited

Loading

rnewson commented Feb 10, 2021 •

edited

Loading

wohali commented Feb 11, 2021

nickva commented Feb 11, 2021 •

edited

Loading

rnewson commented Feb 12, 2021

rnewson commented Feb 12, 2021

rnewson commented Feb 12, 2021

rnewson commented Feb 12, 2021

janl commented Feb 12, 2021 •

edited

Loading

janl left a comment

wohali commented Feb 12, 2021

davisp commented Feb 12, 2021

wohali commented Feb 12, 2021

rnewson commented Feb 12, 2021

wohali commented Feb 12, 2021

rnewson commented Feb 12, 2021

wohali commented Feb 13, 2021



		name(BaseName, DbName) when is_binary(DbName) ->
		N = 1 + (erlang:crc32(DbName) rem num_servers()),

Couch server sharding #3366

Couch server sharding #3366

Conversation

rnewson commented Feb 10, 2021 • edited Loading

Overview

Testing recommendations

Related Issues or Pull Requests

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wohali commented Feb 10, 2021

rnewson commented Feb 10, 2021

rnewson commented Feb 10, 2021

Choose a reason for hiding this comment

chewbranca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wohali commented Feb 10, 2021 • edited Loading

rnewson commented Feb 10, 2021 • edited Loading

wohali commented Feb 11, 2021

nickva commented Feb 11, 2021 • edited Loading

rnewson commented Feb 12, 2021

rnewson commented Feb 12, 2021

rnewson commented Feb 12, 2021

rnewson commented Feb 12, 2021

janl commented Feb 12, 2021 • edited Loading

janl left a comment

Choose a reason for hiding this comment

wohali commented Feb 12, 2021

davisp commented Feb 12, 2021

wohali commented Feb 12, 2021

rnewson commented Feb 12, 2021

wohali commented Feb 12, 2021

rnewson commented Feb 12, 2021

wohali commented Feb 13, 2021

rnewson commented Feb 10, 2021 •

edited

Loading

wohali commented Feb 10, 2021 •

edited

Loading

rnewson commented Feb 10, 2021 •

edited

Loading

nickva commented Feb 11, 2021 •

edited

Loading

janl commented Feb 12, 2021 •

edited

Loading