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

Update index safely between threads. Fixes #160 and #326. #355

Closed
wants to merge 1 commit into from

Conversation

worr
Copy link

@worr worr commented Oct 24, 2020

As mentioned in a warning, as well as #326
and #160, the process of
determining the index added to the default object key is not thread-safe. This
adds some thread-safety until version 2.x is out where chunk_id is used
instead of an index value.

This is not a perfect implementation, since there can still be races between
different workers if workers are enabled in fluentd, or if there are multiple
fluentd instances uploading to the same bucket. This commit is just to resolve
this problem short-term in a way that's backwards compatible.

As mentioned in a warning, as well as fluent#326
and fluent#160, the process of
determining the index added to the default object key is not thread-safe. This
adds some thread-safety until version 2.x is out where chunk_id is used
instead of an index value.

This is not a perfect implementation, since there can still be races between
different workers if workers are enabled in fluentd, or if there are multiple
fluentd instances uploading to the same bucket. This commit is just to resolve
this problem short-term in a way that's backwards compatible.

Signed-off-by: William Orr <[email protected]>
@worr
Copy link
Author

worr commented Oct 24, 2020

I tested this by deploying this to a kubernetes cluster with a single fluentd, and generating artificial log load. This uses substantially fewer HEAD calls than the existing code, and because of that, can upload faster, especially in pathological cases.

concurrent-ruby will use C-based atomic primitives when available, or else it will fall back on a simple mutex.

Again, this is not a bullet-proof solution, since the default object name can still collide with multiple fluentd's or multiple workers.

@github-actions
Copy link

github-actions bot commented Jul 6, 2021

This PR has been automatically marked as stale because it has been open 90 days with no activity. Remove stale label or comment or this PR will be closed in 30 days

@github-actions github-actions bot added the stale label Jul 6, 2021
@github-actions
Copy link

github-actions bot commented Aug 5, 2021

This PR was automatically closed because of stale in 30 days

@github-actions github-actions bot closed this Aug 5, 2021
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.

1 participant