(Work in progress) Support binary protocol

[Ngo-The-Trung]

An attempt at fixing https://github.com/facebook/mcrouter/issues/6

Foreword: I'm not good at C++ at all. All advices welcomed, especially advices on the approach - I'm not sure if this is the right way to do it at all (for instance, is using reinterpret_cast a proper way to parse network data?).

TODO:

• [ ] Early stop when encountering invalid opcode
• [ ] Early stop when command violates field constraints (like having an extras field when there shouldn't be any)
• [ ] Return binary responses
• [ ] Support 'quiet' and 'key' variations of the commands.
• [ ] Implements CAS field (not present in ascii protocol)

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[dormando]

nice. gave it a quick skim and it's certainly a start.

Hopefully there's a way to validate the magic value properly (ie; client request or response code paths)?

[Ngo-The-Trung]

@dormando took me some time to understand your last question 😅

The 0x80 || 0x81 check was a mistake, I wasn't reading the doc carefully and assumed that both are valid magic numbers for requests. I've changed it to check for 0x80 only. The McServerBinaryParser class only concerns with requests, not responses.

The binary response code isn't ready yet, it'll involve adding some more serialization code in a subsequent commit.

[Ngo-The-Trung]

@andreazevedo @dormando one issue I've encountered is that, by right, the binary protocol is supposed to handle variations of commands such as quiet response and return key on response.

So a few ways I've thought up is:

1. Implement these as boolean fields inside the request classes I'm not sure if this has any implication on other protocols. For instance, lib/network/gen/MemcacheMessages.cpp seems like it was generated with some RPC payload generator. Adding these boolean fields will cause them to show up in the RPC payload too, but it's unnecessary.

2. Implement them as separate Request classes (McGetRequest, McGetKRequest, etc) This requires some additional code for other protocols to handle these requests (which feels unnecessary, since these requests shouldn't be able to show up in those protocols).

3. Ignore the additional flags The return payloads may have redundant data (i.e. has a data field on a cache miss, or has a key field on a quiet request). If the memcached client library doesn't explicitly check for these cases (i.e. they just obey whatever length data we pass them in the header) it should still work fine.

My interest is to get mcrouter to work with the Ruby library dalli, and it seems like dalli doesn't really care if we return redundant data. Other libraries may not guarantee such things however.

I'd appreciate any advice on this subject. Meanwhile I'll just go ahead and stick to ignoring the additional flags.

[craig-day]

@Ngo-The-Trung are you still interested in working on this? How can I help?

[Ngo-The-Trung]

I'll find some time to work on this (a bit busy this weekend). Also will have to rebase this since it's been months since I last touch it. But yes, I do plan on completing this.

[craig-day]

@Ngo-The-Trung any updates on this. Any help desired?

[Ngo-The-Trung]

@craig-day I admit I have kind of forgotten about this; taking some time this week to work on this

[wsantos]

@Ngo-The-Trung were you able to work on this? :D

[maximebedard]

👋 I was able to pick this up a little bit before during the holidays, but I'm afraid I'm lacking time to work on this if anyone wants to pick it up. https://github.com/facebook/mcrouter/compare/master...maximebedard:binary-protocol?expand=1

My branch is still very dirty, but I fixed a few bugs and started implementing a python client to be able to swap between ascii/binary in the integration tests. Such client would allow to compare if there's actually feature parity between both protocols. What is still missing is parsing the binary replies from the upstreams, which is very time consuming 😒