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

Add Flush Timeout to s3 Connector #553

Merged
merged 9 commits into from
May 8, 2024
Merged

Add Flush Timeout to s3 Connector #553

merged 9 commits into from
May 8, 2024

Conversation

saegel
Copy link
Collaborator

@saegel saegel commented Mar 26, 2024

This PR adds the flush_timeout configuration option to the S3Output connector.
When the specified flush_timeout interval expires, the s3 connector will write all backlog elements, regardless whether the message backlog is actually full or not.
This prevents the connector from holding onto logs forever, if there are no new logs to process and the configured backlog size is not reached.

Changelog:

  • Implement flush_timeout configuration option for the S3Output connector
  • Add setup method where _write_backlog is scheduled
  • Make _s3_resource a cached property
  • Extend setup method to check whether the specified bucket is accessible
  • Create _handle_s3_error decorator to handle all boto3 and boto-core related exceptions.

@saegel saegel linked an issue Mar 26, 2024 that may be closed by this pull request
@saegel saegel marked this pull request as draft March 26, 2024 13:41
@saegel saegel force-pushed the s3_flush_timeout branch from 7ff807c to 7a6a4db Compare April 8, 2024 13:21
@saegel saegel force-pushed the s3_flush_timeout branch from 7a6a4db to d125974 Compare April 8, 2024 13:43
clumsy9 added 4 commits April 16, 2024 13:03
* Make `_s3_resource` a cached property.
* Move s3 session and resource creation into `setup()` method so that
  task for `flush_timeout` can be scheduled properly.
* Adapt task scheduling unit test to not interfere with local opensearch
  instance
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.44%. Comparing base (eacb5f2) to head (a817c26).
Report is 27 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #553      +/-   ##
==========================================
+ Coverage   94.39%   94.44%   +0.04%     
==========================================
  Files         144      143       -1     
  Lines       10047    10075      +28     
==========================================
+ Hits         9484     9515      +31     
+ Misses        563      560       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@clumsy9 clumsy9 marked this pull request as ready for review April 24, 2024 14:40
@clumsy9 clumsy9 requested a review from ekneg54 April 24, 2024 14:54
Copy link
Collaborator

@ekneg54 ekneg54 left a comment

Choose a reason for hiding this comment

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

thank you for your contribution.
I found some minor issues. Please have a look.

Mostly missing type hints. Please consider to scan the touched files for missing type hints I did not catch

logprep/connector/s3/output.py Outdated Show resolved Hide resolved
logprep/connector/s3/output.py Outdated Show resolved Hide resolved
logprep/connector/s3/output.py Outdated Show resolved Hide resolved
logprep/connector/s3/output.py Outdated Show resolved Hide resolved
logprep/connector/s3/output.py Show resolved Hide resolved
logprep/connector/s3/output.py Outdated Show resolved Hide resolved
logprep/connector/s3/output.py Outdated Show resolved Hide resolved
logprep/connector/s3/output.py Outdated Show resolved Hide resolved
logprep/connector/s3/output.py Outdated Show resolved Hide resolved
@ekneg54 ekneg54 self-requested a review April 25, 2024 06:54
Copy link
Collaborator

@ekneg54 ekneg54 left a comment

Choose a reason for hiding this comment

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

sorry... wrong button. Please consider to fix the issues mentioned above

@clumsy9 clumsy9 requested a review from ekneg54 May 8, 2024 12:09
@ekneg54 ekneg54 merged commit 28dbcec into main May 8, 2024
10 checks passed
@ekneg54 ekneg54 deleted the s3_flush_timeout branch May 8, 2024 20:17
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.

Flush timeout for S3 connector
4 participants