Monkeypatch sphinx socket timeout to 5s #51

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@willkg
Member

willkg commented Aug 13, 2012

Input gets a lot of timeouts from Sphinx. This monkeypatches the
K_TIMEOUT "constant" from 1s to 5s which should reduce the number of
timeouts we get.

Note: This is a stop-gap while we're rewriting the system.

@jsocol r?

Monkeypatch sphinx socket timeout to 5s
Input gets a lot of timeouts from Sphinx. This monkeypatches the
K_TIMEOUT "constant" from 1s to 5s which should reduce the number of
timeouts we get.

Note: This is a stop-gap while we're rewriting the system.
@jsocol

This comment has been minimized.

Show comment Hide comment
@jsocol

jsocol Aug 13, 2012

Member

Looks good to me. The tests don't really run right now so I can't give you a lot more confidence than that.

Member

jsocol commented Aug 13, 2012

Looks good to me. The tests don't really run right now so I can't give you a lot more confidence than that.

@nigelbabu

This comment has been minimized.

Show comment Hide comment
@nigelbabu

nigelbabu Aug 13, 2012

Um, have you seen 42863e4? It already monkeypatches it and the value is configurable from the settings.

Um, have you seen 42863e4? It already monkeypatches it and the value is configurable from the settings.

@willkg

This comment has been minimized.

Show comment Hide comment
@willkg

willkg Aug 13, 2012

Member

I had not seen that.

However, I don't see how that change has any effect since sphinxapi uses K_TIMEOUT to set the socket timeout and doesn't appear to use TIMEOUT at all.

Was that change tested? Do we know if it's having an effect (I'd be puzzled if it was)? Also, did we add a SPHINX_TIMEOUT setting to the settings in production?

Member

willkg commented Aug 13, 2012

I had not seen that.

However, I don't see how that change has any effect since sphinxapi uses K_TIMEOUT to set the socket timeout and doesn't appear to use TIMEOUT at all.

Was that change tested? Do we know if it's having an effect (I'd be puzzled if it was)? Also, did we add a SPHINX_TIMEOUT setting to the settings in production?

@nigelbabu

This comment has been minimized.

Show comment Hide comment
@nigelbabu

nigelbabu Aug 13, 2012

Drat. I didn't realize sphinx used K_TIMEOUT. No, it's not been tested. The idea was to use this to push that settings to production. If this works, happy to have this merged in and that one backed out.

I would ping you on IRC, but my IRC is behaving weirdly today :(

Drat. I didn't realize sphinx used K_TIMEOUT. No, it's not been tested. The idea was to use this to push that settings to production. If this works, happy to have this merged in and that one backed out.

I would ping you on IRC, but my IRC is behaving weirdly today :(

@willkg

This comment has been minimized.

Show comment Hide comment
@willkg

willkg Aug 13, 2012

Member

Boo for bad irc.

I'll tweak my patch to take into account that TIMEOUT change and also to make the default 5s.

Member

willkg commented Aug 13, 2012

Boo for bad irc.

I'll tweak my patch to take into account that TIMEOUT change and also to make the default 5s.

Rework TIMEOUT change from 0e4437f
The previous change chnaged sphinxapi.TIMEOUT rather than
sphinxapi.K_TIMEOUT so it didn't have an effect on the actual
timeout.

However, it allowed us to set the timeout based on a SPHINX_TIMEOUT
setting in settings.

This tweaks the code accordingly.
@willkg

This comment has been minimized.

Show comment Hide comment
@willkg

willkg Aug 13, 2012

Member

@jsocol @nigelbabu Does the latest commit look ok to you two?

Also, did we add a SPHINX_TIMEOUT setting to the settings in -prod? If it's set to something other than 5s, then we'll want to fix that, too.

Member

willkg commented Aug 13, 2012

@jsocol @nigelbabu Does the latest commit look ok to you two?

Also, did we add a SPHINX_TIMEOUT setting to the settings in -prod? If it's set to something other than 5s, then we'll want to fix that, too.

@willkg

This comment has been minimized.

Show comment Hide comment
@willkg

willkg Aug 13, 2012

Member

Got an irc thumbs-up from Mike.

Landed in master in 9d1d1ae.

Member

willkg commented Aug 13, 2012

Got an irc thumbs-up from Mike.

Landed in master in 9d1d1ae.

@willkg willkg closed this Aug 13, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment