tumblr / colossus

I/O and Microservice library for Scala
Apache License 2.0
1.14k stars 96 forks source link

Inconsistency between redis and memcache interfaces #632

Closed benblack86 closed 6 years ago

benblack86 commented 6 years ago

More of a question for @DanSimon

If sending a get request to redis you should generally use mapTry:

redisClient.get(ByteString(name)).mapTry {
  case Success(value) =>
    // data found
  case Failure(_) =>
    // no data found
}

However, memcache returns an option so map is sufficient unless you want to handle the case where a incorrect response is sent by memcached.

memcacheClient.get(ByteString(name)).map {
  case Some(Value(_, data, _)) =>
    // data found
  case None =>
    // no data found
}

Any particular reason for this? Should we make it consistent? Looks like http follows the same style as redis. Which style is the best?

DanSimon commented 6 years ago

So I didn't realize this, but the redis interface has both get and getOption methods. So technically you can already do it either way with redis.

Personally I feel like the way the memcached client does it makes sense for redis (having get return an option and just get rid of getOption), but not for http, where "get" is not really key/value get.

This does bring up a bigger issue of how clients handle responses in a way that's consistent across protocols. In particular, "should protocol errors be encoded as exceptions in failed Futures/Callbacks?" . And a followup question is given a protocol, what kinds of responses are considered errors?

For question 1, I think it depends. For example, I definitely think a memcached get miss should be returned as a None option, not as something like NoDataException, but maybe a 404 for a http get should be a NotFoundException?

For question 2, again I think there's no simple answer, but I do believe we can try to categorize every type of error across protocols:

  1. Failures in which the client should retry (probably on a different host): connection errors, remote server errors, rate-limiting
  2. Failures in which retrying the request will probably not change the outcome a. malformed request b. authentication / authorization problem
  3. Failures which indicate attempting to read non-existant data
  4. Any other response explicitly marked as an error message (memcached and redis have this, not http)
  5. Others?

Once we've properly categorized responses, then we can determine which ones should become exceptions, and maybe decide if we can have some common exceptions shared across protocols.

benblack86 commented 6 years ago

I think for the particular example I raised, updating the Redis client so get returns an option would make me happy. I'll work on that first.

I'm going to punt on the extra questions as I don't think there is a simple answer :)