Skip to content

new concurrency model :single #96

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

new concurrency model :single #96

wants to merge 5 commits into from

Conversation

ph
Copy link
Contributor

@ph ph commented Sep 6, 2016 •
edited
Loading

This plugin in the current state is not thread safe and we should rely to the :single delegator.

@andrewvc andrewvc mentioned this pull request Sep 6, 2016
Closed
5 tasks
ph
ph reviewed Sep 6, 2016
View reviewed changes
lib/logstash/outputs/s3.rb
@@ -175,6 +177,7 @@ def write_on_bucket(file)
# prepare for write the file
object = bucket.objects[remote_filename]
object.write(fileIO,
:upload_id => remote_filename,
Copy link
Contributor Author

@ph ph Sep 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

AWS, changed the write a bit on a patch level.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a change in the gemspec to account for this?

@ph ph mentioned this pull request Sep 7, 2016
Closed
@ph
Copy link
Contributor Author

ph commented Sep 8, 2016

I've let the plugin for a few time and there is threadsafe issue in the current code that prevent it to use shared so I've taken the path of least resistance and used single. The problem is when elements get deleted from disk but are already in the queue. The page counter increment isnt threadsafe too.

The rewrite I did is threadsafe and uses the shared model. (see branch https://github.com/ph/logstash-output-s3/tree/feature/cleaner-code)

ph added 5 commits September 8, 2016 10:56
@ph
new concurrency model :shared
a1f3b6d
@ph
Uses :singles as the concurrency model
a8dd189
@ph
Update to 3.2.0
b5b3803
@ph
Delete test files on the remote bucket
c3fc9f2 
We are reverting the changes we did in #85, if we dont delete the files
the files can polute the bucket. So we will try to delete them and
ignore if it fails. Because in reality we only need the permission to
put an object for this plugin to correctly work.

Fixes: #97
@ph
fix typo in the changelog
808b6e0
@ph ph assigned jsvd Sep 8, 2016
jsvd
jsvd reviewed Sep 12, 2016
View reviewed changes
lib/logstash/outputs/s3.rb
@@ -79,6 +79,8 @@ class LogStash::Outputs::S3 < LogStash::Outputs::Base
config_name "s3"
default :codec, 'line'

concurrency :single
Copy link
Member

Choose a reason for hiding this comment

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

humm the PR name is "new concurrency model :shared" and the description says the plugin threadsafe, however this sets the model to :single? this means that 1 instance is shared among all threads and access to it is globally synchronized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yewps, this plugin is in fact in this current state non threadsafe, I've made the plugin run for a few hours to discover that problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update the description

@ph ph changed the title new concurrency model :shared new concurrency model :single Sep 12, 2016
@jsvd
Copy link
Member

jsvd commented Sep 12, 2016

LGTM

@elasticsearch-bot
Copy link

Pier-Hugues Pellerin merged this into the following branches!

Branch Commits
master 3ae1dd5, ba6aa97, e1b663d, afc7e66, f0365ed

elasticsearch-bot pushed a commit that referenced this pull request Sep 12, 2016
@ph
Uses :singles as the concurrency model
ba6aa97 
Fixes #96
elasticsearch-bot pushed a commit that referenced this pull request Sep 12, 2016
@ph
Update to 3.2.0
e1b663d 
Fixes #96
elasticsearch-bot pushed a commit that referenced this pull request Sep 12, 2016
@ph
Delete test files on the remote bucket
afc7e66 
We are reverting the changes we did in #85, if we dont delete the files
the files can polute the bucket. So we will try to delete them and
ignore if it fails. Because in reality we only need the permission to
put an object for this plugin to correctly work.

Fixes: #97

Fixes #96
elasticsearch-bot pushed a commit that referenced this pull request Sep 12, 2016
@ph
fix typo in the changelog
f0365ed 
Fixes #96
@ph ph deleted the feature/new-concurrency-model branch September 12, 2016 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jsvd jsvd

@andrewvc andrewvc

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ph @jsvd @elasticsearch-bot @andrewvc