Migrate forward plugin to v0.14 api #1306
Conversation
tagomoris
force-pushed
the
migrate-forward-plugin-to-v0.14-api
branch
from
b2de9a0
to
4d9e1a7
Compare
tagomoris
changed the title
|
Pull requests to add socket/server plugin helpers will follow this change. |
|
Okay. I will review it later 👍 |
|
@repeatedly ping? |
| @thread.join | ||
| @lsock.close | ||
|
|
||
| def stop |
There was a problem hiding this comment.
stop is called before shutdown, right?
What happens when close socket before shutdown/close event loop?
Is it safe?
There was a problem hiding this comment.
Ah, it's mistake and it should be done in #close. I'll fix it.
| require 'fluent/input' | ||
| require 'fluent/plugin/socket_util' | ||
| require 'fcntl' | ||
| require 'cool.io' |
There was a problem hiding this comment.
cool.io is required by event_loop helper.
There was a problem hiding this comment.
Ah wait. This commit doesn't include socket helpers. Ignore this comment.
| class Error < StandardError; end | ||
| class ResponseError < Error; end | ||
| class ConnectionClosedError < Error; end | ||
| class ACKTimeoutError < Error; end |
There was a problem hiding this comment.
In v0.12, ForwardOutputACKTimeoutError is subclass of ForwardOutputResponseError.
Change from ResponseError to Error is intented?
There was a problem hiding this comment.
It's intended. There are no actual benefits to make these errors with sub-super classes.
And, in fact, there are no responses under ACK timeout situation.
| ["tag2", time, {"a"=>2}], | ||
| ] | ||
|
|
||
| d.run(expect_records: records.length, timeout: 5) do |
There was a problem hiding this comment.
Just for stability of CI tests.
| d.run do | ||
| entries = records.map { |tag, _time, record| [tag, _time, record] } | ||
| d.run(shutdown: false, expect_records: 2, timeout: 10) do | ||
| entries = [] | ||
| # These entries are skipped | ||
| entries << ['tag1', true, {'a' => 3}] << ['tag2', time, 'invalid record'] |
There was a problem hiding this comment.
Why use 2 lines instead of entries = [[...], [...]]?
There was a problem hiding this comment.
To reduce the number of lines of diff.
| attr_reader :responses, :exceptions | ||
|
|
||
| def write(chunk) |
There was a problem hiding this comment.
Why is this definition needed?
There was a problem hiding this comment.
Output#implement? checks the features of that plugin by checking instance methods implemented in just that class, without methods of superclasses.
This test code defines a one-time class inherits ForwardOutput. If this method is not defined, Output#implement? returns false for check of buffered output feature.
It's better to be fixed in future, but currently, this code works fine.
There was a problem hiding this comment.
I see. I understood the situation.
|
Commented. Others looks good. |
|
I pushed a commit to close sockets in |
This change is to migrate in/out_forward to v0.14 APIs, except for socket/server plugin helpers, to keep commits few and diff small.