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

redis backend to zipkin, CR by Dan Simon #201

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

mosesn commented Nov 8, 2012

motivation

we didn't want to use the cassandra backend, so we used made an alternative storage backend in redis.

implementation

redis data structures are generally pretty good, but we needed slightly different ways of thinking about them for zipkin. I have slightly different abstractions around them, which comprise most of this code. Index and Storage end up mostly being just about manipulating those data structures.

Contributor

johanoskarsson commented Nov 8, 2012

Thanks for contributing this back. Great to have support for more storage systems.
I'll add a few comments inline, but one general request would be to add more comments. Class level describing what the class does and how + method level if it's not immediately clear what is going on (err on the side of adding too many comments).

@johanoskarsson johanoskarsson and 1 other commented on an outdated diff Nov 8, 2012

...la/com/twitter/zipkin/config/RedisStorageConfig.scala
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.twitter.zipkin.config
+
+import com.twitter.zipkin.storage.redis.RedisStorage
+import com.twitter.finagle.redis.Client
+import com.twitter.util.Duration
+import com.twitter.conversions.time.intToTimeableNumber
+
+trait RedisStorageConfig extends StorageConfig {
+ lazy val _client: Client = Client("0.0.0.0:%d".format(port))
@johanoskarsson

johanoskarsson Nov 8, 2012

Contributor

Seems restrictive to hardcode 0.0.0.0 here. Add another val for host?

@mosesn

mosesn Nov 9, 2012

Contributor

Good point, done.

@johanoskarsson johanoskarsson and 1 other commented on an outdated diff Nov 8, 2012

.../com/twitter/zipkin/storage/redis/ExpiringValue.scala
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.twitter.zipkin.storage.redis
+
+import com.twitter.util.Time
+
+case class ExpiringValue[A](expiresAt: Time, value: A)
+
+object ExpiringValue {
+ def apply[A](expiresAt: Long, value: A): ExpiringValue[A] =
@johanoskarsson

johanoskarsson Nov 8, 2012

Contributor

Comment here describing what the expiresAt is seconds would be nice

@mosesn

mosesn Nov 9, 2012

Contributor

Done.

@johanoskarsson johanoskarsson and 1 other commented on an outdated diff Nov 8, 2012

...ala/com/twitter/zipkin/storage/redis/RedisIndex.scala
+
+ /**
+ * Store the service name, so that we easily can
+ * find out which services have been called from now and back to the ttl
+ */
+ override def indexServiceName(span: Span): Future[Unit] = Future.join(
+ for (serviceName <- span.serviceNames.toSeq
+ if serviceName != "")
+ yield serviceArray.add(serviceName))
+
+ /**
+ * Index the span name on the service name. This is so we
+ * can get a list of span names when given a service name.
+ * Mainly for UI purposes
+ */
+ override def indexSpanNameByService(span: Span): Future[Unit] = if (span.name != "") Future.join(
@johanoskarsson

johanoskarsson Nov 8, 2012

Contributor

I'd generally prefer to not cram in too much in addition to the method declaration on one line

@mosesn

mosesn Nov 9, 2012

Contributor

Moved it all over to the next line

@johanoskarsson johanoskarsson commented on the diff Nov 8, 2012

.../scala/com/twitter/zipkin/storage/redis/package.scala
+
+import com.twitter.zipkin.conversions.thrift._
+import java.nio.ByteBuffer
+import com.twitter.zipkin.gen
+import com.twitter.zipkin.common.Span
+import com.twitter.scrooge.BinaryThriftStructSerializer
+import com.twitter.util.Time
+import org.jboss.netty.buffer.ChannelBuffers
+import org.jboss.netty.util.CharsetUtil
+import java.nio.charset.Charset
+import org.jboss.netty.buffer.ChannelBuffer
+
+/**
+ * Useful conversions for encoding/decoding.
+ */
+package object redis {
@johanoskarsson

johanoskarsson Nov 8, 2012

Contributor

Some of these don't seem to be tested, worth adding a few specs

@mosesn

mosesn Nov 9, 2012

Contributor

pretty much fully tested now

@johanoskarsson johanoskarsson and 1 other commented on an outdated diff Nov 8, 2012

...com/twitter/zipkin/storage/redis/RedisIndexSpec.scala
+ doAfter {
+ redisIndex.close()
+ }
+
+ "index and get span names" in {
+ redisIndex.indexSpanNameByService(span1)()
+ redisIndex.getSpanNames("service")() mustEqual Set(span1.name)
+ }
+
+ "index and get service names" in {
+ redisIndex.indexServiceName(span1)()
+ redisIndex.getServiceNames() mustEqual Set(span1.serviceNames.head)
+ }
+
+ /*
+ "index only on annotation in each span with the same value" in {
@johanoskarsson

johanoskarsson Nov 8, 2012

Contributor

There's some old Cassandra references here that I assume can be safely removed

@mosesn

mosesn Nov 9, 2012

Contributor

Yep, completely forgot to clean up this class. Much nicer now.

Contributor

johanoskarsson commented Nov 8, 2012

For those of us that aren't familiar with redis a quick readme describing how to use this module with redis would be cool (including a quick starter on how to get redis running or a link to such a doc).

Contributor

mosesn commented Nov 8, 2012

re comments:
do you want me to add comments to private methods too, or only non-private? also, should methods which override methods in traits (which already have comments) also be commented?

Contributor

johanoskarsson commented Nov 8, 2012

For the comments you don't have to add one if the trait you extend have comments already, unless your implementation does something worth bringing up.

For private methods that are complex or hard to understand I would add a comment, but not needed for the ones where the name and parameters or the code is self explanatory. For public methods I would do the same, but lower your sensitivity to complex methods, meaning: unless it's absolutely clear what is going on I'd add a comment. Methods like getTimeToLive: Duration you don't have to add a comment to for example.

@franklinhu franklinhu and 1 other commented on an outdated diff Nov 8, 2012

...cala/com/twitter/zipkin/config/RedisIndexConfig.scala
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.twitter.zipkin.config
+
+import com.twitter.zipkin.storage.redis.RedisIndex
+import com.twitter.finagle.redis.Client
+import com.twitter.util.Duration
+import com.twitter.conversions.time.intToTimeableNumber
@franklinhu

franklinhu Nov 8, 2012

Contributor

should sort the imports on all files

@mosesn

mosesn Nov 9, 2012

Contributor

can't wait until organize imports for scala-ide can handle effective scala conventions . . . but done.

@franklinhu franklinhu and 1 other commented on an outdated diff Nov 8, 2012

...cala/com/twitter/zipkin/storage/redis/RedisHash.scala
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.twitter.zipkin.storage.redis
+
+import com.twitter.finagle.redis.Client
+import com.twitter.util.Future
+import java.nio.ByteBuffer
+import org.jboss.netty.buffer.ChannelBuffer
+
+class RedisHash(database: Client, name: String) {
@franklinhu

franklinhu Nov 8, 2012

Contributor

docs about what this does?

@mosesn

mosesn Nov 9, 2012

Contributor

added them.

@franklinhu franklinhu commented on the diff Nov 8, 2012

...ala/com/twitter/zipkin/storage/redis/RedisIndex.scala
+ */
+ override def indexSpanByAnnotations(span: Span) : Future[Unit] = Future.join(
+ {
+ def encodeAnnotation(bin: BinaryAnnotation): String = bin.annotationType match {
+ case AnnotationType.Bool => (if (bin.value.get() != 0) true else false).toString
+ case AnnotationType.Double => bin.value.getDouble.toString
+ case AnnotationType.I16 => bin.value.getShort.toString
+ case AnnotationType.I32 => bin.value.getInt.toString
+ case AnnotationType.I64 => bin.value.getLong.toString
+ case _ => ChannelBuffers.copiedBuffer(bin.value)
+ }
+
+ def binaryAnnoStringify(bin: BinaryAnnotation, service: String): String =
+ redisJoin(service, bin.key, encodeAnnotation(bin))
+
+ val time = span.lastAnnotation.get.timestamp
@franklinhu

franklinhu Nov 8, 2012

Contributor

probably shouldn't be calling .get on the Option

@mosesn

mosesn Nov 9, 2012

Contributor

good point, made it a map instead.

@franklinhu franklinhu and 1 other commented on an outdated diff Nov 8, 2012

.../twitter/zipkin/storage/redis/RedisArrayMapSpec.scala
+import com.twitter.conversions.time.intToTimeableNumber
+import org.jboss.netty.buffer.ChannelBuffers
+
+class RedisArrayMapSpec extends RedisSpecification {
+ "RedisArrayMapSpec" should {
+ var arrayMap: RedisArrayMap = null
+ val buf1 = ChannelBuffers.copiedBuffer("value1")
+ val buf2 = ChannelBuffers.copiedBuffer("value2")
+ val buf3 = ChannelBuffers.copiedBuffer("value3")
+
+ doBefore {
+ _client.flushDB()
+ arrayMap= new RedisArrayMap(_client, "prefix", None)
+ }
+
+ "be able to insert an element properly" in {
@franklinhu

franklinhu Nov 8, 2012

Contributor

for brevity we can omit the "be able to"

@mosesn

mosesn Nov 9, 2012

Contributor

done.

@franklinhu franklinhu and 1 other commented on an outdated diff Nov 9, 2012

...ala/com/twitter/zipkin/storage/redis/RedisIndex.scala
+ case AnnotationType.I64 => bin.value.getLong.toString
+ case _ => ChannelBuffers.copiedBuffer(bin.value)
+ }
+
+ def binaryAnnoStringify(bin: BinaryAnnotation, service: String): String =
+ redisJoin(service, bin.key, encodeAnnotation(bin))
+
+ val time = span.lastAnnotation.get.timestamp
+ val binaryAnnos: Seq[Future[Unit]] = span.serviceNames.toSeq flatMap { serviceName =>
+ span.binaryAnnotations map { binaryAnno =>
+ binaryAnnotationsListMap.add(
+ binaryAnnoStringify(binaryAnno, serviceName),
+ time,
+ span.traceId
+ )
+ Future.Unit
@franklinhu

franklinhu Nov 9, 2012

Contributor

This breaks the linking of the futures. If you want to convert a Future[A] into Future[Unit] you can use myFuture.unit

@mosesn

mosesn Nov 9, 2012

Contributor

neat, didn't know about that, thanks

@franklinhu franklinhu and 1 other commented on an outdated diff Nov 9, 2012

...ala/com/twitter/zipkin/storage/redis/RedisIndex.scala
+ }
+ val annos = for (serviceName <- span.serviceNames toSeq;
+ anno <- span.annotations)
+ yield annotationsListMap.add(redisJoin(serviceName, anno.value), time, span.traceId)
+ annos ++ binaryAnnos
+ }
+ )
+
+ /**
+ * Store the service name, so that we easily can
+ * find out which services have been called from now and back to the ttl
+ */
+ override def indexServiceName(span: Span): Future[Unit] = Future.join(
+ for (serviceName <- span.serviceNames.toSeq
+ if serviceName != "")
+ yield serviceArray.add(serviceName))
@franklinhu

franklinhu Nov 9, 2012

Contributor

Not a huge fan of the for(..) syntax. might be clearer as something like

span.serviceNames collect { case name if name != "" => serviceArray.add(name) }
@mosesn

mosesn Nov 9, 2012

Contributor

In general I like the for syntax, but I think you're right that collect makes it cleaner in this case.

@franklinhu franklinhu commented on an outdated diff Nov 9, 2012

...la/com/twitter/zipkin/storage/redis/RedisSetMap.scala
+ */
+
+package com.twitter.zipkin.storage.redis
+
+import com.twitter.finagle.redis.Client
+import com.twitter.finagle.redis.protocol.ZRangeResults
+import com.twitter.util.Future
+import org.jboss.netty.buffer.ChannelBuffer
+import com.twitter.util.Duration
+
+class RedisSetMap(database: Client, prefix: String, defaultTTL: Option[Duration]) {
+ private[this] def preface(key: String) = "%s:%s".format(prefix, key)
+
+ def add(key: String, buf: ChannelBuffer): Future[Unit] = database.sAdd(preface(key), List(buf)) flatMap { _ =>
+ defaultTTL match {
+ case Some(ttl) => database.expire(preface(key), ttl.inSeconds) flatMap (_ => Future.Unit)
@franklinhu

franklinhu Nov 9, 2012

Contributor

.unit instead?

@franklinhu franklinhu commented on an outdated diff Nov 9, 2012

.../twitter/zipkin/storage/redis/RedisSortedSetMap.scala
+
+import com.twitter.finagle.redis.Client
+import com.twitter.util.Future
+import org.jboss.netty.buffer.ChannelBuffer
+import com.twitter.finagle.redis.protocol.ZInterval
+import com.twitter.finagle.redis.protocol.Limit
+import com.twitter.finagle.redis.protocol.ZRangeResults
+import com.twitter.util.Duration
+
+class RedisSortedSetMap(database: Client, prefix: String, defaultTTL: Option[Duration]) {
+ def preface(key: String) = "%s:%s".format(prefix, key)
+
+ def add(key: String, score: Double, buffer: ChannelBuffer): Future[Unit] =
+ database.zAdd(preface(key), score, buffer) flatMap { _ =>
+ defaultTTL match {
+ case Some(ttl) => database.expire(preface(key), ttl.inSeconds) flatMap { _ => Future.Unit }
@franklinhu

franklinhu Nov 9, 2012

Contributor

.unit?

Contributor

mosesn commented Nov 9, 2012

Thanks for all of your prompt feedback. I've made changes that address all of the comments you've laid out for me. Looking forward to more input, thanks!

Contributor

mosesn commented Nov 9, 2012

@johanoskarsson I added a redis doc, you can probably yum/apt-get install redis-server if you're on linux and try it out yourself. Not sure how fully featured you want the redis doc to be. I have basically no experience writing technical documents except for my own gratification, so I know that I am pretty nooby. Also, is it in the right place?

re: comments, I added comments where they seemed useful. I tended toward scaladoc-style comments rather than // comments because I feel like it's easier for // comments to get separated from the code they're referring to, unless you have something like

code() //comment describing code.

@johanoskarsson johanoskarsson and 1 other commented on an outdated diff Nov 9, 2012

.../com/twitter/zipkin/storage/redis/ExpiringValue.scala
+ */
+
+package com.twitter.zipkin.storage.redis
+
+import com.twitter.util.Time
+
+/**
+ * ExpiringValue represents a value with a time to live.
+ * expiresAt is the time when ExpiringValue will expire.
+ * value is the value that will expire.
+ */
+case class ExpiringValue[A](expiresAt: Time, value: A)
+
+object ExpiringValue {
+
+ // expiresAt is a long value in seconds from the epoch.
@johanoskarsson

johanoskarsson Nov 9, 2012

Contributor

I'd actually prefer javadoc style here with a @Param expiresAt so that it shows up properly in generated docs.

@mosesn

mosesn Nov 9, 2012

Contributor

Oops, I completely forgot how @Param works, I'll go back and change all of those tonight. Good catch.

Also not sure if you're aware guys, Redis is available on Travis CI for testing purposes... you just have to ensure it's started: http://about.travis-ci.org/docs/user/database-setup/

Contributor

franklinhu commented Nov 9, 2012

@caniszczyk We have Travis CI disabled right now due to maven.twttr.com's flakiness :(

What dependencies are on maven.twttr.com that we can't publish to Sonatype OSS and Maven Central?

Finagle?

@franklinhu franklinhu commented on the diff Nov 9, 2012

...ala/com/twitter/zipkin/storage/redis/RedisIndex.scala
+ database.release()
+ }
+
+ private[this] def zRangeResultsToSeqIds(arr: ZRangeResults): Seq[IndexedTraceId] =
+ arr.asTuples map (tup => IndexedTraceId(tup._1, tup._2.toLong))
+
+ private[redis] def redisJoin(items: String*) = items.mkString(":")
+
+ override def getTraceIdsByName(serviceName: String, span: Option[String],
+ endTs: Long, limit: Int): Future[Seq[IndexedTraceId]] =
+ serviceSpanMap.get(
+ serviceName,
+ span,
+ ttl map (dur => endTs - dur.inMicroseconds),
+ endTs,
+ limit) map zRangeResultsToSeqIds
@franklinhu

franklinhu Nov 9, 2012

Contributor

would prefer to wrap each of these method blocks in braces

@mosesn

mosesn Nov 11, 2012

Contributor

The scala style guide seems to suggest that I should not use braces in this case. I'm not certain that this will become more clear if I wrap it in curly braces.

@franklinhu franklinhu commented on the diff Nov 9, 2012

...ala/com/twitter/zipkin/storage/redis/RedisIndex.scala
+ )
+
+ override def getSpanNames(service: String): Future[Set[String]] = spanMap.get(service) map (
+ strings => (strings map (new String(_))).toSet
+ )
+
+ override def indexTraceIdByServiceAndName(span: Span) : Future[Unit] = Future.join(
+ (span.serviceNames toSeq) map { serviceName =>
+ (span.lastAnnotation map { last =>
+ serviceSpanMap.put(serviceName, Some(span.name), last.timestamp, span.traceId)
+ }).getOrElse(Future.Unit)
+ }
+ )
+
+ override def indexSpanByAnnotations(span: Span) : Future[Unit] = Future.join(
+ {
@franklinhu

franklinhu Nov 9, 2012

Contributor

maybe a brace at the start of the function, and the Future.join inside?

@franklinhu franklinhu and 1 other commented on an outdated diff Nov 9, 2012

...ala/com/twitter/zipkin/storage/redis/RedisIndex.scala
+ anno <- span.annotations)
+ yield annotationsListMap.add(redisJoin(serviceName, anno.value), time, span.traceId)
+ annos ++ binaryAnnos
+ }
+ )
+
+ override def indexServiceName(span: Span): Future[Unit] = Future.join(
+ span.serviceNames.toSeq collect {case name if name != "" => serviceArray.add(name)}
+ )
+
+ override def indexSpanNameByService(span: Span): Future[Unit] =
+ if (span.name != "") Future.join(
+ for (serviceName <- span.serviceNames.toSeq
+ if serviceName != "")
+ yield spanMap.add(serviceName, span.name)
+ ) else Future.Unit
@franklinhu

franklinhu Nov 9, 2012

Contributor

would be clearer if styled like:

if (...) {
  ...
} else {
  ...
}
@mosesn

mosesn Nov 11, 2012

Contributor

The Scala style guide suggests omitting braces for if/else expressions, but I can see the argument for

if (span.name != "")
  Future.join(
    for (serviceName <- span.serviceNames.toSeq
      if serviceName != "")
      yield spanMap.add(serviceName, span.name)
  )
else
  Future.Unit

Is that more like what you would like to see?

@franklinhu

franklinhu Nov 13, 2012

Contributor

Yeah that's a bit clearer

@mosesn

mosesn Nov 13, 2012

Contributor

changed this to look like this

@franklinhu franklinhu and 1 other commented on an outdated diff Nov 9, 2012

...ala/com/twitter/zipkin/storage/redis/RedisIndex.scala
+
+ override def indexSpanNameByService(span: Span): Future[Unit] =
+ if (span.name != "") Future.join(
+ for (serviceName <- span.serviceNames.toSeq
+ if serviceName != "")
+ yield spanMap.add(serviceName, span.name)
+ ) else Future.Unit
+
+ override def indexSpanDuration(span: Span): Future[Void] = {
+ traceHash.get(span.traceId) map {
+ case None => TimeRange.fromSpan(span) map { timeRange =>
+ traceHash.put(span.traceId, timeRange)
+ }
+ case Some(bytes) => indexNewStartAndEnd(span, bytes)
+ }
+ Future.Void
@franklinhu

franklinhu Nov 9, 2012

Contributor

This breaks the future chaining. If converting to Future[Void], prefer myFuture.voided

@mosesn

mosesn Nov 9, 2012

Contributor

Does Future[Void] even use future chaining? Also, why is it Future[Void] instead of Future[Unit]?

edit: ahh, I thought Future[Void] was special, didn't realize it was just a Future wrapped around a null. Still seems wrong to use Future[Void] instead of Future[Unit].

@franklinhu

franklinhu Nov 10, 2012

Contributor

Yeah I think we should make the interface return Future[Unit] :\ I'll file a ticket

@franklinhu

franklinhu Nov 10, 2012

Contributor

we can fix it in a future review

@mosesn

mosesn Nov 13, 2012

Contributor

fixed this to use .voided for now, I'll switch it when the trait changes.

@franklinhu franklinhu and 1 other commented on an outdated diff Nov 9, 2012

...a/com/twitter/zipkin/storage/redis/RedisListMap.scala
+
+ /**
+ * Returns whether following key refers to a data structure
+ */
+ def exists(key: String): Future[Boolean] = database.exists(preface(key)) map (_.booleanValue)
+
+ /**
+ * Inserts an item into a list
+ */
+ def put(key: String, value: ChannelBuffer): Future[Unit] = {
+ val string = preface(key)
+ Future.join(Seq(database.lPush(string, List(value)),
+ defaultTTL match {
+ case Some(ttl) => database.expire(string, ttl.inSeconds)
+ case None => Future.Unit
+ }))
@franklinhu

franklinhu Nov 9, 2012

Contributor

A little confused about what this is supposed to do. It's pushing a key/value to Redis, then setting the expiration?
Is it okay for the expiration to be set before lPush does? It seems like this should be more like:

database.lPush(string, List(value)) flatMap { _ =>
  defaultTTL match {
    case Some(ttl) => database.expire(string, ttl.inSeconds).unit
    case None => Future.Unit
  }
}
@mosesn

mosesn Nov 11, 2012

Contributor

Yeah, you're right. I didn't really understand Futures when I wrote that.

@franklinhu franklinhu and 1 other commented on an outdated diff Nov 13, 2012

...tter/zipkin/storage/redis/RedisSortedSetMapSpec.scala
+import com.twitter.zipkin.common.{Annotation, BinaryAnnotation, Endpoint, Span}
+import com.twitter.zipkin.conversions.thrift.thriftAnnotationTypeToAnnotationType
+import com.twitter.zipkin.gen
+import java.nio.ByteBuffer
+import java.nio.charset.Charset
+import org.jboss.netty.buffer.ChannelBuffers
+import scala.util.Random
+
+class RedisSortedSetMapSpec extends RedisSpecification {
+ val ep = Endpoint(123, 123, "service")
+
+ def binaryAnnotation(key: String, value: String) =
+ BinaryAnnotation(
+ key,
+ ByteBuffer.wrap(value.getBytes),
+ gen.AnnotationType.String.toAnnotationType,
@franklinhu

franklinhu Nov 13, 2012

Contributor

Should use com.twitter.zipkin.common.AnnotationType.String rather than converting the generated thrift struct.

@mosesn

mosesn Nov 14, 2012

Contributor

thanks, fixed all of these.

@franklinhu franklinhu commented on an outdated diff Nov 13, 2012

...m/twitter/zipkin/storage/redis/RedisStorageSpec.scala
+
+import com.twitter.conversions.time.intToTimeableNumber
+import com.twitter.zipkin.common.{Annotation, BinaryAnnotation, Endpoint, Span}
+import com.twitter.zipkin.conversions.thrift.thriftAnnotationTypeToAnnotationType
+import com.twitter.zipkin.gen
+import java.nio.ByteBuffer
+
+class RedisStorageSpec extends RedisSpecification {
+
+ var redisStorage: RedisStorage = null
+
+ def binaryAnnotation(key: String, value: String) =
+ BinaryAnnotation(
+ key,
+ ByteBuffer.wrap(value.getBytes),
+ gen.AnnotationType.String.toAnnotationType,
@franklinhu

franklinhu Nov 13, 2012

Contributor

Same here

Contributor

franklinhu commented Nov 13, 2012

Just had two small nits. Otherwise lgtm

Contributor

franklinhu commented Nov 15, 2012

I tried running the tests, and didn't realize that they require for redis-server to be running in the background. Ideally this kind of a service dependency should be mocked out.

Contributor

mosesn commented Nov 15, 2012

They don't require redis-server to be running, they do need redis to be installed on the machine that hosts it, though. RedisCluster spins up a new instance of redis. RedisCluster is also the way that finagle-redis tests itself, so it seems like right now it is the standard.

@franklinhu franklinhu commented on an outdated diff Nov 16, 2012

...la/com/twitter/zipkin/storage/redis/RedisSetMap.scala
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.twitter.zipkin.storage.redis
+
+import com.twitter.finagle.redis.Client
+import com.twitter.util.{Duration, Future}
+import org.jboss.netty.buffer.ChannelBuffer
+
+/**
+ * RedisSetMap is a map from strings to sets.
+ * @database the redis client to use
+ * @prefix the namespace of the set
+ * @defaultTTL the timeout on the set
@franklinhu

franklinhu Nov 16, 2012

Contributor

These should be @param annotations

Contributor

franklinhu commented Nov 16, 2012

Tests pass for me. Just one last nit

Contributor

mosesn commented Nov 18, 2012

Ok, I think it should be good now. Weird, I thought I had fixed the last of the @Param problems already.

adriancole added a commit that referenced this pull request Jun 3, 2016

Merge pull request #201 from openzipkin/future-fixies
Cleans up async behavior in CassandaStorage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment