Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ugprade finagle-memcached to finagle-memcachedx #266

Merged
merged 6 commits into from
May 21, 2015
Merged

Ugprade finagle-memcached to finagle-memcachedx #266

merged 6 commits into from
May 21, 2015

Conversation

luciferous
Copy link
Contributor

No description provided.

luciferous
luciferous reviewed May 19, 2015
View reviewed changes
storehaus-memcache/src/main/scala/com/twitter/storehaus/memcache/MemcacheStore.scala
@@ -119,23 +120,27 @@ class MemcacheStore(val client: Client, val ttl: Duration, val flag: Int)
{
override def withPutTtl(ttl: Duration) = new MemcacheStore(client, ttl, flag)

override def get(k: String): Future[Option[ChannelBuffer]] = client.get(k)
override def get(k: String): Future[Option[ChannelBuffer]] =
client.get(k).map(_.map(ChannelBufferBuf.Owned.extract))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like if I pattern matched on the Option here to avoid the allocation of the closure and also for None case (using Future.None)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not hot, I'd prefer to just keep it as is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we've seen any regression because of this, but @ianoc might know more. Doesn't hurt to change it I guess.

@luciferous
Copy link
Contributor Author

Reran the two failures observed in Travis build, both succeeded.

$ ./sbt 'project storehaus-cache' 'testOnly com.twitter.storehaus.cache.MutableTTLCacheProperties'
[...snip...]
[info] + MutableTTLCache.If insert an item with a TTL of X, and wait X + delta then it cannot be there: OK, passed 100 tests.
[info] + MutableTTLCache.if you put with TTL t, and wait 0 time, it should always (almost, except due to scheduling) be in there: OK, passed 100 tests.
[...snip...]
[info] Passed: Total 2, Failed 0, Errors 0, Passed 2

$ ./sbt 'project storehaus-elasticsearch' 'testOnly com.twitter.storehaus.elasticsearch.ElasticSearchStoreSpecs'
[...snip...]
[info] ElasticSearchStoreSpecs:
[info] ElasticSearch Store
[info] - should Put a value
[info] - should Retrieve a value that doesnt exist
[info] - should Update a value
[info] - should Delete a value
[info] - should Put multiple values
[info] - should Retrieve values that do not exist
[info] - should Update multiple values
[info] - should Delete multiple values
[info] - should Search for  values
[info] ScalaTest
[info] Run completed in 42 seconds, 474 milliseconds.
[info] Total number of tests run: 9
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 9, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[info] Passed: Total 9, Failed 0, Errors 0, Passed 9

@luciferous
Copy link
Contributor Author

Would you like me to squash the commits before merge?

@luciferous
Avoid some allocations on get
4db5992 
Also collapsed a flatMap and pattern match for readability.
rubanm
rubanm reviewed May 20, 2015
View reviewed changes
storehaus-memcache/src/main/scala/com/twitter/storehaus/memcache/MemcacheLongStore.scala
@@ -34,9 +35,16 @@ import com.twitter.bijection.netty.ChannelBufferBijection
*/
object MemcacheLongStore {
private implicit val cb2ary = ChannelBufferBijection
private implicit val b2ary = new Bijection[Buf, Array[Byte]] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have AbstractBijection for this. A little clearer than extending the trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Bijection.scala:

/**
 * Abstract class to ease Bijection creation from Java.
 */
abstract class AbstractBijection[A, B] extends Bijection[A, B] {

Doesn't this suggest we should be implementing Bijection directly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was originally created for java compatibility yes but we have mixed usage of that in scala as well. nbd though, this can be left as is.

@rubanm
Copy link
Contributor

rubanm commented May 20, 2015

LGTM. Merge when green (or if the two flaky tests pass locally).

@rubanm
Copy link
Contributor

rubanm commented May 21, 2015

This is green now.. merging

rubanm added a commit that referenced this pull request May 21, 2015
@rubanm
Merge pull request #266 from luciferous/memcachedx
2132071 
Ugprade finagle-memcached to finagle-memcachedx
@rubanm rubanm merged commit 2132071 into twitter:develop May 21, 2015
@luciferous luciferous deleted the memcachedx branch May 21, 2015 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@luciferous @rubanm