Keepalive for out_forward plugin #2393
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Yuta Iwama <[email protected]>
Signed-off-by: Yuta Iwama <[email protected]>
ganmacs
force-pushed
the
keepalive-for-out_forward
branch
from
f20082d to
75fc463
Compare
repeatedly
added
feature request
Change to default ref count is 1 in case that socket is expired but it is used. Signed-off-by: Yuta Iwama <[email protected]>
ganmacs
force-pushed
the
keepalive-for-out_forward
branch
from
f892f42 to
52b4040
Compare
repeatedly
reviewed
Apr 23, 2019
lib/fluent/plugin/out_forward.rb
Outdated
| @@ -201,9 +205,9 @@ def configure(conf) | |||
|
|
|||
| log.info "adding forwarding server '#{name}'", host: server.host, port: server.port, weight: server.weight, plugin_id: plugin_id | |||
| if @heartbeat_type == :none | |||
| @nodes << NoneHeartbeatNode.new(self, server, failure: failure) | |||
| @nodes << NoneHeartbeatNode.new(self, server, failure: failure, keepalive: @keepalive, keepalive_timeout: keepalive_timeout) | |||
lib/fluent/plugin/out_forward.rb
Outdated
| else | ||
| node = Node.new(self, server, failure: failure) | ||
| node = Node.new(self, server, failure: failure, keepalive: @keepalive, keepalive_timeout: keepalive_timeout) |
lib/fluent/plugin/out_forward.rb
Outdated
| @@ -227,6 +231,10 @@ def configure(conf) | |||
| raise Fluent::ConfigError, "forward output plugin requires at least one <server> is required" | |||
| end | |||
|
|
|||
| if !@keepalive && @keepalive_timeout | |||
| log.warn 'The value of keepalive_timeout is ignored. if you want to use keepalive, please add `keepalive: true` to your conf.' | |||
lib/fluent/plugin/out_forward.rb
Outdated
| @@ -279,13 +287,21 @@ def start | |||
| end | |||
| end | |||
| end | |||
|
|
|||
| if @keepalive && @keepalive_timeout | |||
| timer_execute(:keep_alived_socket_watcher, @keep_alive_watcher_interval, &method(:on_purge_obsolete_socks)) | |||
There was a problem hiding this comment.
Adding out_forward prefix is better for timer.
lib/fluent/plugin/out_forward.rb
Outdated
| if expired?(key) | ||
| # Do not close this socket here in case of it will be used by other place (e.g. wait for receiving ack) | ||
| @inactive_socks[key] = @active_socks.delete(key) | ||
| @log.debug("connection #{@inactive_socks[key]} is expired. reconnecting...") |
| # This method is expected to be called in class which doesn't call #fetch_or | ||
| def revoke_by_value(val) | ||
| @mutex.synchronize do | ||
| sock = @active_socks.detect { |_, v| v.sock == val } |
There was a problem hiding this comment.
Use detect so this point guarantees no multiple entries for val, right?
There was a problem hiding this comment.
Yes. and added comment 8bffce6
| @inactive_socks[key].ref -= 1 | ||
| return | ||
| else | ||
| @log.warn("Not found key for dec_ref_by_value: #{key}") |
There was a problem hiding this comment.
How about #{key.name || key.object_id}?
There was a problem hiding this comment.
I expect key is Thread.current.object_id. So it already returns object id(and Thread.current.object_id.name raises NoMethodError I think).
lib/fluent/plugin/out_forward.rb
Outdated
| @socket_cache.fetch_or { @sender.create_transfer_socket(host || resolved_host, port, @hostname) } | ||
| else | ||
| @log.debug('connect new socket') | ||
| @sender.create_transfer_socket(resolved_host, port, @hostname) |
There was a problem hiding this comment.
Is this point also host || resolved_host?
Signed-off-by: Yuta Iwama <[email protected]>
Signed-off-by: Yuta Iwama <[email protected]>
Signed-off-by: Yuta Iwama <[email protected]>
Signed-off-by: Yuta Iwama <[email protected]>
Signed-off-by: Yuta Iwama <[email protected]>
repeatedly
reviewed
Apr 25, 2019
lib/fluent/plugin/out_forward.rb
Outdated
| def clear | ||
| @mutex.synchronize do | ||
| @inactive_socks.values.each do |s| | ||
| s.sock.close |
There was a problem hiding this comment.
Adding rescue nil is better? Hard to recover broken socket here.
lib/fluent/plugin/out_forward.rb
Outdated
| @inactive_socks.clear | ||
|
|
||
| @active_socks.values.each do |s| | ||
| s.sock.close |
lib/fluent/plugin/out_forward.rb
Outdated
| # 0 means sockets stored in this class received all acks | ||
| if @inactive_socks[k].ref <= 0 | ||
| s = @inactive_socks.delete(k) | ||
| s.sock.close |
| sock.close rescue nil | ||
| if @keepalive | ||
| @socket_cache.revoke | ||
| end |
There was a problem hiding this comment.
Following code is needed?
else
sock.close rescue nil
end
There was a problem hiding this comment.
Thanks. I didn't mean to delete the code. d3cb120
Signed-off-by: Yuta Iwama <[email protected]>
Signed-off-by: Yuta Iwama <[email protected]>
|
If no reviews from developers, I will merge soon. |
cosmo0920
approved these changes
May 7, 2019
|
Thanks for hard work! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue(s) this PR fixes:
Fixes #2188
What this PR does / why we need it:
Implemented keepalive feature for out_forward plugin.
Docs Changes:
Needed. I'll add a detail of this feature to https://github.com/fluent/fluentd-docs/blob/master/docs/v1.0/out_forward.txtfluent/fluentd-docs#632
Release Note:
No need or use PR title?
@repeatedly Will you please review?