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

Lock buffers in order of metadata #1722

Merged
merged 4 commits into from
Nov 2, 2017

Conversation

tagomoris
Copy link
Member

When 2 or more input plugin threads try to emit events into an output plugin (and its buffer plugin instance), it may occur dead-lock, because the order of chunks to be locked are random for now.

This change introduces to sort metadata before locking chunks, to lock chunks in same order in different threads.

@tagomoris
Copy link
Member Author

@repeatedly Could you review this change?

return 1
end
# both of v1 and v2 are non-nil
if v1.keys.sort != v2.keys.sort
Copy link
Member

Choose a reason for hiding this comment

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

v1.keys and v2.keys should be assigned to local variables because each keys call creates new object.
metadata comparison is called heavily in buffers so reducing object allocation 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.

I see. Will fix it.

@repeatedly
Copy link
Member

BTW... with this patch, https://github.com/fluent/fluentd/blob/master/test/plugin/test_buffer.rb#L861 this assertion always fails.

@tagomoris tagomoris force-pushed the lock-buffers-in-order-of-metadata branch from 32c5f58 to 2369a4a Compare October 31, 2017 10:17
@tagomoris
Copy link
Member Author

I've pushed commits to fix performance issue and existing tests.

@repeatedly repeatedly merged commit 75fef8f into master Nov 2, 2017
@repeatedly
Copy link
Member

Thx!

@tagomoris tagomoris deleted the lock-buffers-in-order-of-metadata branch November 2, 2017 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants