Add msgpack serialization support#1850
Conversation
|
CLA signed by all contributors. |
|
I think this is an excellent choice of encoding, and much saner than JSON for a transport layer intended to carry arbitrary binary data rather than just UTF-8 textual data. I can't comment on the code, but the theory is great. |
|
Should this also update the docs that we've started putting together for the rest endpoints were we state the supported formats? |
|
@dalen there are a bunch of commits from Patrick here; did you pair with him during the developer day on this? |
|
Yes, we paired, and I then rebased and squashed a bunch of commits together, so most of them are pair programmed really. |
|
Okay, cool. Most of the time when I see two contributors on a single PR it usually means there was a bad rebase, so I wanted to get this sorted out before I dove in. One big thing that jumps out at me is that this is backwards incompatible, since we're removing the Puppet::Provider::Confine = Puppet::ConfineI think this would alleviate this problem. What do you think? |
|
That sounds good. We were under the impression that 4.0 was the next release though, so a small break might not be a blocker :) |
|
@adrienthebo do you want me to add that stub of the old constant to this pull request? |
|
Actually, does the new The thing that jumps out at me is that we're doing three things in this pull request - extracting Confine, generalizing |
|
Without moving the confine we got into a require loop with format_support and provider. Partly because we include format_support in more places to get the to_msgpack method. |
|
Oh. Ew. Fair point. Well, what do you think of splitting up this pull request so we can review each change separately? Or would you prefer to keep this as-is? |
|
well, they are kind of depending on each other. so I'm not sure how to do that with multiple pull requests in github actually without waiting for the first to be merged before opening the next pull request etc. |
|
@dalen it looks like this needs a rebase; in addition could you address the concerns about the constant |
|
It looks like this pull request hasn't seen any activity in about three weeks. Is this something that you're still able to work on? We're happy to keep this pull request open if there's forward movement, and we'll leave it open for another week. If there isn't any activity by then we may have to close this pull request due to lack of activity. Closing the pull request wouldn't mean we don't consider this change valuable. However we try to keep all open pull requests moving towards completion, and closing stale pull requests help us focus on more active pull requests. If you have any questions or concerns, please don't hesitate to ping us in #puppet-dev on irc.freenode.net. |
Split out the parts generating a hash of the object itself from adding document_type metadata. Also split out for all resources that generated the hash directly inside the to_pson method.
Use a feature to check for existence of msgpack library and add the serialization format if it is supported.
Add to_msgpack back to serializable objects. They all have the same implementation. The issue is nested objects; if an object with no msgpack implementation is contained in the "data hash" then msgpack will try to call to_msgpack on it. The alternative is to make sure there are only nested hashes, but this is not backwards compatible with the pson formats. Also strips out the internal fact _timestamp from the node object as that is of type Time and cannot be serialized.
Provide backwards compatibility for code including the old confine location inside provider.
|
I've rebased this again now. The API compatibility concerns were already addressed. |
|
@dalen @adrienthebo Thanks for working on this. I'm going to bring it up at the team's weekly planning today to run this through the home stretch. Thanks for all of your work on this! |
|
@zaphod42 was any decision reached on this? |
|
@dalen We weren't able to get to it for this week. We are fully booked on other work. I'm going to bring it up again on this Wednesday (or cycles are Wednesday-Wednesday). |
|
@dalen I'm picking this up now. Thanks for hanging in there :) |
There was a problem hiding this comment.
typo: o_data_hash = to_data_hash
|
I've fixed up the one thing that I saw in a branch. Don't worry about dealing with it, @dalen :) |
|
Hi @dalen , I'm reviewing this code change and noticed that with msgpack installed the default format switches to msgpack (previously yaml defaulted). I'm assuming this is intentional based on the weight => 20 in the format handler? |
7 participants
This set of patches adds msgpack serialization support if the required library is installed.
Apart from that they also strip out the internal fact _timestamp as that was a time object and couldn't be serialized. This is basically a bugfix but could be considered a backwards incompatible change.
They also move the confine handling out of provider to avoid a require loop as it was also used by formats.