Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Bug fixes from Crash2Burn and ActiveQuery limit by j03m #81

Merged
merged 9 commits into from Feb 24, 2013

Conversation

Projects
None yet
2 participants
Contributor

j03m commented Feb 8, 2013

Hi - we noticed that under extreme load closures from pending requests to memcached were adding up in mem and would eventually crash the node process. On our end it was better to implement a queue limit and log the overages to splunk for tracking (thus know we needed more servers) then to allow the process crash to happen.

You can view usage is examples/queue_limit.js

This also contains Crash2Burns submitted bug fixes. (we work together, I pulled his changes into my fork which we're using right now.)

Let me know what you think.

Joe Mordetsky and others added some commits Jan 28, 2013

Joe Mordetsky Added tracking for running queries and the ability to reject queries …
…if more then N are submitted at a time.
95cea29
Joe Mordetsky Fixed a bug in multi caused by the new queue limit. Modified mocha te…
…sts to point locally by default.
9f60746
Joe Mordetsky Removed the callback for multi as it will cause -- leaks. 72bb406
Joe Mordetsky command callback should not decrement active query count. f22bd98
Joe Mordetsky Removed sharding, confuses the issue. 1 server is better for replicat…
…ion of the problem.
bb060b8
Joe Mordetsky Removed debugging trace. d657569
crash2burn Fixed bug where long keys are not hashed. Fixed bug where spaces in k…
…ey name result in the callback never being called.
96470d6
Joe Mordetsky Fixed a problem with queueing that caused current active request queu…
…e count to be too aggressively torn down when using multikey-gets. This manifested as a memory leak under sustained load when using multikey gets.
090a00b
Joe Mordetsky Restoring old test settings. 59e9d62

@3rd-Eden 3rd-Eden added a commit that referenced this pull request Feb 24, 2013

@3rd-Eden 3rd-Eden Merge pull request #81 from j03m/master
Bug fixes from Crash2Burn and ActiveQuery limit by j03m
7c1eb02

@3rd-Eden 3rd-Eden merged commit 7c1eb02 into 3rd-Eden:master Feb 24, 2013

1 check passed

default The Travis build passed
Details
Owner

3rd-Eden commented Feb 24, 2013

Hey,

Apologies for the late response, I have been hitting some crazy deadlines lately. The code looks solid I'll pull and release a new version in a bit. I was looking through the code and I noticed that there aren't any tests for the for the ActiveQueue. The reason that I'm asking this as I'm working on the 1.0 release of the driver (just finished refactoring the hashring) and I want to make sure that this issue is also solved in there.

Do you have a test for this or a way to reproduce this specific issue?

Contributor

j03m commented Feb 24, 2013

I don't have a unit test - I add submit one my bad. But if you run the
queue lint sample with the queue off you can watch the proc mem grow until
crash

Sent from my iPhone

On Feb 24, 2013, at 2:30 PM, Arnout Kazemier notifications@github.com
wrote:

Hey,

Apologies for the late response, I have been hitting some crazy deadlines
lately. The code looks solid I'll pull and release a new version in a bit.
I was looking through the code and I noticed that there aren't any tests
for the for the ActiveQueue. The reason that I'm asking this as I'm working
on the 1.0 release of the driver (just finished refactoring the hashring)
and I want to make sure that this issue is also solved in there.

Do you have a test for this or a way to reproduce this specific issue?


Reply to this email directly or view it on
GitHubhttps://github.com/3rd-Eden/node-memcached/pull/81#issuecomment-14014196.

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