Reduce intra cluster conflicts

[rnewson]

Overview

CouchDB issues all write requests in parallel without coordination, applying a quorum on the results of those independent actions. When updating a document concurrently this can lead to the introduction of a stored conflict if two different writes reach separate nodes first. This is undesirable.

This patch changes fabric_doc_update in the following ways;

1. Workers are no longer started immediately, but are given a unique reference each.
2. For each range in the write request, one node is chosen to "lead" the write decision (calculated as the lowest live node that hosts the shard range)
3. "Leader" workers are started.
4. Any doc update that receives "conflict" from a Leader is added to the reply dict W times and the doc updates are removed from the other (unstarted) workers. If that leaves the worker with nothing to do, it is removed entirely.

Testing recommendations

There is some existing coverage in the module itself but more testing is needed before this can be merged.

Related Issues or Pull Requests

Checklist

• Code is written and works correctly
• Changes are covered by tests
• Any new configurable parameters are documented in rel/overlay/etc/default.ini
• Documentation changes were made in the src/docs folder
• Documentation changes were backported (separated PR) to affected branches

[rnewson]

noting need for a liveness check at the start (so the 'leader' is always a live node as of the invocation) and also to consider maintenance mode (receipt of that message from a 'leader' specifically)

[nickva]

What if we hit a conflict on the 2nd reply of a W=3 request, would we still want to do a lists:duplicate(W, conflict) set of a fake replies?

[rnewson]

hmmm good question. the 2nd reply is only happening if we started the followers, and if we do that we're doing them all in parallel, so whether we send conflict based on just the 2nd reply or conflict|accepted if we got 201/409 from both, there's still the same chance of a stored conflict, it's only the status code of the response that would be wrong (we'd send 409 instead of 202, but that can happen anyway).

I had considered doing this in tiers. that is, if we hear that the 'leader' shard is down (rexi DOWN or the rexi EXIT for maintenance mode) we could recalculate and deem one of the N-1 remaining shards as the leader, and do the same logic. I didn't do it because I didn't like the latency implications, but it leads to the situation above.

I agree it would be clearer if the fake reply thing only triggers if the reply is from a leader. It won't make things much better but it'll be easier to understand later.

[nickva]

Overall it seems unusual that on a 2nd conflict reply we'd end up with with 4 total replies on an N=3 db -- the original 1 + (W=3) conflict ones. Just worried that has a chance to mess up some our logic since we're, in general, careful around getting just the right number of replies and decide what to based on that.

[nickva]

Tiny style nit: DocRest vs ReplyRest might be more clear, and we're already using them in remove_conflicts function, so it would be more consistent, too.

[rnewson]

sure

[rnewson]

I didn't change append_update_replies, so Rest1 / Rest2 are in the original.

[nickva]

Ah good point. A clean up like that (a rename for clarity) would go a long with a separate #acc{} record PR ;-)

[nickva]

A very nice improvement!

I haven't looked at it in depth yet, so had only a few style suggestions and questions.

Definitely like introducing the acc record. To help review the change easier, would be possible to split it out as a separate PR and and merge that first? It's a great improvement on its own, and it would help minimize the leader/follow bits changes only in a subsequent PR.

[rnewson]

I put the acc introduction in its own commit for exactly that reason and intend to keep those separate commits when merging.

[nickva]

I put the acc introduction in its own commit for exactly that reason and intend to keep those separate commits when merging.

That would look nice when merging, however since a bunch of notes and comments are related to just the acc change may still look nice to do a preliminary cleanup / #acc{} record PR and keep the the overall discussion concerning it in that PR then move on the main one.

[rnewson]

it's hard enough to get CI to run for one pull request. I'll do it this time but I really don't like this idea at all. I separated out the refactoring from the functional change within the PR and it makes little sense to commit one without the other.

[rnewson]

#5385 for acc record

[nickva]

it's hard enough to get CI to run for one pull request. I'll do it this time but I really don't like this idea at all. I separated out the refactoring from the functional change within the PR and it makes little sense to commit one without the other.

I think it's worthwhile. At least in this case the test failures do seem related to bulk_docs:

07:52:20  7) test bulk docs emits conflict error for duplicate doc `_id`s (BulkDocsTest)
07:52:20       test/elixir/test/bulk_docs_test.exs:124
07:52:20       Expected 201 and the same number of response rows as in request, but got
07:52:20       %HTTPotion.Response{
07:52:20         status_code: 500,
07:52:20         body: [
07:52:20           %{
07:52:20             "error" => "conflict",
07:52:20             "id" => "0",
07:52:20             "reason" => "Document update conflict."
07:52:20           },
07:52:20           %{
07:52:20             "error" => "conflict",
07:52:20             "id" => "1",
07:52:20             "reason" => "Document update conflict."
07:52:20           },
07:52:20           %{
07:52:20             "error" => "conflict",
07:52:20             "id" => "1",
07:52:20             "reason" => "Document update conflict."
07:52:20           },
07:52:20           %{
07:52:20             "error" => "error",
07:52:20             "id" => "3",
07:52:20             "reason" => "internal_server_error"
07:52:20           }
07:52:20         ],

I keep an eye on flaky failures in the CI and this not a test that usually fails. internal_server_error seems worrying there.

[rnewson]

for sure, that indicates a bug here. It took 8 runs locally to get an internal_server_error so this will be fun to track down, but it needs to be found