Skip to content
This repository

Ruben’s MySQL codec. #98

Closed
wants to merge 36 commits into from

7 participants

marius a. eriksen Sam Pullara Ruben Oanta Davi Arnaut Steve Gury Jeremy Cole Blake Matheny
marius a. eriksen
Collaborator

This is all Ruben’s code -- I’m submitting it here so we have a forum to do a code review!

Blake Matheny

Looking briefly at the code, naggati doesn't look like a dependency so you can probably drop this.

Collaborator

Yes, it was an old dependency that I forgot to remove. You can remove it.

and others added some commits
Ruben Oanta Removed unused dependency. c450b8b
Ruben Oanta Refactored a bit a2be668
Ruben Oanta Includes the start of a test suite, some refactoring, and bug fixes. …
…The most significant change

includes removing authentication from the codec pipeline and into a ServiceFactoryProxy on
the codec. This more elegantly ensures (through sequential composition) that authentication happens
in the correct order.
bfa7c99
Ruben Oanta removed unused cached files 83d3469
Ruben Oanta Moved common operations used to decode (read) and encode (write) to a…
… byte array into classes BufferReader and BufferWriter, respectively. This removed a lot of code duplication in handling the requests and responses from MySQL.
c64b77d
Ruben Oanta -Fixed an issue when decoding the packet header in the Decoder that w…
…as caused by not masking each byte with 255 and resulted in negative size values.

-Changed the client capabilities to include connectWithDB. Added a check to ensure the server can accept a schema on client connect.
-Added a more extensive decode method for EOF packets.
-Corrected an issue with the take method not correctly incrementing the offset in BufferReader.
e38b7b0
Ruben Oanta -Added the ability to read an unsigned byte and short in BufferReader.
-Fixed an issue where the Field result was being improperly decoded.
b575426
-Changed the codec to support more complex results. Decoupled the pac…
…ket defragmenter from

the primary decoder.
-Started implementing PreparedStatements on the codec.
-Added the ability to easily add/remove from a Capability bit mask.
-Fixed an issue where the LoginRequest client capability was improperly set when the database argument was None.
678d55c
Ruben Oanta -Added decoding of prepared statements in ResultDecoder.
-Fixed readLengthCodedBinary to work properly.
7a57e46
Ruben Oanta -Expanded the ResultSet class to be more useful
-Added some basic methods to the Client that use the new ResultSet capabilities.
-Many other small additions, fixes, and changes.
65c15de
Ruben Oanta Changed tabs to spaces c57b59d
Implemented an initial api that supports basic queries and prepared
statements.
9fcfc5e
Ruben Oanta -Changed the return type of prepareAndSelect of a Future[(PreparedSta…
…tement, Seq[T]] so that the prepared statement is still accesible after it is executed.

-Added a bindParameters() method to PreparedStatements to indicate that the parameters have been sent to the server.
-Added support for Time, Date, Datetime, and Timestamp fields. Still need to add support for sending those types to the mysql server.
feb4412
-Added writeTimestamp and writeDate to Buffer.scala
-Added id method to field that uses name or origName depending on which one is present.
-Simplified the query method in Client.
7dc508f
Added better error handling for Query.* methods. 6e5503f
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/MySQL.scala
((14 lines not shown))
  14
+  def client = Function.const {
  15
+    new Codec[Request, Result] {
  16
+
  17
+      def pipelineFactory = new ChannelPipelineFactory {
  18
+        def getPipeline = {
  19
+          val pipeline = Channels.pipeline()
  20
+
  21
+          pipeline.addLast("frameDecoder", new PacketFrameDecoder)
  22
+          pipeline.addLast("resultDecoder", new ResultDecoder)
  23
+          pipeline.addLast("requestEncoder", RequestEncoder)
  24
+
  25
+          pipeline
  26
+        }
  27
+      }
  28
+
  29
+      /* Authenticate each connection before returning it via a ServiceFactoryProxy. */
1
marius a. eriksen Collaborator

style: use // comments except for scala docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/MySQL.scala
((26 lines not shown))
  26
+        }
  27
+      }
  28
+
  29
+      /* Authenticate each connection before returning it via a ServiceFactoryProxy. */
  30
+      override def prepareConnFactory(underlying: ServiceFactory[Request, Result]) = 
  31
+        new AuthenticationProxy(underlying, username, password, database)
  32
+
  33
+    }
  34
+  }
  35
+}
  36
+
  37
+class AuthenticationProxy(underlying: ServiceFactory[Request, Result], 
  38
+                          username: String, 
  39
+                          password: String,
  40
+                          database: Option[String]) 
  41
+  extends ServiceFactoryProxy(underlying) {
1
marius a. eriksen Collaborator

style:

class AuthenticationProxy(
    underlying: ServiceFactory[Request, Result], 
    username: String, 
    password: String,
    database: Option[String]) 
  extends ServiceFactoryProxy(underlying) {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/MySQL.scala
((27 lines not shown))
  27
+      }
  28
+
  29
+      /* Authenticate each connection before returning it via a ServiceFactoryProxy. */
  30
+      override def prepareConnFactory(underlying: ServiceFactory[Request, Result]) = 
  31
+        new AuthenticationProxy(underlying, username, password, database)
  32
+
  33
+    }
  34
+  }
  35
+}
  36
+
  37
+class AuthenticationProxy(underlying: ServiceFactory[Request, Result], 
  38
+                          username: String, 
  39
+                          password: String,
  40
+                          database: Option[String]) 
  41
+  extends ServiceFactoryProxy(underlying) {
  42
+  val greet = new SimpleRequest(Command.COM_NOOP_GREET)
1
marius a. eriksen Collaborator

private[this] val ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/PacketFrameDecoder.scala
((17 lines not shown))
  17
+ */
  18
+class PacketFrameDecoder extends FrameDecoder {
  19
+  override def decode(ctx: ChannelHandlerContext, channel: Channel, buffer: ChannelBuffer): Packet = {
  20
+    if(buffer.readableBytes < Packet.headerSize)
  21
+      return null
  22
+
  23
+    val header = new Array[Byte](Packet.headerSize)
  24
+    buffer.readBytes(header)
  25
+    val br = new BufferReader(header)
  26
+
  27
+    val (length, seq) = (br.readInt24, br.readByte)
  28
+
  29
+    if(buffer.readableBytes < length)
  30
+      return null
  31
+
  32
+    println("<- Decoding MySQL packet (length=%d, seq=%d)".format(length, seq))
3
Sam Pullara
spullara added a note

println--

marius a. eriksen Collaborator

spurious println

marius a. eriksen Collaborator

btw-- if you want to retain the debugging information here -- it's often useful -- you should probably either (1) use logging at a debug level, or (2) pass a debug flag into the codec constructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/MySQL.scala
((34 lines not shown))
  34
+  }
  35
+}
  36
+
  37
+class AuthenticationProxy(underlying: ServiceFactory[Request, Result], 
  38
+                          username: String, 
  39
+                          password: String,
  40
+                          database: Option[String]) 
  41
+  extends ServiceFactoryProxy(underlying) {
  42
+  val greet = new SimpleRequest(Command.COM_NOOP_GREET)
  43
+
  44
+  def makeLoginReq(sg: ServersGreeting) = LoginRequest(
  45
+            username = username,
  46
+            password = password,
  47
+            database = database,
  48
+            serverCap = sg.serverCap,
  49
+            salt = sg.salt
1
marius a. eriksen Collaborator

style: using x = x stutters a lot, try instead

def makeLoginReq(sg: ServerGreeting) = 
  LoginRequest(username, password, database, sg.serverCap, sg.salt)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/RequestEncoder.scala
... ...
@@ -0,0 +1,23 @@
  1
+package com.twitter.finagle.mysql.codec
  2
+
  3
+import com.twitter.finagle.mysql.protocol._
  4
+import com.twitter.finagle.mysql.util.BufferUtil
  5
+import org.jboss.netty.buffer.{ChannelBuffers, ChannelBuffer}
  6
+import org.jboss.netty.channel.{Channel, ChannelHandlerContext, MessageEvent, Channels, ChannelEvent}
  7
+import org.jboss.netty.handler.codec.oneone.OneToOneEncoder
  8
+
  9
+object RequestEncoder extends OneToOneEncoder {
  10
+  override def encode(context: ChannelHandlerContext, channel: Channel, message: AnyRef) = {
  11
+    message match {
  12
+      case req: SimpleRequest if req.cmd == Command.COM_NOOP_GREET => 
  13
+        ChannelBuffers.EMPTY_BUFFER
  14
+      case req: Request =>
  15
+        println("-> Encoding " + req)
1
Sam Pullara
spullara added a note

println--

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Buffer.scala
((8 lines not shown))
  8
+ * in little endian byte order.
  9
+ */
  10
+
  11
+class BufferReader(val buffer: Array[Byte], private[this] var offset: Int = 0) {
  12
+  require(offset >= 0)
  13
+  require(buffer != null)
  14
+
  15
+  def readable(width: Int = 1): Boolean = offset + width <= buffer.size
  16
+
  17
+   /**
  18
+   * Reads multi-byte numeric values stored in a byte array. 
  19
+   * Starts at offset and reads offset+width bytes. The values are
  20
+   * assumed to be stored with the low byte first at data(offset) 
  21
+   * (i.e. little endian) and the result is returned as a Long.
  22
+   */
  23
+  def read(width: Int): Long = {
3
Sam Pullara
spullara added a note

I'm concerned that this code is too functional at such a low level. I can imagine it being a hotspot. Obviously we will see when we do performance testing but usually you want C-like code when doing operations like this one to avoid a lot of CPU and memory overhead.

marius a. eriksen Collaborator

let's measure first, cut later. i'd like to focus on correctness and clarity first, then performance later -- and of course to make sure that the codec isn't structured in a way that wouldn't allow us to squeeze all the performance we can out of it later.

marius a. eriksen Collaborator

(though i do agree-- this seems likely to be a hotspot)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Buffer.scala
((53 lines not shown))
  53
+  */
  54
+  def readLengthCodedBinary: Long = {
  55
+    val firstByte = readByte
  56
+    if(firstByte < 251)
  57
+      firstByte
  58
+    else
  59
+      firstByte match {
  60
+        case 252 => read(2)
  61
+        case 253 => read(3)
  62
+        case 254 => read(8)
  63
+        case _ => -1 //NULL
  64
+      }
  65
+  }
  66
+
  67
+  def readNullTerminatedString: String = {
  68
+    val result = new StringBuilder()
1
Sam Pullara
spullara added a note

I'm surprised there is no reference to the charset here. I think this only works if the server and client are both in the same one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Buffer.scala
((70 lines not shown))
  70
+      result += readByte.toChar
  71
+
  72
+    readByte //consume null byte
  73
+    result.toString
  74
+  }
  75
+
  76
+  def readLengthCodedString: String = {
  77
+    val size = readUnsignedByte
  78
+
  79
+    if(size == 0xFB) 
  80
+      return "" // NULL string.
  81
+
  82
+    val strBytes = new Array[Byte](size)
  83
+    Array.copy(buffer, offset, strBytes, 0, size)
  84
+    offset += size
  85
+    new String(strBytes)
1
Sam Pullara
spullara added a note

This definitely needs a reference to the charset encoding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Buffer.scala
((98 lines not shown))
  98
+    if(readable(4)) {
  99
+      year = readUnsignedShort
  100
+      month = readUnsignedByte
  101
+      day = readUnsignedByte
  102
+    }
  103
+
  104
+    if(readable(3)) {
  105
+      hour = readUnsignedByte
  106
+      min = readUnsignedByte
  107
+      sec = readUnsignedByte
  108
+    }
  109
+
  110
+    if(readable(4))
  111
+      nano = readInt
  112
+
  113
+    val fmt = "%04d-%02d-%02d %02d:%02d:%02d"
2
Sam Pullara
spullara added a note

String.format is very inefficient. I suggest using a StringBuilder.

Edit: On second thought you might use a Calendar object instead that doesn't require us to format and then parse a string to get a timestamp.

Ruben Oanta Collaborator
roanta added a note

I was wondering where all the set* methods went for Dates. I'll change these methods to use Calendar instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Buffer.scala
((120 lines not shown))
  120
+
  121
+  /**
  122
+   * Read a MySQL binary encoded Time field from the buffer.
  123
+   */
  124
+   def readTime: Time = {
  125
+    val len = readUnsignedByte
  126
+    if(len == 0)
  127
+      return new Time(0)
  128
+
  129
+    val sign = if(readByte == 1) -1 else 1
  130
+    val days = readInt
  131
+    val hour = readUnsignedByte
  132
+    val min = readUnsignedByte
  133
+    val sec = readUnsignedByte
  134
+
  135
+    Time.valueOf("%02d:%02d:%02d".format(hour, min, sec))
3
Sam Pullara
spullara added a note

Again using Calendar here?

Davi Arnaut
darnaut added a note

Also, you need to convert days to hours. I guess using Calendar will solve.

Ruben Oanta Collaborator
roanta added a note

It seems like the SQL Time type is more accurately represented by a duration because time can also be negative. Unless I am missing something, the current Connector/J driver does not support negative Time. It will throw a SQLException if you try to getTime with a negative time value in the database because it uses java.sql.Time to represent SQL Time.

EDIT: java.sql.Time also doesn't support hour values greater than 24.

Is this behavior we want to mimic? Or are we actually interested in negative Time values?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Buffer.scala
((169 lines not shown))
  169
+  def fill(n: Int, b: Byte) = {
  170
+    (offset until offset + n) foreach { j => buffer(j) = b ; offset += 1 }
  171
+    this
  172
+  }
  173
+  def fillRest(b: Byte) = fill(buffer.size - offset, b)
  174
+
  175
+  def writeNullTerminatedString(str: String) = {
  176
+    Array.copy(str.getBytes, 0, buffer, offset, str.length)
  177
+    buffer(offset + str.length) = '\0'.toByte
  178
+    offset += str.length + 1
  179
+    this
  180
+  }
  181
+
  182
+  def writeLengthCodedString(str: String) = {
  183
+    buffer(offset) = str.length.toByte
  184
+    Array.copy(str.getBytes, 0, buffer, offset+1, str.length)
2
Sam Pullara
spullara added a note

Need an explicit charset. I suggest that we require UTF-8 on both sides.

marius a. eriksen Collaborator

i'm sure the protocol has something to say about this, right? probably it's even configurable? :-(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/MySQL.scala
((53 lines not shown))
  53
+    self(conn) flatMap { service => service(greet) flatMap { 
  54
+        case sg: ServersGreeting if sg.serverCap.has(Capability.protocol41) =>
  55
+          Future.value(sg)
  56
+
  57
+        case sg: ServersGreeting => 
  58
+          Future.exception(IncompatibleServerVersion)
  59
+        } flatMap { sg => service(makeLoginReq(sg)) flatMap {
  60
+            case result: OK => 
  61
+              Future.value(service)
  62
+
  63
+            case Error(c, s, m) => 
  64
+              Future.exception(ServerError("Error when authenticating the client "+ c + " - " + m))  
  65
+          }
  66
+        }
  67
+      }
  68
+    }
1
marius a. eriksen Collaborator

here it’s a little difficult to keep track of the scope of the futures and flatMaps. it might help to separate out concerns a little, and flatten things out eg.:

def acceptGreeting(res: Result) = sg match {
  case sg: ServersGreeting if sg.serverCap.has(Capability.protocol41) =>
    Future.value(())
  case sg: ServersGreeting =>
    Future.exception(Incomatib..)
  case v =>
    Future.exception(new Exception("invalid reply type %s".format(v.getClass.getName))
}

def acceptLogin(res: Result) = res match {
 ...
}

// Then the login sequence is very explicit, and flattened by using a
// for comprehension.
def apply(conn: ClientConnection) = for {
  service <- self(conn)
  sg <- service(greet)
  _ <- acceptGreeting(sg)
  res <- service(makeLoginReq(sg))
  _ <- acceptLogin(res)
} yield service
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Request.scala
((93 lines not shown))
  93
+      case t: Timestamp       => setType(Types.TIMESTAMP)
  94
+      case d: Date            => setType(Types.DATE)
  95
+      case b: Byte            => setType(Types.TINY)
  96
+      case s: Short           => setType(Types.SHORT)
  97
+      case i: Int             => setType(Types.LONG)
  98
+      case l: Long            => setType(Types.LONGLONG)
  99
+      case f: Float           => setType(Types.FLOAT)
  100
+      case d: Double          => setType(Types.DOUBLE)
  101
+      case NullValue(code)    => setType(code)
  102
+      case null               => setType(Types.NULL)
  103
+      case _ => throw new IllegalArgumentException("Unhandled query parameter type for " +
  104
+            param + " type " + param.asInstanceOf[Object].getClass.getName)
  105
+    }
  106
+  }
  107
+
  108
+  private def convertToBinary(parameters: List[Any]): Array[Byte] = {
2
Sam Pullara
spullara added a note

Making a new buffer, and a new writer per parameter seems pretty expensive. Can we refactor this to use a single buffer? Also, it generates multiple copies per parameter.

Ruben Oanta Collaborator
roanta added a note

Yes, this can be solved by precomputing the size the parameters will take and using 1 buffer writer. This would eliminate the copying and the object creation.

In retrospect, I am not sure how I reasoned that all this object creation was okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/PacketFrameDecoder.scala
((1 lines not shown))
  1
+package com.twitter.finagle.mysql.codec
  2
+
  3
+import com.twitter.finagle.mysql.protocol.{Packet, BufferReader}
  4
+import com.twitter.finagle.mysql.util.BufferUtil
  5
+import org.jboss.netty.buffer.ChannelBuffer
  6
+import org.jboss.netty.channel.{Channel, ChannelHandlerContext}
  7
+import org.jboss.netty.handler.codec.frame.FrameDecoder
  8
+
  9
+/**
  10
+ * MySQL packets are a length encoded set of bytes written
  11
+ * in little endian byte order.
  12
+ *
  13
+ * The built in LengthFieldBasedFrameDecoder in Netty
  14
+ * doesn't seem to support byte buffers that are encoded in
  15
+ * little endian. Thus, a simple custom FrameDecoder is
  16
+ * needed to defrag a ChannelBuffer into a logical MySQL packet.
1
marius a. eriksen Collaborator

It does: Endianess is Netty (and in Java generally) is handled by the underlying ChannelBuffer. So to do this “properly” you could either demand a little endian ChannelBuffer factory, or override the LengthFieldBasedFrameDecoder and wrap the incoming buffer with one that is little endian.

However, this is so trivial, that it’s both simpler and clearer to do it “manually” as you have done.

But remove the comment I think :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/PacketFrameDecoder.scala
((5 lines not shown))
  5
+import org.jboss.netty.buffer.ChannelBuffer
  6
+import org.jboss.netty.channel.{Channel, ChannelHandlerContext}
  7
+import org.jboss.netty.handler.codec.frame.FrameDecoder
  8
+
  9
+/**
  10
+ * MySQL packets are a length encoded set of bytes written
  11
+ * in little endian byte order.
  12
+ *
  13
+ * The built in LengthFieldBasedFrameDecoder in Netty
  14
+ * doesn't seem to support byte buffers that are encoded in
  15
+ * little endian. Thus, a simple custom FrameDecoder is
  16
+ * needed to defrag a ChannelBuffer into a logical MySQL packet.
  17
+ */
  18
+class PacketFrameDecoder extends FrameDecoder {
  19
+  override def decode(ctx: ChannelHandlerContext, channel: Channel, buffer: ChannelBuffer): Packet = {
  20
+    if(buffer.readableBytes < Packet.headerSize)
1
marius a. eriksen Collaborator

whitespace: if (

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/PacketFrameDecoder.scala
((16 lines not shown))
  16
+ * needed to defrag a ChannelBuffer into a logical MySQL packet.
  17
+ */
  18
+class PacketFrameDecoder extends FrameDecoder {
  19
+  override def decode(ctx: ChannelHandlerContext, channel: Channel, buffer: ChannelBuffer): Packet = {
  20
+    if(buffer.readableBytes < Packet.headerSize)
  21
+      return null
  22
+
  23
+    val header = new Array[Byte](Packet.headerSize)
  24
+    buffer.readBytes(header)
  25
+    val br = new BufferReader(header)
  26
+
  27
+    val (length, seq) = (br.readInt24, br.readByte)
  28
+
  29
+    if(buffer.readableBytes < length)
  30
+      return null
  31
+
1
marius a. eriksen Collaborator

You'll need to save the reader index here so that it is restored next time. See the FrameDecoder example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/PacketFrameDecoder.scala
((12 lines not shown))
  12
+ *
  13
+ * The built in LengthFieldBasedFrameDecoder in Netty
  14
+ * doesn't seem to support byte buffers that are encoded in
  15
+ * little endian. Thus, a simple custom FrameDecoder is
  16
+ * needed to defrag a ChannelBuffer into a logical MySQL packet.
  17
+ */
  18
+class PacketFrameDecoder extends FrameDecoder {
  19
+  override def decode(ctx: ChannelHandlerContext, channel: Channel, buffer: ChannelBuffer): Packet = {
  20
+    if(buffer.readableBytes < Packet.headerSize)
  21
+      return null
  22
+
  23
+    val header = new Array[Byte](Packet.headerSize)
  24
+    buffer.readBytes(header)
  25
+    val br = new BufferReader(header)
  26
+
  27
+    val (length, seq) = (br.readInt24, br.readByte)
1
marius a. eriksen Collaborator

When tuples aren't natural, use sequence the statements instead. While correct, it isn't obvious to the reader that the order is br.readInt24, br.readByte).

Also, since these are side-effecting, they should be furnished with ()'s:

val length = br.readInt24()
val seq = br.readByte()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/PacketFrameDecoder.scala
((20 lines not shown))
  20
+    if(buffer.readableBytes < Packet.headerSize)
  21
+      return null
  22
+
  23
+    val header = new Array[Byte](Packet.headerSize)
  24
+    buffer.readBytes(header)
  25
+    val br = new BufferReader(header)
  26
+
  27
+    val (length, seq) = (br.readInt24, br.readByte)
  28
+
  29
+    if(buffer.readableBytes < length)
  30
+      return null
  31
+
  32
+    println("<- Decoding MySQL packet (length=%d, seq=%d)".format(length, seq))
  33
+    val body = new Array[Byte](length)
  34
+    buffer.readBytes(body)
  35
+    BufferUtil.hex(body)
1
marius a. eriksen Collaborator

spurious statement?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/RequestEncoder.scala
((2 lines not shown))
  2
+
  3
+import com.twitter.finagle.mysql.protocol._
  4
+import com.twitter.finagle.mysql.util.BufferUtil
  5
+import org.jboss.netty.buffer.{ChannelBuffers, ChannelBuffer}
  6
+import org.jboss.netty.channel.{Channel, ChannelHandlerContext, MessageEvent, Channels, ChannelEvent}
  7
+import org.jboss.netty.handler.codec.oneone.OneToOneEncoder
  8
+
  9
+object RequestEncoder extends OneToOneEncoder {
  10
+  override def encode(context: ChannelHandlerContext, channel: Channel, message: AnyRef) = {
  11
+    message match {
  12
+      case req: SimpleRequest if req.cmd == Command.COM_NOOP_GREET => 
  13
+        ChannelBuffers.EMPTY_BUFFER
  14
+      case req: Request =>
  15
+        println("-> Encoding " + req)
  16
+        val bytes = req.toByteArray
  17
+        BufferUtil.hex(bytes)
1
marius a. eriksen Collaborator

spurious?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/RequestEncoder.scala
((1 lines not shown))
  1
+package com.twitter.finagle.mysql.codec
  2
+
  3
+import com.twitter.finagle.mysql.protocol._
  4
+import com.twitter.finagle.mysql.util.BufferUtil
  5
+import org.jboss.netty.buffer.{ChannelBuffers, ChannelBuffer}
  6
+import org.jboss.netty.channel.{Channel, ChannelHandlerContext, MessageEvent, Channels, ChannelEvent}
  7
+import org.jboss.netty.handler.codec.oneone.OneToOneEncoder
  8
+
  9
+object RequestEncoder extends OneToOneEncoder {
  10
+  override def encode(context: ChannelHandlerContext, channel: Channel, message: AnyRef) = {
  11
+    message match {
  12
+      case req: SimpleRequest if req.cmd == Command.COM_NOOP_GREET => 
  13
+        ChannelBuffers.EMPTY_BUFFER
  14
+      case req: Request =>
  15
+        println("-> Encoding " + req)
  16
+        val bytes = req.toByteArray
1
marius a. eriksen Collaborator

define toChannelBuffer instead, which would allow the implementation to avoid extra copies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/ResultDecoder.scala
... ...
@@ -0,0 +1,168 @@
  1
+package com.twitter.finagle.mysql.codec
  2
+
  3
+import com.twitter.finagle.mysql.ClientError
  4
+import com.twitter.finagle.mysql.protocol._
  5
+import com.twitter.logging.Logger
  6
+import org.jboss.netty.buffer.ChannelBuffer
  7
+import org.jboss.netty.channel._
  8
+
  9
+sealed abstract class State
1
marius a. eriksen Collaborator

idiomatic to use traits: sealed trait State

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Request.scala
((133 lines not shown))
  133
+
  134
+    convert(parameters, Array[Byte]())
  135
+  }
  136
+
  137
+  override val data: Array[Byte] = {
  138
+    val cmdByte = Array(Command.COM_STMT_EXECUTE)
  139
+    val bw = new BufferWriter(new Array[Byte](9))
  140
+    bw.writeInt(ps.statementId)
  141
+    bw.writeByte(flags)
  142
+    bw.writeInt(iterationCount)
  143
+    
  144
+    val paramsList = ps.parameters.toList
  145
+    val nullBytes = makeNullBitmap(paramsList, 0, BigInt(0))
  146
+    val newParamsBound: Byte = if(ps.hasNewParameters) 1 else 0
  147
+
  148
+    val result = Array.concat(cmdByte, bw.buffer, nullBytes, Array(newParamsBound))
1
Sam Pullara
spullara added a note

More copies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Result.scala
((31 lines not shown))
  31
+  }
  32
+}
  33
+
  34
+/**
  35
+ * Represents the Error Packet received from the server
  36
+ * and the data sent along with it.
  37
+ */
  38
+case class Error(code: Short, sqlState: String, message: String) extends Result
  39
+
  40
+object Error {
  41
+  def decode(packet: Packet) = {
  42
+    //start reading after flag byte
  43
+    val br = new BufferReader(packet.body, 1)
  44
+    val code = br.readShort
  45
+    val state = new String(br.take(6))
  46
+    val msg = new String(br.takeRest)
5
Sam Pullara
spullara added a note

I'm not sure if we can assume that error codes and messages are in ASCII.

Davi Arnaut
darnaut added a note

It uses the same character set as query results (character_set_results).

Ruben Oanta Collaborator
roanta added a note

Is this also the same as server_language sent during handshaking?

Davi Arnaut
darnaut added a note

The variable is initialized to the charset number sent by the client.

See Connection Character Sets and Collations and Character Set for Error Messages for more details.

Davi Arnaut
darnaut added a note

As suggested above, better to enforce UTF-8 both ways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/ResultDecoder.scala
((24 lines not shown))
  24
+ * There are specific packets received from MySQL that can
  25
+ * be easily decoded based on their first byte. However, more complex 
  26
+ * results need to be defragged as they arrive in the pipeline.
  27
+ * To accomplish this, this handler needs to contain some state.
  28
+ * 
  29
+ * Some of state is synchronized because it is shared between handleDownstream
  30
+ * and handleUpstream events which are usually executed
  31
+ * on separate threads.
  32
+ */
  33
+class ResultDecoder extends SimpleChannelHandler {
  34
+  private val log = Logger("finagle-mysql")
  35
+  private var state: State = WaitingForGreeting
  36
+  @volatile 
  37
+  private var expectPrepareOK: Boolean = false
  38
+  @volatile 
  39
+  private var expectBinaryResults: Boolean = false
1
marius a. eriksen Collaborator

volatiles on same line: @volatile private[this] var ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/ResultDecoder.scala
((62 lines not shown))
  62
+  }
  63
+
  64
+  override def writeRequested(ctx: ChannelHandlerContext, evt: MessageEvent): Unit = {
  65
+    if(!evt.getMessage.isInstanceOf[ChannelBuffer]) {
  66
+      ctx.sendDownstream(evt)
  67
+      return
  68
+    }
  69
+
  70
+    val buffer = evt.getMessage.asInstanceOf[ChannelBuffer]
  71
+    if(buffer.capacity < 5) {
  72
+      ctx.sendDownstream(evt)
  73
+      return
  74
+    }
  75
+
  76
+    //Do we need to block requests over the same
  77
+    //pipeline when we are defragging a result?
1
marius a. eriksen Collaborator

whitespace

// Do ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/ResultDecoder.scala
((56 lines not shown))
  56
+        result map { Channels.fireMessageReceived(ctx, _) }
  57
+
  58
+      case unknown =>
  59
+        Channels.disconnect(ctx.getChannel)
  60
+        log.error("ResultDecoder: Expected packet and received: " + unknown)
  61
+    }
  62
+  }
  63
+
  64
+  override def writeRequested(ctx: ChannelHandlerContext, evt: MessageEvent): Unit = {
  65
+    if(!evt.getMessage.isInstanceOf[ChannelBuffer]) {
  66
+      ctx.sendDownstream(evt)
  67
+      return
  68
+    }
  69
+
  70
+    val buffer = evt.getMessage.asInstanceOf[ChannelBuffer]
  71
+    if(buffer.capacity < 5) {
1
marius a. eriksen Collaborator

this is mysterious to the reader: comment on this behavior. also you probably did mean: buffer.readableBytes? are there valid packets that are fewer than 5 bytes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/util/Query.scala
((41 lines not shown))
  41
+  def flatten(params: Seq[Any]): Seq[Any] = params flatMap {
  42
+    case p: Product => flatten(p.productIterator.toSeq)
  43
+    case i: Iterable[_] => flatten(i.toSeq)
  44
+    case x => Seq(x)
  45
+  }
  46
+
  47
+  /**
  48
+   * Replace each wildcard instance in sql with the output from
  49
+   * running f(param(i))
  50
+   */
  51
+  private def replace(sql: String, params: Seq[Any], f: Any => String): String = {
  52
+    if(params.isEmpty) {
  53
+      return sql
  54
+    }
  55
+
  56
+    val matcher = Query.wildcard.matcher(sql)
10
Sam Pullara
spullara added a note

Does this regular expression work if I have ? within quotes? I also don't know why we are doing the replacement rather than leaving it to the server? This seems pretty dangerous.

Ruben Oanta Collaborator
roanta added a note

For prepared queries, the '?' are just expanded and the replacement happens on the server. This allows for calling prepare("select * from table where id in (?)", 1 to 10) as opposed to prepare("select * from table where id in (?,?,?,?...", 1 to 10)

For regular queries, the parameters have to be replaced. This also allows for cleaner client apis that don't require StringBuilders and String concatenation to build a sql statement.

Sam Pullara
spullara added a note

I would like this to not be in the API at all. It is very difficult to get this right such that you don't introduce SQL injection attacks like the current implementation allows.

Ruben Oanta Collaborator
roanta added a note

As in, a rogue library that has a malicious toString method?

We can completely remove it but the problem of sql injection is still there if the a user builds their own sql statement.

Sam Pullara
spullara added a note
Ruben Oanta Collaborator
roanta added a note

That makes sense. I agree it would need more work to be more robust and safe. But if you don't think it is worth having we can scrap it and let the user explicitly build the queries.

marius a. eriksen Collaborator

I agree with Sam -- let's not include this in the API. Let's make it pure SQL. There are many other approaches to building queries, including DSLs that exploit type safety -- but those can and should be built on top of finagle-mysql

Jeremy Cole
jeremycole added a note

Not totally sure I'm following the above conversation, but if we're to use this in production it will need to support some form of prepared statement substitution. Server-side prepared statements are slower for many use cases and much more difficult to resource-manage. It's quite easy to accidentally run the server out of prepared statement handles and then everybody loses. I generally recommend with MySQL Connector/J JDBC driver to always set useServerPrepStmts=false, causing the client-side to always do the substitution.

Sam Pullara
spullara added a note

It sounds to me then that we need a well tested way of doing prepared statement encoding client side. In order to support this, the MySQL driver appears to have a SQL parser along with all the escaping rules once the context of the ? is well understood through parsing.

http://www.docjar.com/html/api/com/mysql/jdbc/PreparedStatement.java.html

@jeremycole The other possibility is more carefully monitoring server side prepared statement usage. Which one do you think is more doable?

Marius' idea of using a DSL to generate them might also serve the same purpose without having to parse statements but you would still need to do proper escaping of values.

Jeremy Cole
jeremycole added a note

In general I would say to avoid using server-side prepared statements at all. Even if they are perfectly monitored, they are still in very many cases generating 3 round trips (prepare, execute, close) when 1 would suffice (query). In most of our apps that we care about, there would be huge numbers of prepared statements in order to pre-prepare them all (as table name is not an allowed placeholder, and they are mostly sharded into N tables); so the round trip multiplication can't be avoided by pre-preparing all possible statements.

So since a good client-side prepared statement handler is necessary, may as well use it everywhere, even when it's not strictly needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/ResultDecoder.scala
((4 lines not shown))
  4
+import com.twitter.finagle.mysql.protocol._
  5
+import com.twitter.logging.Logger
  6
+import org.jboss.netty.buffer.ChannelBuffer
  7
+import org.jboss.netty.channel._
  8
+
  9
+sealed abstract class State
  10
+case object Idle extends State
  11
+case object WaitingForGreeting extends State
  12
+case class PacketDefragger(
  13
+    firstPacket: Option[Packet] = None,
  14
+    setOne: Option[List[Packet]] = None,
  15
+    setTwo: Option[List[Packet]] = None,
  16
+    setOneExpected: Boolean = true,
  17
+    setTwoExpected: Boolean = true,
  18
+    decoder: (Packet, List[Packet], List[Packet]) => Result
  19
+  ) extends State
3
marius a. eriksen Collaborator

consistent naming (what is the decoder doing?): DefraggingPackets

marius a. eriksen Collaborator

I think this would be clearer if it were split into more states. Right now there seems to be a lot of mutually exclusive configurations of PacketDefragger. Here's a suggestion based on my very primitive understanding of the protocol: depending on the type of the result set, there may be one or two set of packets that are to be decoded. First simplification: I think the Option in the sets are redundant. It's only None whenever there is no data, so use Nil instead to represent this state? Second it seems that you expect a number of packet sets; 0, 1, or 2, and that their order, but not their position is relevant (this understanding may not be accurate). So it seems that what you really want to arrive at is a Seq[Seq[Packet]]

case class Defragging(
  expected: Int,
  packets: Seq[Seq[Packet]]
)

Then your transitions look like

Defragging(2, Nil)

after EOF for the first set

Defragging(1, Seq(Seq(first, set, of, packets)))

...

does that make sense? My intuition is that this approach would simplify greatly.

Ruben Oanta Collaborator
roanta added a note

I've implemented your suggestions. Removing the noise caused by wrapping the sets in Option[...] and not passing around the decoder in the Defragging class cleaned up the code a lot.

The changes actually reduced the amount of cases by 1, but I think it did result in more understandable code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sam Pullara

I would like to see benchmarks against the normal MySQL driver as part of this effort. For performance, memory usage and CPU usage.

finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/ResultDecoder.scala
((74 lines not shown))
  74
+    }
  75
+
  76
+    //Do we need to block requests over the same
  77
+    //pipeline when we are defragging a result?
  78
+    if(state != Idle) {
  79
+      log.warning("Cannot process a writeRequest when ResultDecoder is not idle.")
  80
+      return
  81
+    }
  82
+
  83
+    //set flags that indicate expected results.
  84
+    val cmdByte = buffer.getByte(4)
  85
+    expectPrepareOK = (cmdByte == Command.COM_STMT_PREPARE)
  86
+    expectBinaryResults = (cmdByte == Command.COM_STMT_EXECUTE)
  87
+
  88
+    ctx.sendDownstream(evt)
  89
+  }
2
marius a. eriksen Collaborator

This suggests that the decoder is not separable from the encoder. Perhaps you should just conflate them in the code? It would avoid this extra decoding step and make the code clearer, I think.

Ruben Oanta Collaborator
roanta added a note

In some cases, they are not separable. I don't think it would be a problem to conflate them especially since the encoder is so simple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Buffer.scala
((24 lines not shown))
  24
+    val n = (offset until offset + width).zipWithIndex.foldLeft(0L) {
  25
+      case (result, (b,i)) => result | ((buffer(b) & 0xFFL) << (i*8))
  26
+    }
  27
+    offset += width
  28
+    n
  29
+  }
  30
+
  31
+  def readByte = read(1).toByte
  32
+  def readUnsignedByte = read(1).toShort
  33
+  def readShort = read(2).toShort
  34
+  def readUnsignedShort = read(2).toInt
  35
+  def readInt24 = read(3).toInt
  36
+  def readInt = read(4).toInt
  37
+  def readLong = read(8)
  38
+  def readFloat = JFloat.intBitsToFloat(readInt)
  39
+  def readDouble = JDouble.longBitsToDouble(readLong)
1
marius a. eriksen Collaborator

these should all have ()s since they modify state (offset)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Buffer.scala
... ...
@@ -0,0 +1,188 @@
  1
+package com.twitter.finagle.mysql.protocol
  2
+
  3
+import java.lang.{Float => JFloat, Double => JDouble}
  4
+import java.sql.{Time, Date, Timestamp}
  5
+
  6
+/**
  7
+ * Defines classes to read and write to/from a byte buffer 
  8
+ * in little endian byte order.
  9
+ */
  10
+
  11
+class BufferReader(val buffer: Array[Byte], private[this] var offset: Int = 0) {
2
marius a. eriksen Collaborator

(for future performance optimization) - we should try to use ChannelBuffers as much as possible throughout as it allows us to reduce the amount of copying we do.

Ruben Oanta Collaborator
roanta added a note

Agreed. I visited the idea of having this just wrap a netty ChannelBuffer, but I remember running into a lot of issues. It could have been my lack of experience of working with netty Buffers / nio Buffers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Capability.scala
((5 lines not shown))
  5
+  val foundRows        = 0x0002 /* Found instead of affected rows */
  6
+  val longFlag         = 0x0004 /* Get all column flags */
  7
+  val connectWithDB    = 0x0008 /* One can specify db on connect */
  8
+  val noSchema         = 0x0010 /* Don't allow database.table.column */
  9
+  val compress         = 0x0020 /* Can use compression protocol */
  10
+  val ODBC             = 0x0040 /* Odbc client */
  11
+  val localFiles       = 0x0080 /* Can use LOAD DATA LOCAL */
  12
+  val ignoreSpace      = 0x0100 /* Ignore spaces before '(' */
  13
+  val protocol41       = 0x0200 /* New 4.1 protocol */
  14
+  val interactive      = 0x0400 /* This is an interactive client */
  15
+  val ssl              = 0x0800 /* Switch to SSL after handshake */
  16
+  val ignoreSigPipe    = 0x1000 /* IGNORE sigpipes */
  17
+  val transactions     = 0x2000 /* Client knows about transactions */
  18
+  val secureConnection = 0x8000 /* New 4.1 authentication */
  19
+  val multiStatements  = 0x10000 /* Enable/disable multi-stmt support */
  20
+  val multiResults     = 0x20000 /* Enable/disable multi-results */
1
marius a. eriksen Collaborator

Constants should start with UpperCase -- this is actually not only a matter of style, but the pattern matching does not bind uppercase names, so it allows you to match against constants without further ado. And I also believe the optimizer only folds UppercaseConstants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Handshake.scala
((35 lines not shown))
  35
+      threadId,
  36
+      Array.concat(salt1, salt2),
  37
+      serverCap,
  38
+      language,
  39
+      status
  40
+    )
  41
+  }
  42
+}
  43
+
  44
+/**
  45
+ * Reply to ServerGreeting sent during handshaking phase.
  46
+ */
  47
+case class LoginRequest(
  48
+  clientCap: Capability = Capability(0xA68F),
  49
+  maxPacket: Int = 0x10000000,
  50
+  charset: Byte = 33.toByte, // TODO case class
3
marius a. eriksen Collaborator

comment on what this constant is, or better yet, name it.

Jeremy Cole
jeremycole added a note

It's also worth noting that MySQL conflates character sets and collations at the protocol level, so 33 is "utf8_general_ci" collation, and you could see many other values here for utf8 character set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Packet.scala
... ...
@@ -0,0 +1,40 @@
  1
+package com.twitter.finagle.mysql.protocol
  2
+
  3
+/**
  4
+ * Represents a logical packet received from MySQL.
  5
+ * A MySQL packet consists of a header,
  6
+ * 3-bytes containing the size of the body and a 
  7
+ * 1 byte seq number, followed by the body.
  8
+ */
  9
+
  10
+object Packet {
  11
+  val headerSize = 0x04
  12
+  val okByte = 0x00.toByte
  13
+  val errorByte = 0xFF.toByte
  14
+  val eofByte = 0xFE.toByte
1
marius a. eriksen Collaborator

constants are Capitalized

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Packet.scala
((17 lines not shown))
  17
+    val size = packetSize
  18
+    val number = seq
  19
+    val body = new Array[Byte](size)
  20
+  }
  21
+
  22
+  def apply(packetSize: Int, seq: Byte, data: Array[Byte]) = new Packet {
  23
+    val size = packetSize
  24
+    val number = seq
  25
+    val body = data
  26
+  }
  27
+}
  28
+
  29
+trait Packet {
  30
+  val size: Int
  31
+  val number: Byte //used for sanity checks on server side
  32
+  val body: Array[Byte]
1
marius a. eriksen Collaborator

any reason to not use a case class?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/PreparedStatement.scala
... ...
@@ -0,0 +1,61 @@
  1
+package com.twitter.finagle.mysql.protocol
  2
+
  3
+import com.twitter.util.Promise
  4
+
  5
+trait PreparedStatement extends Result {
  6
+  val statementId: Int
  7
+  val numberOfParams: Int
  8
+  val statement: Promise[String] = new Promise[String]()
4
marius a. eriksen Collaborator

case class?

Ruben Oanta Collaborator
roanta added a note

I wanted to leave room for different implementations.

marius a. eriksen Collaborator

then we can make room when they come :)

Ruben Oanta Collaborator
roanta added a note

case class it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Result.scala
((9 lines not shown))
  9
+ * - COM_PING
  10
+ * - COM_QUERY (INSERT, UPDATE, or ALTER TABLE)
  11
+ * - COM_REFRESH
  12
+ * - COM_REGISTER_SLAVE
  13
+ */
  14
+case class OK(affectedRows: Long, 
  15
+              insertId: Long, 
  16
+              serverStatus: Int,
  17
+              warningCount: Int,
  18
+              message: String) extends Result
  19
+
  20
+object OK {
  21
+  def decode(packet: Packet) = {
  22
+    //start reading after flag byte
  23
+    val br = new BufferReader(packet.body, 1)
  24
+    new OK(
1
marius a. eriksen Collaborator

`new' not needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
marius a. eriksen
Collaborator

Some general comments from my review:

  • try to use ChannelBuffers throughout as they make it easier to optimize away allocations &c.; especially for down the road
  • make sure you test the frame decoder for fragmented packets
  • it would be great to have package docs that give a brief overview of the protocol, the structure of the code, and links to protocol documentation
  • apply whitespace consistently throughout: if (, // a comment

Not for now, but possibly a simplification in the future: it would also be possible to use a Dispatcher to do packet defragmentation. In some sense, it's a more natural abstraction: the pipeline codec produces and consumes Packets, and is stateless. The dispatcher consumes and produces requests/responses, and is stateful. Besides a cleaner separation of responsibilities, you also have the full power of composable futures at hand, which makes it possible to write imperative-looking code. You could do stuff like:

readPacket() flatMap { packet =>
  if (fragmented(packet))
    defrag(packet)
  else
    decode(packet)
}

etc.

finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Handshake.scala
((33 lines not shown))
  33
+      protocol,
  34
+      version,
  35
+      threadId,
  36
+      Array.concat(salt1, salt2),
  37
+      serverCap,
  38
+      language,
  39
+      status
  40
+    )
  41
+  }
  42
+}
  43
+
  44
+/**
  45
+ * Reply to ServerGreeting sent during handshaking phase.
  46
+ */
  47
+case class LoginRequest(
  48
+  clientCap: Capability = Capability(0xA68F),
1
Davi Arnaut
darnaut added a note

Would be better to refer to capabilities by name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/Client.scala
((30 lines not shown))
  30
+      .hostConnectionLimit(1)
  31
+      .buildFactory()
  32
+
  33
+      apply(factory)
  34
+  }
  35
+
  36
+  def apply(host: String, username: String, password: String): Client = {
  37
+    apply(host, username, password, None)
  38
+  }
  39
+
  40
+  def apply(host: String, username: String, password: String, dbname: String): Client = {
  41
+    apply(host, username, password, Some(dbname))
  42
+  }
  43
+
  44
+  class Client(factory: ServiceFactory[Request, Result]) {
  45
+    private lazy val fService = factory.apply()
2
Steve Gury Collaborator
stevegury added a note

private[this] val fService = factory.apply()
2 comments:

  • use private[this] by default
  • The lazy feature here is not very useful
Ruben Oanta Collaborator
roanta added a note

Why is it so important to make private members private to the instance?

Also, my reasoning for the lazy val here was allowing the creating of a client without doing the connection leg work until the user actually used the client. A replacement for having an explicit 'connect()' method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Steve Gury stevegury commented on the diff
project/Build.scala
@@ -235,6 +235,17 @@ object Finagle extends Build {
235 235
     sourceDirectory <<= baseDirectory(_/"src29"),
236 236
     libraryDependencies ++= Seq("junit" % "junit" % "4.8.1" % "test", util("hashing"))
237 237
   ).dependsOn(finagleCore)
  238
+
  239
+  lazy val finagleMySQL = Project(
  240
+    id = "finagle-mysql",
  241
+    base = file("finagle-mysql"),
  242
+    settings = Project.defaultSettings ++
  243
+      StandardProject.newSettings ++
  244
+      sharedSettings
  245
+    ).settings(
  246
+      name := "finagle-mysql",
  247
+      libraryDependencies ++= Seq(util("logging"))
  248
+    ).dependsOn(finagleCore)
1
Steve Gury Collaborator
stevegury added a note

You should also create/update the pom.xml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/ResultSet.scala
((107 lines not shown))
  107
+  val br = new BufferReader(row)
  108
+  val values: IndexedSeq[String] = (0 until indexMap.size) map { _ => br.readLengthCodedString }
  109
+
  110
+  def findColumnIndex(name: String) = indexMap.get(name)
  111
+
  112
+  /**
  113
+   * The readLengthCodedString method returns an empty string when
  114
+   * a null value is returned from the database. This method handles the
  115
+   * empty string and returns an Option[String] to avoid attempting to cast an
  116
+   * empty string. 
  117
+   *
  118
+   * This also has a nice side-effect of translating null values
  119
+   * into Option values.
  120
+   */
  121
+  private def getValue(index: Int): Option[String] = 
  122
+    if(values(index) == "") None else Some(values(index))
1
Jeremy Cole
jeremycole added a note

Do I understand correctly that this conflates NULL and empty string by always translating empty string to None? If a legitimate empty string were returned would it result in None and thus be indistinguishable from NULL? That would not be a good thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Buffer.scala
((48 lines not shown))
  48
+
  49
+  /**
  50
+  * Read MySQL data field - a variable length encoded binary.
  51
+  * Depending on the first byte, read a different width from
  52
+  * the data array.
  53
+  */
  54
+  def readLengthCodedBinary: Long = {
  55
+    val firstByte = readByte
  56
+    if(firstByte < 251)
  57
+      firstByte
  58
+    else
  59
+      firstByte match {
  60
+        case 252 => read(2)
  61
+        case 253 => read(3)
  62
+        case 254 => read(8)
  63
+        case _ => -1 //NULL
1
Jeremy Cole
jeremycole added a note

This is not really correct. I would think it better to explicitly case 251 as SQL NULL, and throw an exception for 255, as it implies corrupted data or out of sync client/server, since it's not a valid value. Continuing your read after getting a 255 here would end up with an exception in some other unrelated place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Buffer.scala
((65 lines not shown))
  65
+  }
  66
+
  67
+  def readNullTerminatedString: String = {
  68
+    val result = new StringBuilder()
  69
+    while(buffer(offset) != 0)
  70
+      result += readByte.toChar
  71
+
  72
+    readByte //consume null byte
  73
+    result.toString
  74
+  }
  75
+
  76
+  def readLengthCodedString: String = {
  77
+    val size = readUnsignedByte
  78
+
  79
+    if(size == 0xFB) 
  80
+      return "" // NULL string.
1
Jeremy Cole
jeremycole added a note

Blank string and SQL NULL should never be conflated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Buffer.scala
((61 lines not shown))
  61
+        case 253 => read(3)
  62
+        case 254 => read(8)
  63
+        case _ => -1 //NULL
  64
+      }
  65
+  }
  66
+
  67
+  def readNullTerminatedString: String = {
  68
+    val result = new StringBuilder()
  69
+    while(buffer(offset) != 0)
  70
+      result += readByte.toChar
  71
+
  72
+    readByte //consume null byte
  73
+    result.toString
  74
+  }
  75
+
  76
+  def readLengthCodedString: String = {
3
Jeremy Cole
jeremycole added a note

This seems to not actually implement the length-coded string. What am I missing?

Ruben Oanta Collaborator
roanta added a note

I read the spec wrong. This of course managed to work because I haven't dealt with string lengths that don't fit into a unsigned byte. I'll fix it to use the length coded binary.

Ruben Oanta Collaborator
roanta added a note

Is there an upper bound on the length of a string? Can I assume that the size will never need to be represented with 8 bytes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Buffer.scala
((83 lines not shown))
  83
+    Array.copy(buffer, offset, strBytes, 0, size)
  84
+    offset += size
  85
+    new String(strBytes)
  86
+  }
  87
+
  88
+  /**
  89
+   * Read a MySQL binary encoded Timestamp from the buffer.
  90
+   */
  91
+  def readTimestamp: Timestamp = {
  92
+    val len = readUnsignedByte
  93
+    if(len == 0)
  94
+      return new Timestamp(0)
  95
+
  96
+    var year, month, day, hour, min, sec, nano = 0
  97
+
  98
+    if(readable(4)) {
2
Jeremy Cole
jeremycole added a note

If it is not readable, then what? Certainly this needs an else and should probably throw an exception of some sort.

Ruben Oanta Collaborator
roanta added a note

I need to find a better way to represent corrupt data. From what I understand, it isn't idiomatic to throw exceptions that are not future encoded within Finagle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/ResultSet.scala
... ...
@@ -0,0 +1,301 @@
  1
+package com.twitter.finagle.mysql.protocol
  2
+
  3
+import com.twitter.logging.Logger
  4
+import scala.math.BigInt
  5
+import java.sql.{Timestamp, Date, Time}
  6
+
  7
+/**
  8
+ * ResultSets are returned from the server for any
  9
+ * query except for INSERT, UPDATE, or ALTER TABLE.
1
Jeremy Cole
jeremycole added a note

There are a lot of other SQL queries which don't return result sets. To avoid confusion it's probably better not to try to list them here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Types.scala
((29 lines not shown))
  29
+  val STRING      = 0xfe;
  30
+  val GEOMETRY    = 0xff;
  31
+}
  32
+
  33
+sealed case class NullValue(typeCode: Int)
  34
+object NullValues {
  35
+  import Types._
  36
+
  37
+  val legalValues = Set(DECIMAL, TINY, SHORT, LONG, FLOAT, DOUBLE, NULL, TIMESTAMP, LONGLONG,
  38
+                        INT24, DATE, DATETIME, YEAR, NEWDATE, VARCHAR, BIT, NEWDECIMAL, ENUM,
  39
+                        SET, TINY_BLOB, MEDIUM_BLOB, LONG_BLOB, BLOB, VAR_STRING, STRING, GEOMETRY)
  40
+
  41
+  val NullString = NullValues(VARCHAR)
  42
+  val NullInt = NullValues(LONG)
  43
+  val NullDouble = NullValues(DOUBLE)
  44
+  val NullBoolean = NullValues(BIT)
1
Jeremy Cole
jeremycole added a note

In terms of naming, this is not really correct. Type BIT represents a bitmap of 1 or more bits. It's really an integer type of varying length, and only in the minimal case of BIT/BIT(1) is it potentially a boolean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jeremy Cole jeremycole commented on the diff
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/util/Query.scala
... ...
@@ -0,0 +1,75 @@
  1
+package com.twitter.finagle.mysql.util
  2
+
  3
+import java.util.regex.Pattern
  4
+
  5
+class TooFewQueryParametersException(t: Throwable) extends Exception(t)
  6
+class TooManyQueryParametersException(t: Throwable) extends Exception(t)
  7
+
  8
+object Query {
  9
+  val wildcard = Pattern.compile("\\?")
1
Jeremy Cole
jeremycole added a note

This is, of course, far too simple. I would argue that regular expression substitution is itself too complex to be part of this library though, and should be delegated to something else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
added some commits
Ruben Oanta Includes most of the changes suggested during the code review. c6f7c9e
Ruben Oanta fixed multi-line comment spacing. d8c27ed
Ruben Oanta Fixed the README to use markdown + some comment changes. 65c53d4
Ruben Oanta Ensured client/server are using UTF8 b59501c
Ruben Oanta added .md extension to README 9aff55c
Ruben Oanta - Added the ability to read length coded bytes from the Buffer
- Other small changes and fixes.
c05ab0d
Ruben Oanta - Changed the Row interface to use a Value ADT. This adds type safety…
… to results and more explicitly expresses undecoded/raw values.
632f301
Ruben Oanta Fixed small mistakes in readLengthCodedBytes and readLengthCodedString cd5fd4d
Ruben Oanta - Changed the BufferReader and BufferWriter classes to interfaces to …
…accommodate easily changing the implementation when profiling performance.

- Made some general memory optimizations to the BufferReader class.
- Removed complex decoding of types from the BufferReader/BufferWriter implementations. It seemed more correct to decode/encode only primitives on Buffers.
- Moved decoding more complex types onto the Types object.
- Other small changes and fixes.
5db3c76
Ruben Oanta Changed tabs to spaces. bdac4aa
Ruben Oanta Avoid extra copying in readLengthCodedString 2eac63b
Ruben Oanta Updates to the README, Row interface, and Example. e5f0880
Ruben Oanta - Changed default implementations of BufferReader and BufferWriter to…
… wrap Netty ChannelBuffers.

- Updated references in unit tests.
- Added reference to defaultCharset in LoginRequest.encryptPassword.
70774b9
Ruben Oanta Cleaned up Buffer.scala 9943d26
Ruben Oanta - new unit tests + an integration test
- moved codec entry point code into seperate methods to allow for easier testing without needing to mock netty objects.
- added a new example that shows a table create / insert / query.
- Cleaned up Buffer.scala
- Added a ping method to client that runs a COM_PING request on the server.
- Removed deprecated requests for COM_CREATE_DB and COM_DROP_DB
- Fixed bugs with Date / Datetime / Timestamp offsets.
- Fixed a bug with executing a prepared statement that had bound parameters.
- Other small fixes and changes.
a229f5b
Ruben Oanta tabs -> spaces aaa5613
Ruben Oanta - Added a synthetic response for CloseRequest which doesn't get a res…
…ponse from the server.

- Better handling for unsupported LONG_BLOBs
- Closed prepared statement in Example.scala
- style fixes and comment fixes.
935f86e
Ruben Oanta Fixed 8 byte length coded binary test to reflect current implementation. 8dae5a2
Ruben Oanta Removed onSuccess from insertResults. 6758be5
marius a. eriksen mariusaeriksen commented on the diff
finagle-mysql/pom.xml
... ...
@@ -0,0 +1,43 @@
  1
+<?xml version="1.0"?>
  2
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
  3
+  <modelVersion>4.0.0</modelVersion>
  4
+  <groupId>com.twitter</groupId>
  5
+  <artifactId>finagle-mysql</artifactId>
  6
+  <packaging>jar</packaging>
  7
+  <version>4.0.3-SNAPSHOT</version>
1
marius a. eriksen Collaborator

version needs update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
marius a. eriksen mariusaeriksen commented on the diff
finagle-mysql/pom.xml
((14 lines not shown))
  14
+  <properties>
  15
+    <git.dir>${project.basedir}/../../.git</git.dir>
  16
+  </properties>
  17
+  <dependencies>
  18
+    <!-- library dependencies -->
  19
+    <!-- project dependencies -->
  20
+    <dependency>
  21
+      <groupId>com.twitter</groupId>
  22
+      <artifactId>finagle-core</artifactId>
  23
+      <version>4.0.3-SNAPSHOT</version>
  24
+    </dependency>
  25
+    <dependency>
  26
+      <groupId>com.twitter</groupId>
  27
+      <artifactId>util-logging</artifactId>
  28
+      <version>4.0.1</version>
  29
+    </dependency>
1
marius a. eriksen Collaborator

ditto for these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
marius a. eriksen mariusaeriksen commented on the diff
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/Client.scala
((61 lines not shown))
  61
+   * in the ResultSet, call f on the row and return the results.
  62
+   * @param sql A sql statement that returns a result set.
  63
+   * @param f A function from ResultSet to any type T.
  64
+   * @return a Future of Seq[T]
  65
+   */
  66
+  def select[T](sql: String)(f: Row => T): Future[Seq[T]] = query(sql) map {
  67
+    case rs: ResultSet => rs.rows.map(f)
  68
+    case ok: OK => Seq()
  69
+  }
  70
+  
  71
+  /**
  72
+   * Sends a query to server to be prepared for execution.
  73
+   * @param sql A query to be prepared on the server.
  74
+   * @return PreparedStatement 
  75
+   */
  76
+  def prepare(sql: String, params: Any*) = {
1
marius a. eriksen Collaborator

explicitly annotate return type here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
marius a. eriksen mariusaeriksen commented on the diff
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/Client.scala
((154 lines not shown))
  154
+   * Close the ServiceFactory and its underlying resources.
  155
+   */
  156
+  def close() = factory.close()
  157
+
  158
+  /**
  159
+   * Helper function to send requests to the ServiceFactory
  160
+   * and handle Error responses from the server.
  161
+   */
  162
+  private[this] def send[T](r: Request)(handler: PartialFunction[Result, Future[T]])  = 
  163
+    fService flatMap { service =>
  164
+      service(r) flatMap (handler orElse {
  165
+        case Error(c, s, m) => Future.exception(ServerError(c + " - " + m))
  166
+        case result         => Future.exception(ClientError("Unhandled result from server: " + result))
  167
+      })
  168
+    }
  169
+}
1
marius a. eriksen Collaborator

for all public interfaces here: include return types. it serves both as (in code) documentation and it also fixes the types, which is important; otherwise refinement types may be exposed unintentionally, causing API conflicts later:

http://twitter.github.com/effectivescala/#Types%20and%20Generics-Return%20type%20annotations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
marius a. eriksen mariusaeriksen commented on the diff
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/Example.scala
((126 lines not shown))
  126
+    preparedFuture.isReturn && insertResults.forall(_.isReturn)
  127
+  }
  128
+
  129
+  def parseArgs(parsed: Map[String, Any], args: List[String]): Map[String, Any] = args match {
  130
+    case Nil => parsed
  131
+    case "-host" :: value :: tail =>
  132
+      parseArgs(parsed + ("host" -> value), tail) 
  133
+    case "-port" :: value :: tail => 
  134
+      parseArgs(parsed + ("port" -> value.toInt), tail)
  135
+    case "-u" :: value :: tail => 
  136
+      parseArgs(parsed + ("username" -> value), tail)
  137
+    case "-p" :: value :: tail => 
  138
+      parseArgs(parsed + ("password" -> value), tail)
  139
+    case unknown :: tail => parsed
  140
+  }
  141
+}
1
marius a. eriksen Collaborator

love this. we'll move it under finagle-example when it's ready for prime time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
marius a. eriksen mariusaeriksen commented on the diff
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/codec/Endec.scala
((162 lines not shown))
  162
+      None
  163
+
  164
+    // second set complete - no sets can follow.
  165
+    case (Defragging(2, Seq(header, xs, ys)), Packet.EofByte) =>
  166
+      transition(Idle)
  167
+      Some(defragDecoder(header(0), xs.reverse, ys.reverse))
  168
+
  169
+    // prepend onto second set
  170
+    case (Defragging(2, Seq(header, xs, ys)), _) =>
  171
+      transition(Defragging(2, Seq(header, xs, packet +: ys)))
  172
+      None
  173
+
  174
+    case _ =>
  175
+        throw new ClientError("Endec: Unexpected state when defragmenting packets.")
  176
+  }
  177
+}
1
marius a. eriksen Collaborator

very nice. FOR LATER: i think we could improve this a bit further by embedding expectOK and defragDecoder into the state as well; but let’s leave it as-is for now. (I find it much easier to understand than the previous version)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
marius a. eriksen mariusaeriksen commented on the diff
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/Buffer.scala
((2 lines not shown))
  2
+
  3
+import com.twitter.finagle.mysql.ClientError
  4
+import org.jboss.netty.buffer.ChannelBuffer
  5
+import org.jboss.netty.buffer.ChannelBuffers._
  6
+import java.nio.charset.{Charset => JCharset}
  7
+import java.nio.ByteOrder
  8
+
  9
+/**
  10
+ * The BufferReader and BufferWriter interfaces provide methods for 
  11
+ * reading/writing primitive data types exchanged between the client/server. 
  12
+ * This includes all primitive numeric types and strings (null-terminated and length coded). 
  13
+ * All Buffer methods are side-effecting. That is, each call to a read* or write* 
  14
+ * method will increase the current offset. 
  15
+ *
  16
+ * Both BufferReader and BufferWriter assume bytes are written
  17
+ * in little endian. This conforms with the MySQL protocol.
2
marius a. eriksen Collaborator

The reader will be curious as to why you wouldn’t just use a ChannelBuffer (why didn’t you?)

Ruben Oanta Collaborator
roanta added a note

There are several reason why I thought it would be better to wrap ChannelBuffer and provide a separate interface.

  1. Each time a user intends to read from a MySQL packet, a ChannelBuffer needs to be created with the correct ByteOrder. It just seemed more naturally to offer an interface specific to the codec that assures this.

  2. There are specific methods that a ChannelBuffer doesn't offer and are at the core of the protocol (readLengthCodedString/Bytes and writeLengthCodedString/Bytes). This could have easily been offered as a method in a an object ex. Buffer.readLengthCodedString(c: ChannelBuffer). Again, I thought it would be more naturally to provide this as part of an interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
marius a. eriksen mariusaeriksen commented on the diff
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/PreparedStatement.scala
... ...
@@ -0,0 +1,74 @@
  1
+package com.twitter.finagle.mysql.protocol
  2
+
  3
+import com.twitter.util.Promise
  4
+
  5
+case class PreparedStatement(statementId: Int, numberOfParams: Int) extends Result {
  6
+  val statement: Promise[String] = new Promise[String]()
  7
+  private[this] var params: Array[Any] = new Array[Any](numberOfParams)
  8
+  private[this] var hasNewParams: Boolean = false
  9
+
  10
+  def parameters: Array[Any] = params
  11
+  def hasNewParameters: Boolean = hasNewParams
  12
+  
  13
+  def bindParameters() = hasNewParams = false
1
marius a. eriksen Collaborator

for side effecting methods like this, convention dicatates the use of {}:

def bindParameters() { hasNewParams = false }

otherwise it’s difficult to discern whether you mean to do do the assignment, or if it’s a bug and you instead meant:

def bindParameters() = hasNewParams == false
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
marius a. eriksen mariusaeriksen commented on the diff
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/protocol/ResultSet.scala
((64 lines not shown))
  64
+   */
  65
+  def indexOf(columnName: String): Option[Int]
  66
+
  67
+  /**
  68
+   * Retrieves the Value in the column with the 
  69
+   * given name.
  70
+   * @param columnName name of the column.
  71
+   * @return Some(Value) if the column 
  72
+   * exists with the given name. Otherwise, None.
  73
+   */ 
  74
+  def valueOf(columnName: String): Option[Value] = 
  75
+    valueOf(indexOf(columnName))
  76
+
  77
+  protected def valueOf(columnIndex: Option[Int]): Option[Value] =
  78
+    for (idx <- columnIndex) yield values(idx)
  79
+}
1
marius a. eriksen Collaborator

it might make sense to name "valueOf" -> "apply".

by convention in scala, maps use that for their keys, then you can access columns thus:

val row: Row
row("theColumn")
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
marius a. eriksen mariusaeriksen commented on the diff
finagle-mysql/src/main/scala/com/twitter/finagle/mysql/util/Query.scala
((37 lines not shown))
  37
+    while (matcher.find) {
  38
+      try {
  39
+        matcher.appendReplacement(result, expand(params(i)))
  40
+      } catch {
  41
+        case e: ArrayIndexOutOfBoundsException =>
  42
+          throw new TooFewQueryParametersException(e)
  43
+        case e: NoSuchElementException =>
  44
+          throw new TooFewQueryParametersException(e)
  45
+      }
  46
+      i += 1
  47
+    }
  48
+
  49
+    matcher.appendTail(result)
  50
+    result.toString
  51
+  }
  52
+}
1
marius a. eriksen Collaborator
mariusaeriksen added a note