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.

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

Fix: Added exception handling for S3 processing #1551

Merged
merged 4 commits into from
Jul 5, 2022
Merged

Fix: Added exception handling for S3 processing #1551

merged 4 commits into from
Jul 5, 2022

Conversation

asifsmohammed
Copy link
Collaborator

@asifsmohammed asifsmohammed commented Jun 30, 2022 •
edited
Loading

Signed-off-by: Asif Sohail Mohammed [email protected]

Description

  • Updated exception handling for S3Object processing when there's no permission to getObject from S3
  • Added min and max validation to maximum_messages configuration option
  • Updated poll delay logic

Issues Resolved

Resolves #1544
Resolves #1550

Check List

  • New functionality includes testing.
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@asifsmohammed asifsmohammed requested a review from a team as a code owner June 30, 2022 02:57
@asifsmohammed
Added final to variables
4a36ef9 
Signed-off-by: Asif Sohail Mohammed <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2022 •
edited
Loading

Codecov Report

Merging #1551 (fc64da3) into main (d737daf) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1551   +/-   ##
=========================================
  Coverage     94.18%   94.18%           
  Complexity     1181     1181           
=========================================
  Files           165      165           
  Lines          3386     3386           
  Branches        277      277           
=========================================
  Hits           3189     3189           
  Misses          141      141           
  Partials         56       56

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a233c9f...fc64da3. Read the comment docs.

@asifsmohammed
Updated delay timer metric
22a858d 
Signed-off-by: Asif Sohail Mohammed <[email protected]>
asifsmohammed
asifsmohammed commented Jun 30, 2022
View reviewed changes
...prepper-plugins/s3-source/src/main/java/com/amazon/dataprepper/plugins/source/SqsWorker.java
Comment on lines +173 to +178
s3Service.addS3Object(s3ObjectReference);
deleteMessageBatchRequestEntry = Optional.of(buildDeleteMessageBatchRequestEntry(entry.getKey()));
sqsMessageDelayTimer.record(Duration.between(
Instant.ofEpochMilli(entry.getValue().get(0).getEventTime().toInstant().getMillis()),
Instant.now()
));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want to point out that we don't delete SQS message if there is an exception in S3ObjectWorker. Let me know if you have any ideas on how to improve the user experience here.

dlvenable
dlvenable requested changes Jun 30, 2022
View reviewed changes
...prepper-plugins/s3-source/src/main/java/com/amazon/dataprepper/plugins/source/SqsWorker.java
final S3ObjectReference s3ObjectReference) {
Optional<DeleteMessageBatchRequestEntry> deleteMessageBatchRequestEntry = Optional.empty();
// SQS messages won't be deleted if we are unable to process S3Objects because of S3Exception: Access Denied
try {
Copy link
Member

Choose a reason for hiding this comment

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

This try-catch block will suppress the exceptions thrown from #1544 . But, I'm still a little concerned that the current while(true) loop can have other exceptions thrown from it which result in this source mysteriously shutting down.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did check for any other cases of failure. Just to be sure, I will surround processSqsMessages in a try catch block.

@asifsmohammed
Updated while loop with try-catch block
fc64da3 
Signed-off-by: Asif Sohail Mohammed <[email protected]>
sbayer55
sbayer55 approved these changes Jul 5, 2022
View reviewed changes
...prepper-plugins/s3-source/src/main/java/com/amazon/dataprepper/plugins/source/SqsWorker.java
final Map.Entry<Message, List<S3EventNotification.S3EventNotificationRecord>> entry,
final S3ObjectReference s3ObjectReference) {
Optional<DeleteMessageBatchRequestEntry> deleteMessageBatchRequestEntry = Optional.empty();
// SQS messages won't be deleted if we are unable to process S3Objects because of S3Exception: Access Denied
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth creating a javadoc comment for this method to raise the visibility of this comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll make this change in the upcoming PR

...prepper-plugins/s3-source/src/main/java/com/amazon/dataprepper/plugins/source/SqsWorker.java
Instant.now()
));
} catch (final Exception e) {
LOG.warn("Unable to process S3Object: s3ObjectReference={}.", s3ObjectReference, e);
Copy link
Member

Choose a reason for hiding this comment

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

It might be outside the scope of this PR but it would be nice to expose the number of objects that threw exceptions as a metric.

Copy link
Member

Choose a reason for hiding this comment

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

We do catch these metrics further down.

@asifsmohammed asifsmohammed merged commit 406ae1e into opensearch-project:main Jul 5, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 5, 2022
@asifsmohammed @github-actions
Fix: Added exception handling for S3 processing (#1551)
5987903 
* Fix: Added exception handling for S3 processing and updated poll delay condition

Signed-off-by: Asif Sohail Mohammed <[email protected]>
(cherry picked from commit 406ae1e)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jul 5, 2022
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 5, 2022
@asifsmohammed @github-actions
Fix: Added exception handling for S3 processing (#1551)
9f01a33 
* Fix: Added exception handling for S3 processing and updated poll delay condition

Signed-off-by: Asif Sohail Mohammed <[email protected]>
(cherry picked from commit 406ae1e)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jul 5, 2022
Merged
asifsmohammed added a commit that referenced this pull request Jul 6, 2022
@opensearch-trigger-bot @asifsmohammed
Fix: Added exception handling for S3 processing (#1551) (#1565)
27f853b 
* Fix: Added exception handling for S3 processing and updated poll delay condition

Signed-off-by: Asif Sohail Mohammed <[email protected]>
(cherry picked from commit 406ae1e)

Co-authored-by: Asif Sohail Mohammed <[email protected]>
asifsmohammed added a commit that referenced this pull request Jul 6, 2022
@opensearch-trigger-bot @asifsmohammed
Fix: Added exception handling for S3 processing (#1551) (#1564)
254f933 
* Fix: Added exception handling for S3 processing and updated poll delay condition

Signed-off-by: Asif Sohail Mohammed <[email protected]>
(cherry picked from commit 406ae1e)

Co-authored-by: Asif Sohail Mohammed <[email protected]>
finnroblin pushed a commit to finnroblin/data-prepper that referenced this pull request Jul 11, 2022
@asifsmohammed @finnroblin
Fix: Added exception handling for S3 processing (opensearch-project#1551
6ec2ace 
)

* Fix: Added exception handling for S3 processing and updated poll delay condition

Signed-off-by: Asif Sohail Mohammed <[email protected]>
Signed-off-by: Finn Roblin <[email protected]>
engechas pushed a commit to engechas/data-prepper that referenced this pull request Sep 12, 2022
@asifsmohammed @engechas
Fix: Added exception handling for S3 processing (opensearch-project#1551
00925fa 
)

* Fix: Added exception handling for S3 processing and updated poll delay condition

Signed-off-by: Asif Sohail Mohammed <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbayer55 sbayer55 sbayer55 approved these changes

@dlvenable dlvenable dlvenable approved these changes

Assignees
No one assigned
Labels
backport 1.x backport 1.5
Projects
None yet
Milestone
No milestone
4 participants
@asifsmohammed @codecov-commenter @dlvenable @sbayer55