Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Make couch_peruser a proper Erlang app #756
This PR removes
GitHub issue number
Related Pull Requests
@chewbranca how to reproduce this?
@chewbranca on IRC:
More @chewbranca on IRC:
Bit of help from @nickva
[21:25:20] the logic is mostly isolated to https://github.com/apache/couchdb/blob/master/src/couch_replicator/src/couch_replicator_clustering.erl
I’ve done a bit of further research on this code and I think I’ve come up with a decent plan forward. I’m writing it down here for my own benefit, to see if I have all the missing parts, and for @chewbranca and @nickva to +1 the plan.
I’ve identified two independent issues with this module, both stem from it being fundamentally being designed for CouchDB 1.x and the 2.x port being done rather haphazardly in the past (by me).
Problem No. 1: this module gets started on each node in a cluster, each listening to
Problem No. 2: this module doesn’t handle the case of the node it being run on failing and restarting. This is less an issue, if Problem No. 1 isn’t solved yet, as in a typical cluster there are at least two more instances of this module running that could create the database.
But say we fix Problem No. 1 (as outlined above) so that each user creation will only ever result in a single attempt to create the associated database. Then, what should happen if in between picking up the notification to create the database and doing the DB creation, the current node becomes unavailable?
The module currently opens
A solution to this would be to add per-node high-watermark
The solution to Problem No. 1 as outline by @nickva is to re-use the replicator’s code that makes sure a replication is only run on a single node (and specifically on the node that handles the shard that the
That module is structured in a way that it can tell, given a
I propose to keep the behaviour of running the peruser module on each node in the cluster, but modify it’s changes handler to make use of
One concern is the use of
@janl I think you have the right idea there.
Technically you don't have to use the clustering ownership function but could make a copy of that function, it's pretty small and we maybe want to customize it in the future. Even better (!) if you want we can extract the function and put them in mem3, the would be the cleanest way to do it:
https://github.com/apache/couchdb/blob/master/src/couch_replicator/src/couch_replicator_clustering.erl#L198 (would be owner/2 perhaps there).
https://github.com/apache/couchdb/blob/master/src/couch_replicator/src/couch_replicator_clustering.erl#L102 (would be owner/3 then in mem3).
So now you don't depend on replicator app.
Then another thing you'd do is to handle the stable / unstable state locally in your app just like replicator clustering does. Keep a boolean flag and updated as you get stable/unstable events.
Another reason not to reuse replicator code is that we might want to have different setting for stability checks for per-users than what replicator uses. Replicator uses a minute, we might want to have something less than that.
Here is how that configuration is passed to the stability monitoring gen_server:
Note that's a gen_server that's linked against your gen-server and would be a separate instance of it running in couch_replicator, rexi and your code:
(rexi example as well: https://github.com/apache/couchdb/blob/master/src/rexi/src/rexi_server_mon.erl#L69-L70)
Also @chewbranca gets the credit for the idea of re-using replicator code, that was a good insight and should solve the problem nicely.
@nickva latest push has peruser untangled from couch_replicator_clustering and owner/3 moved into mem3.
I occasionally see this in the logs, which is somewhat concerning as we don’t want databases with lax security, but haven’t found the source of this yet.
The one additional thing I want to do before getting this merged is handling the case when there are multiple shards of the
My idea so far here is to use a similar trick to the consistent node hashing: hashing the chance notification sans shard name against the listener pid. That should ensure only one create/delete sequence to be initiated.
This also might help with the
Okay that was a nice excursion in implementing singletons, but I found out the issue was different after all: there was a duplicate call to