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

Add Engine#msgpack_factory for v0.14 compatibility #693

Merged
merged 2 commits into from
Nov 2, 2015

Conversation

repeatedly
Copy link
Member

In v0.14, core and plugin use Engine#msgpack_factory for creating MessagePack Packer / Unpacker.
For supporting v0.12 and v0.14 in plugin code, providing same API is better.

Related to #653

@repeatedly
Copy link
Member Author

@frsyuki @tagomoris @sonots Could you check this PR? If no problem, I will merge it and release fluentd v0.12.17.

end

def unpacker(io = nil)
MessagePack::Unpacker.new(io)
Copy link
Member

Choose a reason for hiding this comment

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

I think this (and packer) method should be:

def unpacker(*args)
  MessagePack::Unpacker.new(*args)
end

because Unpacker accepts some options such as allow_unknown_ext: true in addition to io.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. I will update the patch. Thanks

@frsyuki
Copy link
Member

frsyuki commented Oct 29, 2015

I commented one thing. Other than that, looks good to me. Good job.

@repeatedly
Copy link
Member Author

Applied. Does someone have any concern?

@repeatedly
Copy link
Member Author

if there are no any concerns, I will merge this today.

repeatedly added a commit that referenced this pull request Nov 2, 2015
Add Engine#msgpack_factory for v0.14 compatibility
@repeatedly repeatedly merged commit 267cbb1 into v0.12 Nov 2, 2015
@repeatedly repeatedly deleted the add-msgpack-factory-api branch December 3, 2015 10:32
@sonots sonots added the v0.12 label Dec 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants