Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(#2685) - fix bogus conflicts, remove 20x speedup #2685

Merged
merged 1 commit into from Aug 29, 2014
Merged

(#2685) - fix bogus conflicts, remove 20x speedup #2685

merged 1 commit into from Aug 29, 2014

Conversation

nolanlawson
Copy link
Member

@daleharvey Correct me if I'm wrong, but shouldn't the _conflicts array be empty in these cases? For the remote db it's returning ['1-cf398aa8cceefcf9b8484d02bdaee3fe'] and for the local one, ['2-cf398aa8cceefcf9b8484d02bdaee3fe']. Very bizarre.

@nolanlawson
Copy link
Member Author

Well crap. Removing the 20x include_docs=true speedup fixes the bug. Looks like we can have fast replication, we can have correct replication, but we can't have both. :(

@nolanlawson
Copy link
Member Author

This is one of the saddest commits I've had to write. Also, it's broken in LevelDB, and I don't have time to fix it right now.

@nolanlawson
Copy link
Member Author

Nevermind, it's passing.

@nolanlawson
Copy link
Member Author

Sigh. There's an intermittent:

{ failures: 
   [ { message: 'res.rows[0] is undefined',
       title: 'Replicates modified docs (issue #2636)',
       stack: '@http://127.0.0.1:8000/tests/test.replication.js:1058\nunwrap/<@http://127.0.0.1:8000/dist/pouchdb.js:8070\ndrainQueue@http://127.0.0.1:8000/dist/pouchdb.js:8101\n' } ],
  passed: 485,
  failed: 1,
  lastPassed: 'Replicates deleted docs w/ compaction' }

@nolanlawson
Copy link
Member Author

OK, figured it out. Apparently open_revs="all" doesn't mean what I think it means, and it doesn't necessarily return all the non-leaf revisions (only the leaves). So to avoid the potential URL-too-long error (which is tested btw), I had to implement a batching solution instead.

On the bright side, we have more stable and informative tests now. On the downside, we are back to slowsville for replication.

delete diffs[doc.ok._id];
}
function getDocs() {
return getRevisionOneDocs().then(getAllDocs);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ This used to have an if (src.type() === 'http') around it, so we only used this optimization for http adapters. But now that allDocs is fixed in all three adapters, we can use this optimization everywhere.

@@ -85,7 +85,6 @@ adapters.forEach(function (adapters) {
});

it('Test pull replication with many changes', function (done) {
this.timeout(20000);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the default is 30000 anyway. this is a mistake

@nolanlawson
Copy link
Member Author

Travis would be green, but this is blocked by an unrelated issue: #2690. Because when it rains, it pours.

@nolanlawson nolanlawson changed the title (#2685) - shows conflicts when there are none (#2685) - fix bogus conflicts, remove 20x speedup Aug 28, 2014
@nolanlawson
Copy link
Member Author

Green. Looking for a +1, then a 3.0.3 release.

@calvinmetcalf I know you have some other commits waiting, but I can look at them after the 3.0.3 release. This is important enough that I think it should get out soon, if you agree.

@calvinmetcalf
Copy link
Member

+1

@calvinmetcalf
Copy link
Member

Though the changes canceling one that should also be in
On Aug 28, 2014 11:28 PM, "Nolan Lawson" notifications@github.com wrote:

Green. Looking for a +1, then a 3.0.3 release.

@calvinmetcalf https://github.com/calvinmetcalf I know you have some
other commits waiting, but I can look at them after the 3.0.3 release. This
is important enough that I think it should get out soon, if you agree.


Reply to this email directly or view it on GitHub
#2685 (comment).

@nolanlawson
Copy link
Member Author

I'll look at it, but I don't think it should block 3.0.3. We can always do a 3.0.4.

@nolanlawson nolanlawson merged commit ded1b43 into master Aug 29, 2014
@nolanlawson
Copy link
Member Author

ded1b43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants