implement binary protocol (minimum) #29
Conversation
|
Thank you for your pull request. |
app.go
Outdated
| scanner := bufio.NewScanner(conn) | ||
| bufReader := bufio.NewReader(conn) | ||
| isBin, err := app.IsBinaryProtocol(bufReader) | ||
| if isBin { |
There was a problem hiding this comment.
At first, err must be checked.
binary_protocol.go
Outdated
| value string | ||
| } | ||
|
|
||
| func newRequest(r io.Reader) (req request, err error) { |
There was a problem hiding this comment.
Usally, newXXX() returns a pointer value.
I recommend to early return if returning error as below.
func newRequest(r io.Reader) (*request, error) {
req := request{}
// ...
if err != nil {
return nil, err
}
// ...
return &req, nil
}
binary_protocol.go
Outdated
| opcodeVersion = 0x0b | ||
| ) | ||
|
|
||
| type request struct { |
There was a problem hiding this comment.
request sounds generally... katsubushi.request represents which of a value for binary or text?
I like binaryRequst, binRequest or bRequest.
binary_protocol.go
Outdated
| // BytesToCmd converts byte array to a MemdBCmd and returns it. | ||
| func (app *App) BytesToBinaryCmd(req request) (cmd MemdCmd, err error) { | ||
| switch req.opcode { | ||
| case opcodeGet: |
There was a problem hiding this comment.
atomic.AddInt64(&(app.cmdGet), 1) is required at here to count up stats cmdGet.
Line 281 in 60ec3f4
| } | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
app.extendDeadline(conn) is required at here.
Line 214 in 60ec3f4
There was a problem hiding this comment.
Is it better that place extendDeadline at the beginning of this loop?
I guess, it sets timeout for reading a binary request, too.
|
|
||
| // IsBinaryProtocol judges whether a protocol is binary or text | ||
| func (app *App) IsBinaryProtocol(r *bufio.Reader) (bool, error) { | ||
| firstByte, err := r.Peek(1) |
There was a problem hiding this comment.
check err before use firstByte.
I encountered a panic while running benchmark.
$ go test -bench "AppBinary" -benchmem
goos: darwin
goarch: amd64
pkg: github.com/kayac/go-katsubushi
BenchmarkAppBinary-4 panic: runtime error: index out of range
goroutine 209 [running]:
github.com/kayac/go-katsubushi.(*App).IsBinaryProtocol(0xc4201be000, 0xc4201221e0, 0x1000, 0xc420221000, 0x1000)
/Users/fujiwara/src/github.com/kayac/go-katsubushi/binary_protocol.go:193 +0x73
github.com/kayac/go-katsubushi.(*App).handleConn(0xc4201be000, 0x14bdc20, 0xc4200160d8, 0x14bf860, 0xc42000e040)
/Users/fujiwara/src/github.com/kayac/go-katsubushi/app.go:212 +0x1b3
created by github.com/kayac/go-katsubushi.(*App).Listen
/Users/fujiwara/src/github.com/kayac/go-katsubushi/app.go:188 +0x380
exit status 2
FAIL github.com/kayac/go-katsubushi 14.006s|
I've fixed codes for all review comments. |
binary_protocol.go
Outdated
| atomic.AddInt64(&(app.cmdGet), 1) | ||
| cmd = &MemdBCmdGet{ | ||
| Name: "GET", | ||
| Keys: strings.Fields(req.key), |
There was a problem hiding this comment.
Why do you split the key?
GET opecode accepts just only one key, doesn't it?
(or I misread the specification?)
There was a problem hiding this comment.
Thank you for reviewing!
Sorry, it's redundant as you said.
I'll remove it.
I also try to fix field name Keys to Key.
binary_protocol.go
Outdated
| keyLenBytes := make([]byte, 2) | ||
| binary.BigEndian.PutUint16(keyLenBytes, uint16(keyLen)) | ||
|
|
||
| extraLenByte := byte(uint8(extraLen)) |
There was a problem hiding this comment.
byte is an alias of uint8. No need to cast.
binary_protocol.go
Outdated
|
|
||
| bodyBuf := make([]byte, bodyLen) | ||
| n2, e2 := io.ReadFull(r, bodyBuf) | ||
| if uint32(n2) < bodyLen { |
There was a problem hiding this comment.
io.ReadFull returns an error in this case.
So, no need to check n2.
binary_protocol.go
Outdated
| data[22] = res.cas[6] | ||
| data[23] = res.cas[7] | ||
|
|
||
| for i := 0; i < extraLen; i++ { |
There was a problem hiding this comment.
I think ´copy(data[headerSize:], res.extras)´ is better.
purpose
We want to use katsubushi with Ruby products.
But its popular memcached client supports only binary protocol.
strategy
I've implemented only GET (single key) and VERSION which are called by Dalli.
Binary protocol implementation is separated from app.go.
Since text protocol is mainly maintained.
out of scope
etc
If you found implementation issues (naming, conding style, ...), I'll fix as I can as possible.
Goに不慣れなため、もし変な実装をしてしまっていたら細かなことでも対応いたします。