Refactoring and cleaning of the S3 outputs #1

ph · 2014-11-10T15:35:40Z

This PRs includes a lot of changes.

Merged @jsvd documentation improvement and added more documentation on new attributes Fix: clean up s3 output docs elastic/logstash#1340
Ruby code cleanup, following the STYLE.md
It now use the aws_config mixing support IAM roles fix s3 input: do not require aws credentials (fallback to IAM roles) elastic/logstash#1778 fix AWS credentials are not needed if using IAM Roles elastic/logstash#1575
the plugin also use standard codec, added a new codec s3_plain to support previous version
refactoring of the code to improve clarity and allow easier testing
Created initial test suite for it.
Fixed a bug causing events to be dropped when using the size_file option and the file rotation.
Fixed a bug when the tags were nil in the plain text format. fix If there is no tags in the input part, the s3 output plugin fails elastic/logstash#1626
Fixed a bug when specifying the file_size, the doc says KB but the condition was on bytes.
Added the option to specify a temporary_directory for the working files. fix S3 output use an hardcorded path for tmp and fail creating directories elastic/logstash#1860
S3 output issue when working with multiple workers added a threaded uploader fix workers > 1 - S3 output plugin - no such file or directory elastic/logstash#1915
S3 output: when the codec is started it will try to create a simple file on your bucket to test the permissions fix S3 ouput - Friendlier error when you dont have the permission on your S3 bucket.? elastic/logstash#1914

jordansissel · 2014-11-11T06:47:16Z

Does this fix elastic/logstash#1528, also?

ph · 2014-11-11T15:03:53Z

yes it also fix elastic/logstash#1528
Had a similar bug when I moved to an async uploader.

jsvd · 2014-11-21T11:26:30Z

@ph please remove the "Gemfile.orig" from the PR :)

ph · 2014-11-21T14:15:30Z

Good catch @jsvd !

luismfonseca · 2015-01-02T15:15:45Z

Can anyone estimate when will this be merged?

gemspec to require stud 0.0.18 adding integration test for s3

…'s comment

ph · 2015-01-07T15:04:56Z

@suyograo can you review this? It also have the feature describe in #2

purbon · 2015-01-08T11:51:10Z

spec/outputs/s3_spec.rb

Just a matter of style, but this two lines could be generalized using using let statements.

purbon · 2015-01-08T15:17:53Z

As a general comment on the specs, I would enforce the idea of using more the power of subjects, something like what we can see http://www.relishapp.com/rspec/rspec-core/v/2-11/docs/subject/explicit-subject#subject-in-top-level-group, what do you think?

ph · 2015-01-09T20:05:53Z

@purbon I have added the tests to ensure we don't lose any events when doing the file rotation.
Concerning using more subjects, could you take one or 2 example in the suite to give me an idea how you would do it?

suyograo · 2015-01-12T16:09:13Z

lib/logstash/outputs/s3.rb

Do we need a timestamp for this temp file name?

After thinking, this would make sure the check works if something goes wrong.
I'll add it.

jsvd · 2015-01-15T15:33:16Z

lib/logstash/outputs/s3.rb

maybe move this to the outside of the if, since it will be called either way

ph · 2015-01-15T18:06:36Z

adressed @jsvd and @purbon comments.

purbon · 2015-01-16T10:01:20Z

spec/outputs/s3_spec.rb

As far as this test are using a real S3 for testing, they should go into the integration part.

Well they test if the file rotation is done locally and we don't drop events when creating new files. Not sure why this local test would require a real S3 connection?

It requires a local connection for me, you should probably mock the connection parts.

I'll do more testing, the AWS.stub! at https://github.com/logstash-plugins/logstash-output-s3/pull/1/files#diff-14cf776b93312a411664cdaf9463b8b3R14 should mock all the connections made from the aws-sdk.

not in this case, at less for me.

Adding a note:
The call was mocked, but the S3 settings were not explicitly set in the config for the test.
The aws_mixin has multiples fallback and it was picking the config in my environment.

elasticsearch-bot · 2015-01-16T19:05:30Z

Merged sucessfully into master!

… gemspec to require stud 0.0.18 adding integration test for s3 Fixes #1

…'s comment Fixes #1

Fixes #1

This was referenced Nov 12, 2014

Fix/s3 input/output with codec elastic/logstash#1913

Closed

Update s3.rb elastic/logstash#1861

Closed

ph mentioned this pull request Nov 21, 2014

S3 output critical race condition problem fixed elastic/logstash#1528

Closed

adamvduke mentioned this pull request Dec 22, 2014

Support file compression before uploading to s3 #3

Closed

ph added 4 commits January 6, 2015 15:44

Refactoring and cleaning of the S3 outputs

d9ff83a

Use Stud.interval's sleep_then_run for the periodic uploader updating

b34e7cf

gemspec to require stud 0.0.18 adding integration test for s3

separate the filename generation from the path, following @jordansissel…

91e06a8

…'s comment

fix rotation issue with the size_file option turn on

be13950

jordansissel added the O(3) label Jan 7, 2015

purbon reviewed Jan 8, 2015
View reviewed changes

spec/outputs/s3_spec.rb Outdated

Copy link

purbon Jan 8, 2015

Choose a reason for hiding this comment

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

Just a matter of style, but this two lines could be generalized using using let statements.

ph added 3 commits January 8, 2015 10:37

code review fixes

b939bf1

specific readme part for this plugin

36f7cb5

Adding tests for the file rotation to make sure we dont drop events

2488abd

ph mentioned this pull request Jan 12, 2015

plugin fail if tags is not defined on config file #5

Closed

suyograo reviewed Jan 12, 2015
View reviewed changes

add a Timestamp to the test file

e5b73fd

wiibaa mentioned this pull request Jan 13, 2015

Added null check for tags in format_message and made temp_directory configurable elastic/logstash#1514

Closed

jordansissel added the O(3) label Jan 13, 2015

jsvd reviewed Jan 15, 2015
View reviewed changes

lib/logstash/outputs/s3.rb Outdated

Copy link

Member

jsvd Jan 15, 2015

Choose a reason for hiding this comment

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

maybe move this to the outside of the if, since it will be called either way

Changing visibility on some method and moving them around

b691d33

abort on exception

1d0600b

purbon reviewed Jan 16, 2015
View reviewed changes

ph added 5 commits January 16, 2015 10:17

Raise a Logstash:Error when something is wrong with S3

2e0d435

adding credentials for the rotating file test

4a7fb60

improve error on running the integration test

2da826e

allow to specify a region the integration testing

ff14e8f

endpoint_region was overriding the region setting

f8e50db

ph closed this in f1816fb Jan 16, 2015

ph added a commit that referenced this pull request Jan 16, 2015

Use Stud.interval's sleep_then_run for the periodic uploader updating…

6a8989c

… gemspec to require stud 0.0.18 adding integration test for s3 Fixes #1

ph added a commit that referenced this pull request Jan 16, 2015

separate the filename generation from the path, following @jordansissel…

309fea6

…'s comment Fixes #1

ph added a commit that referenced this pull request Jan 16, 2015

fix rotation issue with the size_file option turn on

45250c2

Fixes #1

ph added a commit that referenced this pull request Jan 16, 2015

code review fixes

277b484

Fixes #1

ph added a commit that referenced this pull request Jan 16, 2015

specific readme part for this plugin

dd21cab

Fixes #1

ph added a commit that referenced this pull request Jan 16, 2015

Adding tests for the file rotation to make sure we dont drop events

d836a5a

Fixes #1

ph added a commit that referenced this pull request Jan 16, 2015

add a Timestamp to the test file

f10cb3c

Fixes #1

ph added a commit that referenced this pull request Jan 16, 2015

Changing visibility on some method and moving them around

7b62c1e

Fixes #1

ph added a commit that referenced this pull request Jan 16, 2015

abort on exception

13ab83a

Fixes #1

ph added a commit that referenced this pull request Jan 16, 2015

Raise a Logstash:Error when something is wrong with S3

41afc87

Fixes #1

ph added a commit that referenced this pull request Jan 16, 2015

adding credentials for the rotating file test

a114cce

Fixes #1

ph added a commit that referenced this pull request Jan 16, 2015

improve error on running the integration test

fb4b284

Fixes #1

ph added a commit that referenced this pull request Jan 16, 2015

allow to specify a region the integration testing

2bd19d9

Fixes #1

ph added a commit that referenced this pull request Jan 16, 2015

endpoint_region was overriding the region setting

e269df1

Fixes #1

suyograo mentioned this pull request Jan 16, 2015

clean up s3 output docs elastic/logstash#1340

Closed

kooksee mentioned this pull request Aug 4, 2021

Too many open files in syste #238

Open

tcalvillo mentioned this pull request Feb 5, 2024

Logstash S3 Output plugin requires both access_key_id and secret_access_key (if both not provided throws errors) #261

Open

Refactoring and cleaning of the S3 outputs #1

Refactoring and cleaning of the S3 outputs #1

Uh oh!

Conversation

ph commented Nov 10, 2014

Uh oh!

jordansissel commented Nov 11, 2014

Uh oh!

ph commented Nov 11, 2014

Uh oh!

jsvd commented Nov 21, 2014

Uh oh!

ph commented Nov 21, 2014

Uh oh!

luismfonseca commented Jan 2, 2015

Uh oh!

ph commented Jan 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

purbon commented Jan 8, 2015

Uh oh!

ph commented Jan 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ph commented Jan 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearch-bot commented Jan 16, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants