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

Introduce socket plugin helper #1356

Merged
merged 8 commits into from
Dec 9, 2016
Merged

Conversation

tagomoris
Copy link
Member

This change introduces socket plugin helper, and migrate out_forward to v0.14 API fully with plugin helpers.

This change does:

  • add fluent/plugin_helper/socket and #socket_create (_tcp, _udp) methods
  • fix server plugin helper to call it with existing socket (only for UDP, to create a server without any assigned port)
  • fix output plugin to move it into recovery (retry) state when chunk is rollbacked in async mode
  • rewrite out_forward with socket/timer/server/thread plugin helpers using #try_write for at-least-once mode

This change doesn't:

  • add tests for socket plugin helper itself ... not yet
  • tcp connection pools and/or keepalive implementation

@tagomoris
Copy link
Member Author

@repeatedly Could you review this change?

@@ -898,9 +900,9 @@ def commit_write(chunk_id, delayed: @delayed_commit, secondary: false)
@retry_mutex.synchronize do
if @retry # success to flush chunks in retries
if secondary
log.warn "retry succeeded by secondary.", plugin_id: plugin_id, chunk_id: dump_unique_id_hex(chunk_id)
log.warn "retry succeeded by secondary.", chunk_id: dump_unique_id_hex(chunk_id)
Copy link
Member

Choose a reason for hiding this comment

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

Why remove plugin_id from the logs? Redundancy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Redundancy. Plugin logger fills it automatically now.

def update_retry_state(chunk_id, using_secondary, error = nil)
@retry_mutex.synchronize do
@counters_monitor.synchronize{ @num_errors += 1 }
chunk_id_hex = dump_unique_id_hex(chunk_id)
Copy link
Member

Choose a reason for hiding this comment

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

chunk_id_hex is used in only log message.
Not critical but avoid dump_unique_id_hex is bit better.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are almost no pass not to output a log with chunk_id_hex.
It's just:

  • @retry exists, and
    • without error (called from #rollback_commit)
    • or
    • @retry.limit? is true (all chunks will be cleared)

That variable is referred in most cases, so I think it's worth to assign into a variable to reduce code complexity.


# TODO: implement connection pool for specified host

def socket_create(host, port, proto: :tcp, **kwargs, &block)
Copy link
Member

Choose a reason for hiding this comment

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

How about proto argument is changed to required?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I'll update to socket_create(proto, host, port, **kwargs, &block).

when :unix
raise "not implemented yet"
else
raise ArgumentError, "invalid protocol"
Copy link
Member

Choose a reason for hiding this comment

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

"invalid protocol: #{proto}" is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

gotcha.

@tagomoris tagomoris merged commit 3613b32 into master Dec 9, 2016
@tagomoris tagomoris deleted the introduce-socket-plugin-helper branch December 9, 2016 02:25
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.

2 participants