You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2022/12/01 20:30:39 UTC
[GitHub] [couchdb] nickva opened a new pull request, #4284: Remove all usage of global
nickva opened a new pull request, #4284:
URL: https://github.com/apache/couchdb/pull/4284
Global has a tendency to create deadlocks [1], and when that happens replication jobs can't start on any of the nodes in the cluster.
We don't really need strict consistent locking for replication jobs. It's mostly to avoid replication jobs thrashing the same checkpoint docs back and forth between different session IDs. So, remove global to avoid any issues around it, and replace it with `pg` -- the new (Erlang 23+) process group implementation. (Technically `global` is still running in the runtime system as it's started by the `kernel` app. We just avoid interacting with it and registering any names to avoid deadlocks).
In `pg` we start a replication `scope`, and then in that scope make every RepId a process `group`. When replication processes spawn, their Pids becomes `members` of that group:
```
couch_replicator_pg (scope):
"a12c+create_target" (RepId group):
[Pid1, Pid2] (members)
...
```
As per `pg` implementation, groups are created and remove automatically as members are added/removed from it, so we don't have to do anything there.
If there are already any running Pids in the same group, we avoid starting the jobs, and fail like we did before when we used global. In the even more rare case of a race condition, when 2+ jobs do manage to start, we do a membership check before each checkpoint. One of the jobs then stops to yield to another. For simplicity pick the one running on the lowest lexicographically sorted node name to survive.
[1] https://github.com/erlang/otp/issues/6524
[2] https://www.erlang.org/doc/man/pg.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4284: Remove all usage of global
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4284:
URL: https://github.com/apache/couchdb/pull/4284#discussion_r1037552866
##########
src/couch_replicator/src/couch_replicator_pg.erl:
##########
@@ -0,0 +1,86 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+% Use pg process groups to reduce the chance of duplicate replication jobs
+% running on the same cluster.
+%
+% A custom replicator pg group is started via start_link/0. Then, replication
+% jobs check if they would be leaders before starting. If, by chance, two jobs
+% with the same RepId start anyway, then replication jobs would do an extra
+% check before each checkpoint. If the they are not leaders any longer, they
+% should stop running. The "leader" is just the first sorted element in the
+% [node(Pid), ...] list.
+
+-module(couch_replicator_pg).
+
+-export([
+ start_link/0,
+ join/2,
+ leave/2,
+ pids/1,
+ should_start/2,
+ should_run/2
+]).
+
+% Start a custom pg group. Should be called from the replication supervisor.
+%
+start_link() ->
+ pg:start_link(?MODULE).
+
+% Join a replication job pid to a RepId group
+%
+join({_, _} = RepId, Pid) when is_pid(Pid) ->
+ pg:join(?MODULE, id(RepId), Pid).
+
+% Leave a replication RepId group. This doesn't have to be called explicitly as
+% the processes are monitored and automatically removed by pg. It may be nice,
+% to call it from terminate/2 to speed things along a bit and clear the group
+% quicker.
+%
+leave({_, _} = RepId, Pid) when is_pid(Pid) ->
+ pg:leave(?MODULE, id(RepId), Pid).
+
+% Determine if a replication job should start on a particular node. If it
+% should, return `yes`, otherwise return `{no, OtherPid}`. `OtherPid` is
+% the pid of the replication job that is already running.
+%
+should_start({_, _} = RepId, Node) when is_atom(Node) ->
+ no_other_nodes(Node, pids(RepId)).
+
+% Determine if the replication job should keep running as the main job for that
+% RepId. If it is, return yes, otherwise return `{no, OtherPid}`. `OtherPid` is
+% the pid of the replication job that should stay running instead of this one.
+%
+should_run({_, _} = RepId, Pid) when is_pid(Pid) ->
+ case pids(RepId) of
+ [OtherPid | _] when OtherPid =/= Pid -> {no, OtherPid};
+ _ -> yes
Review Comment:
It follows what global did before it returned `yes | no` on registrations [1].
I had as `true | false` first. But after when adding `OtherPid` it looked odd having `true | {false, Pid}` since it's not quite a boolean answer anyway. The idea behind `OtherPid` is we want `start_link(...)` to return the same shape response as from a supervisor previously `{error, {already_started, OtherPid}}`.
[1] https://www.erlang.org/doc/man/global.html#register_name-2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4284: Remove all usage of global
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4284:
URL: https://github.com/apache/couchdb/pull/4284#discussion_r1037552866
##########
src/couch_replicator/src/couch_replicator_pg.erl:
##########
@@ -0,0 +1,86 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+% Use pg process groups to reduce the chance of duplicate replication jobs
+% running on the same cluster.
+%
+% A custom replicator pg group is started via start_link/0. Then, replication
+% jobs check if they would be leaders before starting. If, by chance, two jobs
+% with the same RepId start anyway, then replication jobs would do an extra
+% check before each checkpoint. If the they are not leaders any longer, they
+% should stop running. The "leader" is just the first sorted element in the
+% [node(Pid), ...] list.
+
+-module(couch_replicator_pg).
+
+-export([
+ start_link/0,
+ join/2,
+ leave/2,
+ pids/1,
+ should_start/2,
+ should_run/2
+]).
+
+% Start a custom pg group. Should be called from the replication supervisor.
+%
+start_link() ->
+ pg:start_link(?MODULE).
+
+% Join a replication job pid to a RepId group
+%
+join({_, _} = RepId, Pid) when is_pid(Pid) ->
+ pg:join(?MODULE, id(RepId), Pid).
+
+% Leave a replication RepId group. This doesn't have to be called explicitly as
+% the processes are monitored and automatically removed by pg. It may be nice,
+% to call it from terminate/2 to speed things along a bit and clear the group
+% quicker.
+%
+leave({_, _} = RepId, Pid) when is_pid(Pid) ->
+ pg:leave(?MODULE, id(RepId), Pid).
+
+% Determine if a replication job should start on a particular node. If it
+% should, return `yes`, otherwise return `{no, OtherPid}`. `OtherPid` is
+% the pid of the replication job that is already running.
+%
+should_start({_, _} = RepId, Node) when is_atom(Node) ->
+ no_other_nodes(Node, pids(RepId)).
+
+% Determine if the replication job should keep running as the main job for that
+% RepId. If it is, return yes, otherwise return `{no, OtherPid}`. `OtherPid` is
+% the pid of the replication job that should stay running instead of this one.
+%
+should_run({_, _} = RepId, Pid) when is_pid(Pid) ->
+ case pids(RepId) of
+ [OtherPid | _] when OtherPid =/= Pid -> {no, OtherPid};
+ _ -> yes
Review Comment:
It follows what global did before it returned `yes | no` on registrations [1].
I had it as `true | false` first. But after when adding `OtherPid` it looked odd having `true | {false, Pid}` since it's not quite a boolean answer anyway. The idea behind `OtherPid` is we want `start_link(...)` to return the same shape response as from a supervisor previously `{error, {already_started, OtherPid}}`.
[1] https://www.erlang.org/doc/man/global.html#register_name-2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4284: Remove all usage of global
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4284:
URL: https://github.com/apache/couchdb/pull/4284#discussion_r1037552866
##########
src/couch_replicator/src/couch_replicator_pg.erl:
##########
@@ -0,0 +1,86 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+% Use pg process groups to reduce the chance of duplicate replication jobs
+% running on the same cluster.
+%
+% A custom replicator pg group is started via start_link/0. Then, replication
+% jobs check if they would be leaders before starting. If, by chance, two jobs
+% with the same RepId start anyway, then replication jobs would do an extra
+% check before each checkpoint. If the they are not leaders any longer, they
+% should stop running. The "leader" is just the first sorted element in the
+% [node(Pid), ...] list.
+
+-module(couch_replicator_pg).
+
+-export([
+ start_link/0,
+ join/2,
+ leave/2,
+ pids/1,
+ should_start/2,
+ should_run/2
+]).
+
+% Start a custom pg group. Should be called from the replication supervisor.
+%
+start_link() ->
+ pg:start_link(?MODULE).
+
+% Join a replication job pid to a RepId group
+%
+join({_, _} = RepId, Pid) when is_pid(Pid) ->
+ pg:join(?MODULE, id(RepId), Pid).
+
+% Leave a replication RepId group. This doesn't have to be called explicitly as
+% the processes are monitored and automatically removed by pg. It may be nice,
+% to call it from terminate/2 to speed things along a bit and clear the group
+% quicker.
+%
+leave({_, _} = RepId, Pid) when is_pid(Pid) ->
+ pg:leave(?MODULE, id(RepId), Pid).
+
+% Determine if a replication job should start on a particular node. If it
+% should, return `yes`, otherwise return `{no, OtherPid}`. `OtherPid` is
+% the pid of the replication job that is already running.
+%
+should_start({_, _} = RepId, Node) when is_atom(Node) ->
+ no_other_nodes(Node, pids(RepId)).
+
+% Determine if the replication job should keep running as the main job for that
+% RepId. If it is, return yes, otherwise return `{no, OtherPid}`. `OtherPid` is
+% the pid of the replication job that should stay running instead of this one.
+%
+should_run({_, _} = RepId, Pid) when is_pid(Pid) ->
+ case pids(RepId) of
+ [OtherPid | _] when OtherPid =/= Pid -> {no, OtherPid};
+ _ -> yes
Review Comment:
It follows what global does, how it returns `yes | no` on registrations [1].
I had it as `true | false` first. But after when adding `OtherPid` it looked odd having `true | {false, Pid}` since it's not quite a boolean answer anyway. The idea behind `OtherPid` is we want `start_link(...)` to return the same shape response as from a supervisor previously `{error, {already_started, OtherPid}}`.
[1] https://www.erlang.org/doc/man/global.html#register_name-2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4284: Remove all usage of global
Posted by GitBox <gi...@apache.org>.
jaydoane commented on code in PR #4284:
URL: https://github.com/apache/couchdb/pull/4284#discussion_r1038327083
##########
src/couch_replicator/src/couch_replicator_scheduler_job.erl:
##########
@@ -406,6 +417,18 @@ terminate({shutdown, max_backoff}, State) ->
),
terminate_cleanup(State),
couch_replicator_notifier:notify({error, RepId, max_backoff});
+terminate({shutdown, {duplicate_job, OtherPid}}, State) ->
+ #rep_state{
+ source_name = Source,
+ target_name = Target,
+ rep_details = #rep{id = {BaseId, Ext} = RepId}
+ } = State,
+ couch_log:error(
+ "Replication `~s` (`~s` -> `~s`) with pid ~p was usurped by ~p on node ~p",
Review Comment:
"usurped" is a nice touch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva merged pull request #4284: Remove all usage of global
Posted by GitBox <gi...@apache.org>.
nickva merged PR #4284:
URL: https://github.com/apache/couchdb/pull/4284
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4284: Remove all usage of global
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4284:
URL: https://github.com/apache/couchdb/pull/4284#discussion_r1038348391
##########
src/couch_replicator/src/couch_replicator_scheduler_job.erl:
##########
@@ -406,6 +417,18 @@ terminate({shutdown, max_backoff}, State) ->
),
terminate_cleanup(State),
couch_replicator_notifier:notify({error, RepId, max_backoff});
+terminate({shutdown, {duplicate_job, OtherPid}}, State) ->
+ #rep_state{
+ source_name = Source,
+ target_name = Target,
+ rep_details = #rep{id = {BaseId, Ext} = RepId}
+ } = State,
+ couch_log:error(
+ "Replication `~s` (`~s` -> `~s`) with pid ~p was usurped by ~p on node ~p",
Review Comment:
I saw in another log line in couch_replicator_doc_processor and thought it sounded great!
In the logs it looks like:
```
[notice] Starting replication 17f4ee1a38b4e4e77a6e1b69f2bcfb06+continuous ...
[error] Replication `17f4ee1a38b4e4e77a6e1b69f2bcfb06+continuous`
(`http://127.0.0.1:61447/eunit-test-db-184e35ec5e1ba8c30d28be2783438031/` ->
`http://127.0.0.1:61447/eunit-test-db-bf60295c8cb46108c52c9a32d6ee6a1e/`)
with pid <0.5230.0> was usurped by <18199.89.0> on node 'A@1'
```
(From couch_replicator/.eunit/couch.log)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4284: Remove all usage of global
Posted by GitBox <gi...@apache.org>.
jaydoane commented on code in PR #4284:
URL: https://github.com/apache/couchdb/pull/4284#discussion_r1037547724
##########
src/couch_replicator/src/couch_replicator_pg.erl:
##########
@@ -0,0 +1,86 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+% Use pg process groups to reduce the chance of duplicate replication jobs
+% running on the same cluster.
+%
+% A custom replicator pg group is started via start_link/0. Then, replication
+% jobs check if they would be leaders before starting. If, by chance, two jobs
+% with the same RepId start anyway, then replication jobs would do an extra
+% check before each checkpoint. If the they are not leaders any longer, they
+% should stop running. The "leader" is just the first sorted element in the
+% [node(Pid), ...] list.
+
+-module(couch_replicator_pg).
+
+-export([
+ start_link/0,
+ join/2,
+ leave/2,
+ pids/1,
+ should_start/2,
+ should_run/2
+]).
+
+% Start a custom pg group. Should be called from the replication supervisor.
+%
+start_link() ->
+ pg:start_link(?MODULE).
+
+% Join a replication job pid to a RepId group
+%
+join({_, _} = RepId, Pid) when is_pid(Pid) ->
+ pg:join(?MODULE, id(RepId), Pid).
+
+% Leave a replication RepId group. This doesn't have to be called explicitly as
+% the processes are monitored and automatically removed by pg. It may be nice,
+% to call it from terminate/2 to speed things along a bit and clear the group
+% quicker.
+%
+leave({_, _} = RepId, Pid) when is_pid(Pid) ->
+ pg:leave(?MODULE, id(RepId), Pid).
+
+% Determine if a replication job should start on a particular node. If it
+% should, return `yes`, otherwise return `{no, OtherPid}`. `OtherPid` is
+% the pid of the replication job that is already running.
+%
+should_start({_, _} = RepId, Node) when is_atom(Node) ->
+ no_other_nodes(Node, pids(RepId)).
+
+% Determine if the replication job should keep running as the main job for that
+% RepId. If it is, return yes, otherwise return `{no, OtherPid}`. `OtherPid` is
+% the pid of the replication job that should stay running instead of this one.
+%
+should_run({_, _} = RepId, Pid) when is_pid(Pid) ->
+ case pids(RepId) of
+ [OtherPid | _] when OtherPid =/= Pid -> {no, OtherPid};
+ _ -> yes
Review Comment:
Is there a reason to use `yes` here rather than the usual`true`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org