cmullaparthi / ibrowse

Erlang HTTP client
Other
516 stars 190 forks source link

Status pipeline algorithm changes? #122

Closed benjaminplee closed 9 years ago

benjaminplee commented 9 years ago

My team has been using iBrowse for some time now in one of our products without issue, thank you, but some recent scale testing found that the current find_best_connection logic was problematic with our internal load balancers between systems. After reviewing the current code we have fielded (slightly behind current master e18ea912b62b0f4a48d0f0edc8bb2f542863a88c), I see our problem is with consistent attempts at giving the "first" connection the next request, even if others have fewer outstanding in their pipeline or round-robin. I was about to begin on a patch to address this when I saw your unmerged new_pipeline branch which appears to address this same issues, along with some other items.

I am curious as to the status of this branch and what if anything you see as remaining before it can be called complete. As we have an immediate need for a modified algorithm, I am happy to help close out this feature branch or work to separate out the specific algorithm changes from the others. Please let me know what I can do to help drive this to completion.

I am also wondering about your choice to use a timestamp as part of the key-tuple for connection lookup in ets. Are you looking for order of creation to be preserved if two connections have the same speculative size? In our usage, we have a max sessions size of ~512 for traffic between our servers (network load balancer inbetween) and are seeing a significant skew of load (e.g. the first server in a 3 wide cluster gets 10-20% more load than the second, which gets 10-20% more load than the first). I believe I have traced this back to the repeated offering of work to the first connection before offering to others. I would like to find a way to more evenly distribute requests across connections with the same pipeline depth in order to ensure the long running connections don't skew overtime.

benjaminplee commented 9 years ago

Please ignore my specific question regarding the use of a timestamp in the ets key ... I missed that on decrement pipeline counter you reset it with a new timestamp. This makes sense to me and looks good for our load balancing needs.

Please let me know if there is anything I can do to help finish this feature. I have a few other small questions, but will wait to hear your thoughts on status.

cmullaparthi commented 9 years ago

Hi Benjamin

Thanks for getting in touch and offering to help. I'm snowed under at the moment but the feature is pretty much complete. I'll check this weekend if anything is left to do, if not I'll promote it to master.

It would be a great help if you can try it out and let me know if there are any issues.

Chandru

W: http://chandrusoft.wordpress.com

On 17 Nov 2014, at 19:09, Benjamin Lee notifications@github.com wrote:

Please ignore my specific question regarding the use of a timestamp in the ets key ... I missed that on decrement pipeline counter you reset it with a new timestamp. This makes sense to me and looks good for our load balancing needs.

Please let me know if there is anything I can do to help finish this feature. I have a few other small questions, but will wait to hear your thoughts on status.

— Reply to this email directly or view it on GitHub.

benjaminplee commented 9 years ago

This pull request will be closed and replaced by #123 which re-implemented the algorithm changes without the other feature changes.

benjaminplee commented 9 years ago

Any thoughts on the alternate pull request I sent along?

Thanks,

Benjamin Lee 314-678-2200 (company)

From: Chandrashekhar Mullaparthi notifications@github.com<mailto:notifications@github.com> Reply-To: cmullaparthi/ibrowse reply@reply.github.com<mailto:reply@reply.github.com> Date: Tuesday, November 18, 2014 1:18 AM To: cmullaparthi/ibrowse ibrowse@noreply.github.com<mailto:ibrowse@noreply.github.com> Cc: Benjamin Lee benjamin.lee@asynchrony.com<mailto:benjamin.lee@asynchrony.com> Subject: Re: [ibrowse] Status pipeline algorithm changes? (#122)

Hi Benjamin

Thanks for getting in touch and offering to help. I'm snowed under at the moment but the feature is pretty much complete. I'll check this weekend if anything is left to do, if not I'll promote it to master.

It would be a great help if you can try it out and let me know if there are any issues.

Chandru

W: http://chandrusoft.wordpress.com

On 17 Nov 2014, at 19:09, Benjamin Lee notifications@github.com<mailto:notifications@github.com> wrote:

Please ignore my specific question regarding the use of a timestamp in the ets key ... I missed that on decrement pipeline counter you reset it with a new timestamp. This makes sense to me and looks good for our load balancing needs.

Please let me know if there is anything I can do to help finish this feature. I have a few other small questions, but will wait to hear your thoughts on status.

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/cmullaparthi/ibrowse/pull/122#issuecomment-63431324.