Skip to content
This repository was archived by the owner on Jun 4, 2019. It is now read-only.

Handle s3:TestEvent (and other unexpected events) #524

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

akarve
Copy link
Member

@akarve akarve commented May 15, 2019

  • Make indexer more robust to unexpected event types
  • Slight readability tweaks
  • Testing

@akarve akarve requested review from meffij and stevededalus May 15, 2019 02:07
@codecov
Copy link

codecov bot commented May 15, 2019

Codecov Report

Merging #524 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #524   +/-   ##
=======================================
  Coverage   86.06%   86.06%           
=======================================
  Files          25       25           
  Lines        3395     3395           
=======================================
  Hits         2922     2922           
  Misses        473      473

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 0315cdc...c4bd76f. Read the comment docs.

raw_message = json.loads(outer['body'])['Message']
message = json.loads(raw_message)
if 'Records' not in message:
# consume event (we don't want to index it)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure to be the only consumer of the events in this Queue? If not, it might be worth writing a special case for TestEvent and simply ignoring (not consuming) other events.

Copy link
Member Author

Choose a reason for hiding this comment

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

@stevededalus is there a way to ignore events without consuming them? from what i read below, consumption == lambda task did not throw.

@@ -185,7 +194,8 @@ def handler(event, _):
elif eventname == 'ObjectCreated:Put':
event_type = 'Create'
else:
event_type = eventname
# we should only send either Create or Delete events
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Event handling is kind of weird in the indexer before your change, but we need to handle many more event types than what are explicitly mentioned. https://docs.aws.amazon.com/AmazonS3/latest/dev/NotificationHowTo.html#notification-how-to-event-types-and-destinations

Copy link
Contributor

Choose a reason for hiding this comment

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

Returning without handling these events will lead to objects not being indexed correctly

@akarve akarve requested review from kevinemoore and meffij May 17, 2019 05:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants