Simplify TCP/IPC wire protocol #29339
Conversation
As I was working on another project, I ran into some other project doing exactly this. Our current wire protocol is effectively "len-of-thing thing", but msgpack already includes headers in the serialization--making it an iterably consumable format. This means we don't need to use/waste bytes on the wire for the len of the string (esp. since its currently a string serialization of how many bytes). This change simplifies the wire protocol, but still maintains flexibility for changes later-- since we are effectively just msgpacking dicts across the wire. Note: this is a backwards incompatible wire protocol change. This is only being done as TCP is considered "experimental". This will be the last and only change to the wire protocol in a backwards incompatible fashion-- as we are working towards first-class support for the next release.
|
I'm also considering wrapping the unpacker so we can contain all the msgpack in frame.py -- it'll make changes in the future easier. |
jfindlay
added
Core
|
Very clean. This looks great to me. @jacobhammons You'll want to keep an eye on this. Even though our current TCP implementation is considered experimental, you'll want to heavily document that with the release of Boron that the wire protocol has changed and will not be backward compatible with prior releases. |
|
@jacksontj Please let me know if you want to wrap the unpacker in this PR or a forthcoming one. I'm fine either way. |
|
@cachedout after thinking about it some more I think I'll hold off on the msgpack cleanup. Tests are passing, so this should be ready for merge. Also, do we have any more outstanding TCP issues? IIRC the 3 outstanding ones from the 2015.8 release are all closed now? And were we able to get the tcp tests running in jenkins again? |
|
@jacksontj I believe there is an open issue for the TCP transport on FreeBSD, but that's the only one that comes to mind. I'll get this in today. I put TCP tests back online for Jenkins a while ago and they are currently passing on the 2015.8 branch. We need to get some going for develop as well. I'll ask QA to get started on that today. |
Simplify TCP/IPC wire protocol
|
Awesome! Looking forward to getting this finished up :) On Thu, Dec 3, 2015 at 8:59 AM, Mike Place notifications@github.com wrote:
|
|
is this change going to be ported to the salt/2015.8 branch? |
|
@adelcast I am not very inclined to. This is a backward incompatabile wire-protocol change. |
As I was working on another project, I ran into some other project doing exactly this. Our current wire protocol is effectively "len-of-thing thing", but msgpack already includes headers in the serialization--making it an iterably consumable format. This means we don't need to use/waste bytes on the wire for the len of the string (esp. since its currently a string serialization of how many bytes).
This change simplifies the wire protocol, but still maintains flexibility for changes later-- since we are effectively just msgpacking dicts across the wire.
Note: this is a backwards incompatible wire protocol change. This is only being done as TCP is considered "experimental". This will be the last and only change to the wire protocol in a backwards incompatible fashion-- as we are working towards first-class support for the next release.