monix / shade

Memcached client for Scala
MIT License
106 stars 19 forks source link

Add server-side increment / decrement support #38

Closed mspielberg closed 8 years ago

mspielberg commented 8 years ago

Memcache protocol and spymemcached support server-side atomic increment and decrement of keys with a specific format (ASCII-string of the decimal representation of an integer). This should help performance over client-managed CAS, especially for highly contented use cases.

lloydmeta commented 8 years ago

Looks like one of the new tests is failing.

That aside, this looks like a great new feature. I'll leave a few nitpicky comments.

Also, really love the new tests :)

mspielberg commented 8 years ago

Interesting, this test passes in my local environment. The error messages from the test are also malformed, so it's possible that memcached is returning a control character that is disrupting the formatting. Do you know what version of memcached is running inside the test container?

mspielberg commented 8 years ago

Alright, I was able to find the raw test output showing the actual result of

Some("00 3

123

")

This is bizarre, and the "123" being used in other tests suggests some sort of memory corruption going on.

I was able to find the version of memcache used by looking here, which lead to here showing version 1.4.13.

I've been running the tests against 1.4.20 locally, and looking through the release notes for 1.4.17 lists the following fix:

Fix for incorrect length of initial value set via binary increment protocol.

So yes, there's a bug in the memcached used in travis for exactly the behavior we're testing for, setting a default value via binary increment when the specified key doesn't already exist. How would you like to proceed?

lloydmeta commented 8 years ago

Nice find ! I think there are a few possible paths we can take

  1. Add a conditional bypass/ignore for that test if we are running on Travis or if the Memcached version is under 1.4.17
  2. Add before_scripts to Travis.yml to manually install the newer Memcached version instead of just using services: memcached. Since we are running in Container mode, we'll probably need to use the apt plugin

I'm not quite sure which one is easier or better but I would personally prefer 2.

mspielberg commented 8 years ago

Since we are running in Container mode, we'll probably need to use the apt plugin

None of the existing whitelisted apt sources are later than Ubuntu Trusty, which only has memcached 1.4.14, and it looks like Travis is no longer accepting requests for additional sources to be added to the whitelist.

Other options seem to be to compile memcached from a source or binary tarball, but not install to avoid a need for sudo, or to switch to VM-based Travis instead of container-based. I'll start investigating a GCE VM-based build.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.6%) to 84.538% when pulling 9896341d4f5d87e69e1027401cf6aa4b6680a3cf on mspielberg:master into c8819b4946ecd8f1c889e7864bb8b8aa54069f67 on alexandru:master.

lloydmeta commented 8 years ago

Cool stuff @mspielberg. Awesome work on getting Travis CI working well :)

I think this looks good right now. @alexandru do you want to merge this now and we can change the file to use result.tryComplete(Failure(ex)) in another commit?

alexandru commented 8 years ago

@lloydmeta @mspielberg yes, go ahead.

lloydmeta commented 8 years ago

@mspielberg FYI, the newly released v1.7.4 of Shade now includes your feature.