Skip to content

fix changes feed for couchbase sync gateway #289

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

Closed

Conversation

hermitdemschoenenleben
Copy link
Contributor

No description provided.

@djc
Copy link
Owner

djc commented Jun 1, 2016

Hey, thanks for looking into fixing this!

First of all, obviously this will need to pass Travis' tests to be accepted.

Second of all, I don't think we want to have both iterchunks() and iterdocs(). Can we just fix up iterchunks() to do the right thing? Can you paste some demonstrations of ways in which Couchbase breaks the current implementation? I.e., could we still split by line (but not by chunk) and get a single doc, or do they split their updates over response content lines, as well?

@hermitdemschoenenleben
Copy link
Contributor Author

Hello,

I introduced iterdocs() because I needed a quick fix and didn't want to break something. I now looked into it again and thought that I found an easier solution, however it breaks couchdb compatibility... I'll try again...

@hermitdemschoenenleben
Copy link
Contributor Author

ok, now it should work with couchdb as well as couchbase sync. How can I run Travis again for the last commit?

@djc
Copy link
Owner

djc commented Jun 5, 2016

You should probably rebase on top of master, then push to your PR branch again (with --force).

@hermitdemschoenenleben
Copy link
Contributor Author

ok, now it should work. Note however that I changed the test_double_iteration_over_same_response_body test such that the test data ends with a \n, because couchdb states that JSON messages are terminated by a newline character
(http://docs.couchdb.org/en/1.6.1/query-server/protocol.html)

@djc
Copy link
Owner

djc commented Jun 9, 2016

Fails on Python 3. Do you think you can fix that?

@hermitdemschoenenleben
Copy link
Contributor Author

Hmm, the error at travis using python 3 is "No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself", I'm not sure whether this is really caused by my small change... (I don't have the time to test it using python3 at the moment, but I will look into it...)

@djc
Copy link
Owner

djc commented Jun 14, 2016

Well, on my server I can reproduce that the Python 2 tests pass with your changes, whereas with Python 3 the tests hang. So this definitely still needs some work before it can be merged.

@hermitdemschoenenleben hermitdemschoenenleben force-pushed the master branch 2 times, most recently from 058d8e1 to 1454eb5 Compare June 21, 2016 11:33
@hermitdemschoenenleben
Copy link
Contributor Author

ok, now it should work with python 3, too.

@djc
Copy link
Owner

djc commented Jun 24, 2016

Would you mind giving me a real email address to include in your commit? It looks like you had your git commit identity email set to "ben@localhost.localdomain" when committing this. Thanks!

@hermitdemschoenenleben
Copy link
Contributor Author

damn, the tests still fail... I'll look into this again once I have a little bit more time (in about a month)

@hermitdemschoenenleben
Copy link
Contributor Author

ok, found the time to finally fix this, now the tests pass and it works with couchbase sync gateway, too :) (I also fixed my git configuration to use my right email adress)

@hermitdemschoenenleben
Copy link
Contributor Author

is there anything still missing?

@djc
Copy link
Owner

djc commented Jul 18, 2016

No, I've had this sit open in a tab waiting for some time to review. As it is, it feels like a lot of complexity to support a backend that I imagine is rare among CouchDB-Python users, so I want to take some time and see if it can be simplified (in terms of lines of code, say). Sorry that I have not yet gotten to it, I am aware that you have invested quite some time in this PR.

@djc
Copy link
Owner

djc commented Aug 5, 2016

I (finally) squashed your changes into ae055ce, and then did some followup cleanups in c88ba9e, a5cd8a2 and 88fcad9. Thanks for sticking with it, and sorry for the delay!

@hermitdemschoenenleben
Copy link
Contributor Author

Thank you for merging and don't worry about the delay!

@djc
Copy link
Owner

djc commented Aug 5, 2016

Also, I just released the 1.1 release that includes this.

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.

2 participants