finagle-redis: Add support for Redis Geo commands #628
Conversation
|
|
|
@Mura-Mi thanks for the patch/feature! gonna try to find someone who knows finagle-redis better than i to take a look at this. |
There was a problem hiding this comment.
Hi @Mura-Mi thanks so much! This is awesome!
In addition to the inline comments, I have a few notes.
- Could you please refactor things like fn defs to follow our style guidelines? https://github.com/twitter/finagle/blob/master/CONTRIBUTING.md#style
- There's no need for lazy vals on singletons and long lived objects. Can you leave them only on things like command responses?
- I think it would be helpful to link to some geohash-related documentation somewhere since not everyone is familiar with them. I suggest the redis page: https://redis.io/commands/geohash. Also, maybe a note about which versions of redis support these commands, so folks have that info at hand.
| import com.twitter.io.Buf | ||
| import com.twitter.io.Buf.Utf8 | ||
|
|
||
| case class GeoAdd(key: Buf, values: (Double, Double, Buf)*) |
There was a problem hiding this comment.
Make a case class for (Double, Double, Buf) instead of a tuple for clarity
| withCoord: JBoolean = false, | ||
| withDist: JBoolean = false, | ||
| withHash: JBoolean = false, | ||
| count: JInt = null, |
There was a problem hiding this comment.
How about we avoid nulls here and just use Option directly?
There was a problem hiding this comment.
Thanks @Mura-Mi! This looks pretty decent already! I have just a few nits around using Java numeric types (what's the reason for that?).
Also, please, don't forget to sign the CLA as our CLAsistant bot suggest.
| * | ||
| * @return Future of the number of elements added to the sorted set | ||
| */ | ||
| def geoAdd(key: Buf, values: (Double, Double, Buf)*): Future[JLong] = |
There was a problem hiding this comment.
Why this has to be Java Long?
| * @return Future of an array where each element is a tuple representing | ||
| * longitude and latitude (x,y) of each member, or `None` if member is missing. | ||
| */ | ||
| def geoPos(key: Buf, members: Buf*): Future[Seq[Option[(JDouble, JDouble)]]] = { |
There was a problem hiding this comment.
Why using Java Doubles?
| case EmptyMBulkReply => Future.Nil | ||
| case MBulkReply(positions) => Future.value(positions.map { | ||
| case MBulkReply(Seq(longitude, latitude)) => (longitude, latitude) match { | ||
| case (BulkReply(lon), BulkReply(lat)) => |
There was a problem hiding this comment.
You can inline Buf.Utf8(lon) in this pattern matching to get the string out of BulkReply(lon). This way, you won't need to call BufToString later.
| } | ||
| } | ||
|
|
||
| private lazy val geoDistResultHandler: PartialFunction[Reply, Future[Option[JDouble]]] = { |
There was a problem hiding this comment.
Similar question about JDouble.
|
|
||
| private lazy val geoDistResultHandler: PartialFunction[Reply, Future[Option[JDouble]]] = { | ||
| case EmptyBulkReply => Future.None | ||
| case BulkReply(buf) => Future.value(Utf8.unapply(buf).map(_.toDouble)) |
There was a problem hiding this comment.
Please inline Buf.Utf8() extractor here.
| * @return Future of the distance in the specified unit, | ||
| * or `None` if one or both the elements are missing. | ||
| */ | ||
| def geoDist(key: Buf, member1: Buf, member2: Buf, unit: GeoUnit = null): Future[Option[JDouble]] = { |
There was a problem hiding this comment.
I'd prefer this w/o nulls. Also why JDouble?
| case BulkReply(dist) => (r: GeoRadiusResult) => r.copy(dist = Some(BufToString(dist).toDouble)) | ||
| case MBulkReply(BulkReply(lon) :: BulkReply(lat) :: Nil) => | ||
| (r: GeoRadiusResult) => | ||
| r.copy(coord = Some((BufToString(lon).toDouble, BufToString(lat).toDouble))) |
There was a problem hiding this comment.
Please, inline Buf.Utf8() extractor instead.
| object GeoUnit { | ||
|
|
||
| /** Meter */ | ||
| case object m extends GeoUnit("m") |
There was a problem hiding this comment.
What do you think about GeoUnit.Meter, GeoUnit.Mile, etc?
There was a problem hiding this comment.
It's good. I've named like these as described in Redis document, but I'll modify.
|
@Mura-Mi this looks good to me once @vkostyukov's comments about nulls/java types are addressed. Thanks! |
|
@Mura-Mi Thanks for working on this! I still think we don't need to use Java types directly. Also, please, don't use |
Mura-Mi
force-pushed
the
feature/redis-geo
branch
from
d1e6984
to
adfc309
Compare
|
@Mura-Mi, are you still interested in getting this merged? This looks pretty close, but there appears to be a few outstanding comments. |
Mura-Mi
force-pushed
the
feature/redis-geo
branch
from
adfc309
to
533358e
Compare
|
Thank you for reviewing my pull request. I'm removing the usage of Java primitive numbers and In TravisCI, latest redis-server is not available so the Geo API Redis integration test fails. I'm trying to install |
Mura-Mi
force-pushed
the
feature/redis-geo
branch
from
533358e
to
717a421
Compare
Problem finagle-redis does not support geo index operation Solution Implement GEOADD, GEOPOS, GEOHASH, GEODIST, GEORADIUS and GEORADIUSBYMEMBER operation in finagle-redis.
Latest version of `redis-server` is not available in default TravisCI setting. To avoid Redis Geo API integration test's failing, make latest redis-server available via apt-package.
|
@Mura-Mi would it be possible to move the integration tests into finagle-redis/src/it? All redis integration tests were moved there recently. Run in sbt with |
|
@yannick-polius Is this PR replaced by d32d123 ? |
|
@mkhq yeah we were able to land it with that change. |
Problem
finagle-redis does not support geo index operation
Solution
Implement GEOADD, GEOPOS, GEOHASH, GEODIST, GEORADIUS and
GEORADIUSBYMEMBER operation in finagle-redis.