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

backport: Fix missing error class and test for out_forward #942

Closed
wants to merge 4 commits into from

Conversation

okkez
Copy link
Contributor

@okkez okkez commented May 9, 2016

This backports #922

@tagomoris tagomoris added the v0.12 label May 9, 2016
@@ -450,9 +450,17 @@ def initialize
super
end

def format_stream(tag, es)
if @time_as_integer
Copy link
Member

@repeatedly repeatedly May 9, 2016

Choose a reason for hiding this comment

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

v0.12 doesn't hae time_as_integer option so this commit should not be backported to v0.12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed @time_as_integer.

Because v0.12 doesn't hae time_as_integer option
@repeatedly
Copy link
Member

@tagomoris Can we merge this?

def emit(tag, es, chain)
@emit_count += 1
data = es.to_msgpack_stream
data = format_stream(tag, es)
Copy link
Member

Choose a reason for hiding this comment

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

Extracting routine to format_stream is for @time_as_integer so this diff itself is not needed for this patch?
We want to reduce the change without no reason.

@tagomoris
Copy link
Member

Honestly speaking, I'm not sure this patch is needed for v0.12 branch. This change is important to add further fixes on these code/plugins, but v0.12 branch is not development branch.

Code and merging this itself is ok for me.

@repeatedly
Copy link
Member

ForwardOutputConnectionClosedError definition is needed for v0.12, right?

@tagomoris
Copy link
Member

@repeatedly Right. But it's just a bug, and its fix should be separated from test-driver fixes ideally, especially in stable branch.

@repeatedly
Copy link
Member

@tagomoris okay.

@okkez Could you create a new PR?

@okkez
Copy link
Contributor Author

okkez commented May 16, 2016

@repeatedly OK. Do we need 2 new PRs?

  • Just add ForwardOutputConnectionClosedError
  • Except above change in this PR

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