You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by nickva <gi...@git.apache.org> on 2016/03/31 18:12:02 UTC
[GitHub] couchdb-couch-replicator pull request: Better handling of multiple...
GitHub user nickva opened a pull request:
https://github.com/apache/couchdb-couch-replicator/pull/34
Better handling of multiple concurrent replications
* Reduce checkpoint frequency from 5 to 30 seconds
* Avoid calling replicator manager for ownership checks. During attempted checkpoints each replication made a gen_server call to replication manager to decide if replication should still be running on that node. Use a function instead.
COUCHDB-2979
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-couch-replicator couch-2979
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-couch-replicator/pull/34.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #34
----
commit 43f3c720811be0ad757cd9b9b823b30104b6913c
Author: Nick Vatamaniuc <va...@gmail.com>
Date: 2016-03-31T15:50:53Z
Reduce checkpoint frequency from 5 to 30 seconds
Use a macro to avoid hard-coding magic number
in two places.
COUCHDB-2979
commit 1a2eb18f0ed1a2dbca6dd6b346ae7e0f96deed95
Author: Nick Vatamaniuc <va...@gmail.com>
Date: 2016-03-31T15:58:01Z
Avoid calling replicator manager for ownership checks
During attempted checkpoint each replication made
a gen_server call to replication manager to decide
if replication should still be running that node.
The call is generally fast (10s to 100s of microseconds),
however if replication manager is blocked,
for example, fetching large filter functions,
none of the replications could checkpoint &
make progress.
Now replications call the ownership function
without going through a gen_server call.
COUCHDB-2979
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-replicator pull request: Better handling of multiple...
Posted by nickva <gi...@git.apache.org>.
Github user nickva commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/34#issuecomment-204135460
@kxepal Oh interesting. I took at look and yeah PouchDB supports _bulk_docs on _local docs ( https://pouchdb.com/guides/local-documents.html ). So the idea is that instead of PUT-ing to _local separately, replicator will just send those updates in the _bulk_docs ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-replicator pull request: Better handling of multiple...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/couchdb-couch-replicator/pull/34
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-replicator pull request: Better handling of multiple...
Posted by nickva <gi...@git.apache.org>.
Github user nickva commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/34#discussion_r58091563
--- Diff: src/couch_replicator.erl ---
@@ -76,7 +78,7 @@
target_monitor = nil,
source_seq = nil,
use_checkpoints = true,
- checkpoint_interval = 5000,
+ checkpoint_interval = ?DEFAULT_CHECKPOINT_INTERVAL,
--- End diff --
Good catch. Fixed. Thanks @kxepal
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-replicator pull request: Better handling of multiple...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/34#discussion_r58082617
--- Diff: src/couch_replicator_manager.erl ---
@@ -134,9 +134,14 @@ replication_error(#rep{id = {BaseId, _} = RepId}, Error) ->
continue(#rep{doc_id = null}) ->
{true, no_owner};
continue(#rep{id = RepId}) ->
- Owner = gen_server:call(?MODULE, {owner, RepId}, infinity),
- {node() == Owner, Owner}.
-
+ case rep_state(RepId) of
+ nil ->
+ {false, nonode};
+ #rep_state{rep = #rep{db_name = DbName, doc_id = DocId}} ->
+ Node = node(),
+ Owner = owner(DbName, DocId, [Node | nodes()]),
--- End diff --
The reason we stopped using `nodes()` and instead tracked the live set of nodes in server state was to avoid situations where, momentarily, all nodes declined to run a job (thinking that some other node will).
Now, it's possible that this didn't help but I don't know how we can be certain either way. Obviously we intend to write a better distributed job scheduler in the near future so unwinding this is probably no big deal.
If you believe it's safe to remove the protection described above, then please modify your second commit to remove the `live` member of state and all the code that manipulates it, since it will be vestigial.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-replicator pull request: Better handling of multiple...
Posted by nickva <gi...@git.apache.org>.
Github user nickva commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/34#issuecomment-204138765
@kxepal Yeah it seems like a decent optimization.
@rnewson @davisp -- Robert, Paul, what do you think? Is this doable based on how _local docs are handled? i.e. to modify _bulk_docs to handle _local then to modify replicator to use it in some cases.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-replicator pull request: Better handling of multiple...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/34#issuecomment-204136297
@nickva Right. This saves you one HTTP request and makes checkpoints more often for free. However, it doesn't removes need to do PUT to _local as not all the docs we send via _bulk_docs. But if attachments are the rare thing in the database, this will give the highest profit for free.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-replicator pull request: Better handling of multiple...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/34#discussion_r58082760
--- Diff: src/couch_replicator_manager.erl ---
@@ -134,9 +134,14 @@ replication_error(#rep{id = {BaseId, _} = RepId}, Error) ->
continue(#rep{doc_id = null}) ->
{true, no_owner};
continue(#rep{id = RepId}) ->
- Owner = gen_server:call(?MODULE, {owner, RepId}, infinity),
- {node() == Owner, Owner}.
-
+ case rep_state(RepId) of
+ nil ->
+ {false, nonode};
+ #rep_state{rep = #rep{db_name = DbName, doc_id = DocId}} ->
+ Node = node(),
+ Owner = owner(DbName, DocId, [Node | nodes()]),
--- End diff --
(The idea being that, eventually, all nodes would have rescanned with the same [node()|nodes()] value if we're tracking it in state. It could well be just the same as you are doing).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-replicator pull request: Better handling of multiple...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/34#discussion_r58083278
--- Diff: src/couch_replicator.erl ---
@@ -76,7 +78,7 @@
target_monitor = nil,
source_seq = nil,
use_checkpoints = true,
- checkpoint_interval = 5000,
+ checkpoint_interval = ?DEFAULT_CHECKPOINT_INTERVAL,
--- End diff --
You have change `5000` to `30`. That's indeed is timeout reducing, but I fear that in wrong way.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-replicator pull request: Better handling of multiple...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/34#issuecomment-204082838
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-replicator pull request: Better handling of multiple...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/34#issuecomment-204108936
@nickva Idea for future: I think it worth to steal the idea from PouchDB about sending checkpoint with _bulk_docs update. This is: 1) cheap 2) reduces the window of not-checkpointed data 3) reduces queue of processes who want to set checkpoint via separate request.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-replicator pull request: Better handling of multiple...
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/34#issuecomment-204155467
Not a bad idea. I'd not try and add it to the current replicator though as there are way too many sharp corners with the HTTP stuff for us to trip up on and accidentally fail to checkpoint often enough or something.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---