Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement binary protocol (minimum) #29

Merged
merged 9 commits into from
Jan 11, 2018

Conversation

xorphitus
Copy link
Contributor

@xorphitus xorphitus commented Jan 9, 2018

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

  • performance tuning for binary protocol
    • I want to try next pull request
  • implementation of other commands
  • implementation of multiple key GET

etc

If you found implementation issues (naming, conding style, ...), I'll fix as I can as possible.

Goに不慣れなため、もし変な実装をしてしまっていたら細かなことでも対応いたします。

@fujiwara
Copy link
Contributor

fujiwara commented Jan 9, 2018

Thank you for your pull request.
I'm studying about binary protocol. Please wait a few days for reviewing.

app.go Outdated
scanner := bufio.NewScanner(conn)
bufReader := bufio.NewReader(conn)
isBin, err := app.IsBinaryProtocol(bufReader)
if isBin {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first, err must be checked.

value string
}

func newRequest(r io.Reader) (req request, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

opcodeVersion = 0x0b
)

type request struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request sounds generally... katsubushi.request represents which of a value for binary or text?
I like binaryRequst, binRequest or bRequest.

// 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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

atomic.AddInt64(&(app.cmdGet), 1) is required at here to count up stats cmdGet.

atomic.AddInt64(&(app.cmdGet), 1)

}
return
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

app.extendDeadline(conn) is required at here.

app.extendDeadline(conn)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@xorphitus
Copy link
Contributor Author

I've fixed codes for all review comments.
Please confirm them.

atomic.AddInt64(&(app.cmdGet), 1)
cmd = &MemdBCmdGet{
Name: "GET",
Keys: strings.Fields(req.key),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you split the key?
GET opecode accepts just only one key, doesn't it?
(or I misread the specification?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

keyLenBytes := make([]byte, 2)
binary.BigEndian.PutUint16(keyLenBytes, uint16(keyLen))

extraLenByte := byte(uint8(extraLen))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

byte is an alias of uint8. No need to cast.


bodyBuf := make([]byte, bodyLen)
n2, e2 := io.ReadFull(r, bodyBuf)
if uint32(n2) < bodyLen {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

io.ReadFull returns an error in this case.
So, no need to check n2.

data[22] = res.cas[6]
data[23] = res.cas[7]

for i := 0; i < extraLen; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ´copy(data[headerSize:], res.extras)´ is better.

Copy link
Contributor

@fujiwara fujiwara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@fujiwara fujiwara merged commit 0485f9d into kayac:master Jan 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants