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

Retry5xxSessionProxy + graceful reindexing #208

Closed
wants to merge 10 commits into from

Conversation

nz
Copy link
Member

@nz nz commented Apr 12, 2012

For your code reviewing pleasure.

Travis build: http://travis-ci.org/#!/sunspot/sunspot/builds/1071595

sunspot

New Retry5xxSessionProxy which will retry a 5xx error once, then gracefully return the Solr response to be parsed by the application.

This thing is basically opt-in and not on by default…

sunspot_rails

Exception: the sunspot:reindex task will now use the Retry5xxSessionProxy. I figure this is a pretty reasonable place for it, sine the reindexing is desperately brittle right now and really needs the love.

TODO

More manual integration testing of the rake task. Please do lend a hand if you have a few minutes. Even just a normal reindex on a test app to help confirm that I haven't screwed anything up.

gem 'sunspot_rails', git: 'git://github.com/sunspot/sunspot.git', branch: 'retry_5xx_session_proxy'

Still trying to decide how I'm going to set up my testing rig to generate random 500 errors. Probably a little proxymachine app in front of Solr.

/cc @fizx


Did my testing, found a few bugs, added ECONNRESET to the retryable exceptions. I also added some $stderr.puts calls to make some kind of noise to the user, but I'm not entirely sure how I feel about those being there. /shrug

@ghost ghost assigned nz Apr 12, 2012
@brupm
Copy link
Contributor

brupm commented Apr 13, 2012

Much better solution indeed.

@nz
Copy link
Member Author

nz commented Apr 13, 2012

Hmm… SemVer quandary:

Do I release this as 1.4.0 because Retry5xxSessionProxy is new functionality?

Or do I release it as 1.3.2 because it "fixes" the existing brittle reindexing behavior?

@fizx
Copy link
Contributor

fizx commented Apr 13, 2012

Hmm… SemVer quandary:

I like my bike sheds red. Joking aside, I think introduction of new apis is a minor release, not a patch, regardless of purpose.

@brupm
Copy link
Contributor

brupm commented Apr 13, 2012

My vote is for 1.3.2 as long as it does not break anything.

On Friday, April 13, 2012 at 12:47 PM, Nick Zadrozny wrote:

Hmm… SemVer quandary:

Do I release this as 1.4.0 because Retry5xxSessionProxy is new functionality?

Or do I release it as 1.3.2 because it "fixes" the existing brittle reindexing behavior?


Reply to this email directly or view it on GitHub:
#208 (comment)

@mikepack
Copy link
Contributor

Looks good. I'm seeing 500 errors in production, would love to have this as 1.3.2.

@alindeman
Copy link
Contributor

This looks good! The small qualms you have (e.g., $stderr.puts) seem valid, but also things that could be handled later.

Also let's make sure it's rebased and merged into master too, right? I think it would be good to update the README there as well with info on how to use this.

@alindeman
Copy link
Contributor

If you'd like me to handle the merge and rebase, I'd be glad to. Just say the words :)

@alindeman
Copy link
Contributor

Relabeling as for 1.3.2.

@nz
Copy link
Member Author

nz commented Apr 15, 2012

@alindeman make it so! I'm afk all day and only blocked on bikeshed issues at this point :)

Nick Zadrozny

On Sunday, April 15, 2012 at 4:40, Andy Lindeman wrote:

Relabeling as for 1.3.2.


Reply to this email directly or view it on GitHub:
#208 (comment)

@alindeman
Copy link
Contributor

1-3-stable: 1067583
master: e5eccf4

@alindeman alindeman closed this Apr 15, 2012
@dtropp
Copy link

dtropp commented Apr 16, 2012

Awesome. This is just what we are after. We've been putting up with the 500 errors for a little while now. As @nz would know, Websolr have done some good things to hopefully make these problems go away too. http://help.websolr.com/kb/status-reports/ongoing-sporadic-50x-errors-updated-15-apr-12

@nz
Copy link
Member Author

nz commented Apr 16, 2012

Fun times in the cloud!

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.

6 participants