Timeout considered in milliseconds

[zedfmario]

Goal

Allowing timeouts in milliseconds

Need

For a high performance system, I needed a connection to elasticsearch, that couldn't last more that 250ms. If that case occurs, there's a fallback in memory.

Problem

There's no option for curlopt_timeout_ms developed.

Solution

A new parameter on the connection that determines whether the timeout given should be considered as milliseconds or not.

Tests

I run the tests and everything worked fine.

Feedback

Please, contact me in case of any doubt.

[ruflin]

@zedfmario The only thing I'm not 100% happy about is the naming of the function. The name at the moment is not clear that it is about the timeout. I would suggest the following names:

• millisecondTimeout (configuration variable)
• getMillisecondTimeout

What do you think?

[zedfmario]

@ruflin sounds great. I'm gonna change that ASAP.

[skymeyer]

@zedfmario did you try setting the curl parameter CURLOPT_TIMEOUT_MS in the connection configuration ? This should allow you to set your ms timeout. Curl parameters are processed in Elastica\Transport\Http::_setupCurl(). Given the fact that this method is called after CURLOPT_TIMEOUTis being set (see https://github.com/ruflin/Elastica/blob/master/lib/Elastica/Transport/Http.php#L79) the CURLOPT_TIMEOUT_MS option should override the previously set timeout (see curl docs "If both CURLOPT_TIMEOUT and CURLOPT_TIMEOUT_MS are set, the value set last will be used." - http://curl.haxx.se/libcurl/c/CURLOPT_TIMEOUT_MS.html).

I don't think it's a good idea to add yet another transport dependent configuration parameter directly in Elastica.

[zedfmario]

@skymeyer I did. But it sound a bit tricky to me to set the timeout in ms on the curl config, when the transport is allowing a timeout. It could be hard for a user to figure out that Elastica provides such functionality without digging into the code. I guessed it would be a nice-easy solution wich is actually verbose. But sure, I'm proposing and you can always decide not to include the request :)

[skymeyer]

@zedfmario this is just a matter of documentation and doesn't seem tricky at all. The global Elastica timeout parameter can be used by the different supported transports (although memcache is currently not setting it). The specific timeout in milliseconds seems more specific to one transport only. Not sure what @ruflin 's roadmap is for this, but it might make more sense to remove the legacy configuration parameters all together and move it all into the connections configuration - each of which can have transport specific settings (i.e. having a proxy configuration in the global config only makes sense for http/s). This will make it more transparent to include other transport mechanisms for those who need to.

[zedfmario]

@skymeyer I see your point and I agree your statement, that's a curl related issue and sounds fair enough to have the curl parameter overwritten.

My idea was just trying to set a new feature that could improve the user experience. It could be easily solved with documentation, sure, but I felt it more clear in this way since the user doesn't know whether Elastica uses timeout or timeout_ms by default.

[ruflin]

@zedfmario @skymeyer Sorry for the late reply.

If I look at the long term, I fully agree with @skymeyer that we should remove all the legacy parameters and move it into the transport configuration. That the documentation must be improved is obvious. So my question is if one of you two would have the time to make these adjustments? In case yes, I would suggest to "drop" this pull request and go with the new solution. This will perhaps break some backward compatibility but makes more sense for the long term.

@skymeyer @zedfmario Your thoughts?

[merk]

I dont like the idea that we add more parameters to a legacy configuration array. I'm all for the transport supporting these options and as long as the 'proper' way to create a transport is documented all should be well.

[skymeyer]

@ruflin having some backlog here, will schedule some time soon to take care of this.

[ruflin]

@skymeyer Prefect. Thx.

[ruflin]

@skymeyer Any update here?

[ruflin]

Closing this one as no updates for more then a year.