(#6730) - fix single-sided checkpointing #6734

alxndrsn · 2017-09-05T12:25:20Z

No description provided.

willholley

mostly looking good to me - just a few things that could be improved in the tests, I think. Nicely done!

willholley · 2017-09-05T16:27:36Z

packages/node_modules/pouchdb-replication/src/replicate.js

@@ -392,6 +392,9 @@ function replicate(src, target, opts, returnValue, result) {
 return;
 }
 return checkpointer.getCheckpoint().then(function (checkpoint) {
+ if (typeof result.initial_checkpoint === 'undefined') {


I'm not keen on changing the result to support the tests here. I think you can test by intercepting the _changes call as in https://github.com/pouchdb/pouchdb/blob/master/tests/integration/test.replication.js#L2400

I agree. I'll look into the suggested approach.

Change and it works, although a little weird as changes() is called twice for each call to replicate(), first with the initial checkpoint value, and then with the final checkpoint value.

willholley · 2017-09-05T16:28:09Z

packages/node_modules/pouchdb-checkpointer/src/index.js

@@ -133,9 +133,21 @@ var comparisons = {

 Checkpointer.prototype.getCheckpoint = function () {
 var self = this;
+
+ if (self.opts && self.opts.writeSourceCheckpoint && !self.opts.writeTargetCheckpoint) {


I wonder whether this should be combined with self.readOnlySource - it seems like we have two properties to express the same thing

The way I read it, there are four different options:

source \ target on off

on default previously broken

off previously broken no checkpointing

How would you recommend combining?

self.readOnlyCheckpoint is set when we detect that we don't have write permissions on the source. It seems like, once set, we'd want the behaviour to be the same as if opts.writeSourceCheckpoint is false. Looking at it again, this line probably doesn't change, but we have 2 similar-looking flags adjusting checkpoint behaviour that look like they should be combined in general

I'm not sure it's that simple, though. One flag is a request for behaviour, and the other is a reaction to discovered properties of a DB. E.g.

Checkpointer.prototype.updateSource = function (checkpoint, session) { if (this.opts.writeSourceCheckpoint) { var self = this; if (this.readOnlySource) { return Promise.resolve(true); }

Perhaps the auto-detection could be removed and an error thrown if the source is discovered to be read only when the user has requested checkpointing on both sides? OTOH the current behaviour is quite helpful...

@willholley PR for combining the two at #6738

willholley · 2017-09-05T16:29:14Z

tests/integration/test.replication.js

+ .on('complete', function (result) {
+ result.docs_read.should.equal(1);
+ result.docs_written.should.equal(1);
+ result.initial_checkpoint.should.equal(0);


as above, suggest capturing the since parameter sent to changes() rather than changing the schema of result

willholley · 2017-09-05T16:31:18Z

tests/integration/test.replication.js

+ .catch(done);
+ });
+
+ it('Test replicate on source only resumes OK', function (done) {


perhaps 'Test replication resumes when checkpointing on source only' is more descriptive

willholley · 2017-09-05T16:32:39Z

tests/integration/test.replication.js

+ result.initial_checkpoint.should.equal(0);
+ done();
+ });
+


remove whitespace

willholley · 2017-09-05T16:33:02Z

tests/integration/test.replication.js

+ result.initial_checkpoint.should.equal(1);
+ done();
+ });
+


remove whitespace

willholley · 2017-09-05T16:33:17Z

tests/integration/test.replication.js

+ .catch(done);
+ });
+
+ it('Test replicate on target only resumes OK', function (done) {


perhaps 'Test replication resumes when checkpointing on target only' is more descriptive

willholley · 2017-09-05T16:34:24Z

tests/integration/test.replication.js

+
+ var replicateOpts = { checkpoint: 'target' };
+
+ db.bulkDocs({ docs: docs.slice(0, 1) })


lots of duplication here with the previous tests. Consider refactoring so that the test body can be reused, whilst injecting different replicateOpts

alxndrsn · 2017-09-05T18:23:27Z

@willholley thanks for the feedback 🙂

afinne · 2017-09-05T20:14:12Z

tests/integration/test.replication.js

@@ -2643,15 +2759,13 @@ adapters.forEach(function (adapters) {
 var targetPut = target.put;
 target.put = putter;

- var changes = source.changes;
- source.changes = function (opts) {
+ interceptChanges(source, function(opts) {


Travis complains about this one, function(opts) vs. function (opts)

Thanks :¬)

alxndrsn · 2017-09-06T10:47:43Z

Ready for review again :¬)

alxndrsn · 2017-09-06T13:23:49Z

Test failures for https://travis-ci.org/pouchdb/pouchdb/builds/272428063:

1. `CLIENT=selenium:firefox:47.0.2 COMMAND=test`

https://travis-ci.org/pouchdb/pouchdb/jobs/272428078:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

2. `CLIENT=node SERVER=couchdb-master COMMAND=test`

https://travis-ci.org/pouchdb/pouchdb/jobs/272428087

  1) suite2 test.replication.js-http-http Test replication resumes when checkpointing is enabled:
     expected '1-g1AAAAIZeJyV0FEKgjAYwPGvDKqnjlAniM3NOZ_yJrXtm5jYeuq5blI3qZvUTWzqg0gEyuAbbPx_g5UAsMgDhJU7o6UpDeMt8YuW_mKqQK-rqiryQMHJH8wNCxNBCcLy4tBmR2fxb6w3fupdrxdExMTQYX1a9_teHybWSM2H9Ye6v_bfV4ohk4N6N_MTbn7zxL0zGJdWxmqE8WiNZ2cozamyYoTxao13ZyCjaFQ2wvi0RvMfk8aILNNRyH-r4gtSH43J' to equal 1

Looks like seq numbers on the remote DB are strings rather than integers. Can we just do something like parseInt(string.substring(0, string.indexOf('-'))) in the test?

3. `CLIENT=selenium:firefox:47.0.2 SERVER=couchdb-master COMMAND=test`

https://travis-ci.org/pouchdb/pouchdb/jobs/272428088

I don't really understand what's going wrong with this build.

4. `COMMAND=report-coverage COVERAGE=1`

https://travis-ci.org/pouchdb/pouchdb/jobs/272428095

ERROR: Coverage for lines (99.98%) does not meet global threshold (100%)

Any advice on getting these issues fixed gratefully received 🙂

willholley · 2017-09-06T13:28:45Z

Looks like seq numbers on the remote DB are strings rather than integers.
This is correct - sequence values are not guaranteed to be incremental and so cannot be compared. Is the issue that the tests assume specific values for the checkpoints? If so, you'd need to just capture the expected checkpoint value and ensure it's passed to changes verbatim.

For the firefox tests, I'll restart them - it's probably just saucelabs being flakey.

The code coverage errors indicate that some of the new code paths aren't being exercised by the tests. This is worth looking at and, ideally, adding covering tests.

Issue pouchdb#6730

afinne · 2017-09-18T05:19:12Z

I'm not sure if this is 100% relevant, but it seems like sequence numbers can't be trusted to be ordered on CouchDB: https://stackoverflow.com/questions/44447505/sequence-number-bug-in-couchdb-2-or-is-there-another-way-to-compare-sequence-num

Can we think of another way to check whether we get the desired since?

afinne · 2017-09-18T05:21:41Z

tests/integration/test.replication.js

+ .then(function () {
+ PouchDB.replicate(db, remote)
+ .on('error', done)
+ .on('complete', function (result) {


Does the result contain anything that could be useful, like seq_written or something?

This reverts commit 0f70948.

This reverts commit 84fa6d6.

This aims to fix build jobs 24 and 25

alxndrsn · 2017-11-01T12:09:56Z

I think I might have the tests passing. Just waiting for a full batch to complete on Travis…

alxndrsn · 2017-11-02T09:58:18Z

The build is passing and this is now ready for review. Happy to rebase and squash beforehand if that's helpful.

daleharvey

Awesome job, huge 👍 sorry for not being around more to help but good job getting this implemented, tested and all passing, will try to be a bit quicker helping out with follow ups :)

alxndrsn · 2017-11-03T15:48:52Z

Thanks! And extra thanks for including my unedited commit message log ;¬)

afinne · 2017-11-03T15:50:00Z

<3

willholley reviewed Sep 5, 2017

View reviewed changes

afinne reviewed Sep 5, 2017

View reviewed changes

alxndrsn added 9 commits September 6, 2017 13:55

Show that single-sided checkpoints are ignored

8626a18

Issue pouchdb#6730

Make the tests pass properly

05acd06

Re-enable other tests

a482c42

Handle missing/undefined opts

52dc8f1

Delete some whitespace

efb6077

use proper english for test names

5decf23

Refactor changes call interceptions into a separate function

124feaf

Fix whitespace

1b4bdc2

Remove initial_checkpoint from result

90aa0fd

alxndrsn force-pushed the checkpoint branch from fa979d7 to 90aa0fd Compare September 14, 2017 13:43

afinne reviewed Sep 18, 2017

View reviewed changes

alxndrsn added 12 commits October 2, 2017 10:38

Try to debug coverage report on Travis

84fa6d6

Try to run coveralls when coverage fails

0f70948

Revert "Try to run coveralls when coverage fails"

73ab95c

This reverts commit 0f70948.

Revert "Try to debug coverage report on Travis"

5408dbc

This reverts commit 84fa6d6.

Ignore uncovered line

34c6a60

Try to fix replication change checks for couch2

e982fda

check changes second round blah blah blah

a59caaa

A clearly awful approach

d720d10

lint

1210d48

Revert to working code (except on couch 2)

0ed73d4

Check expectedSince for couch 2

1d4f4b5

More debug

3a97cf8

alxndrsn added 7 commits October 5, 2017 15:12

shorter lines

b014189

Try really hard to make the test pass

fadf7f5

assert backwards just for fun

455f47c

Better feedback for failure

04537b6

run away!

ea28b2a

rebuild

373365f

jshint

8b27ae3

daleharvey mentioned this pull request Oct 30, 2017

fix Checkpointer.getCheckpoint if checkpoints are only written one db (#6730) #6804

Closed

alxndrsn added 4 commits October 31, 2017 14:18

Revert to last reasonable code

dd4cfdf

Try comparing string sinces differently

1b3097a

This aims to fix build jobs 24 and 25

Compare sinces more consistently

73cf912

This aims to fix build jobs 24 and 25

Try to fix coverage report (build job 32)

cf1abcf

daleharvey approved these changes Nov 3, 2017

View reviewed changes

daleharvey merged commit c79f1c7 into pouchdb:master Nov 3, 2017

alxndrsn deleted the checkpoint branch November 3, 2017 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(#6730) - fix single-sided checkpointing #6734

(#6730) - fix single-sided checkpointing #6734

alxndrsn commented Sep 5, 2017

willholley left a comment

willholley Sep 5, 2017

alxndrsn Sep 5, 2017

alxndrsn Sep 6, 2017

willholley Sep 5, 2017

alxndrsn Sep 5, 2017

willholley Sep 5, 2017

alxndrsn Sep 6, 2017

alxndrsn Sep 6, 2017

willholley Sep 5, 2017

willholley Sep 5, 2017

willholley Sep 5, 2017

willholley Sep 5, 2017

willholley Sep 5, 2017

willholley Sep 5, 2017

alxndrsn commented Sep 5, 2017

afinne Sep 5, 2017

alxndrsn Sep 6, 2017

alxndrsn commented Sep 6, 2017

alxndrsn commented Sep 6, 2017

willholley commented Sep 6, 2017

afinne commented Sep 18, 2017

afinne Sep 18, 2017

alxndrsn commented Nov 1, 2017

alxndrsn commented Nov 2, 2017

daleharvey left a comment

alxndrsn commented Nov 3, 2017

afinne commented Nov 3, 2017

`source` \ `target`	on	off
on	default	previously broken
off	previously broken	no checkpointing


		var replicateOpts = { checkpoint: 'target' };

		db.bulkDocs({ docs: docs.slice(0, 1) })

(#6730) - fix single-sided checkpointing #6734

(#6730) - fix single-sided checkpointing #6734

Conversation

alxndrsn commented Sep 5, 2017

willholley 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

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

alxndrsn commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxndrsn commented Sep 6, 2017

alxndrsn commented Sep 6, 2017

1. CLIENT=selenium:firefox:47.0.2 COMMAND=test

2. CLIENT=node SERVER=couchdb-master COMMAND=test

3. CLIENT=selenium:firefox:47.0.2 SERVER=couchdb-master COMMAND=test

4. COMMAND=report-coverage COVERAGE=1

willholley commented Sep 6, 2017

afinne commented Sep 18, 2017

Choose a reason for hiding this comment

alxndrsn commented Nov 1, 2017

alxndrsn commented Nov 2, 2017

daleharvey left a comment

Choose a reason for hiding this comment

alxndrsn commented Nov 3, 2017

afinne commented Nov 3, 2017

1. `CLIENT=selenium:firefox:47.0.2 COMMAND=test`

2. `CLIENT=node SERVER=couchdb-master COMMAND=test`

3. `CLIENT=selenium:firefox:47.0.2 SERVER=couchdb-master COMMAND=test`

4. `COMMAND=report-coverage COVERAGE=1`