-
Notifications
You must be signed in to change notification settings - Fork 239
S3 batch #179
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
S3 batch #179
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After making the one field name change (result VS results), I was able to successfully run an S3 Batch Operations job that invokes a Lambda written using these models.
private String invocationSchemaVersion; | ||
private String treatMissingKeysAs; | ||
private String invocationId; | ||
private List<Result> result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private List<Result> result; | |
private List<Result> results; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
Since we have the ResultCode enum now, I'm thinking S3BatchOpResponse.treatMissingKeysAs should be of type ResultCode.
The following are ideas that might go too far for this library; let me know what you think.We could default treatMissingKeysAs to PermanentFailure, as this is likely the value most people will use.
We could have a special constructor to seed a response instance. These must be set for the response to be considered valid. If one does not want to use this, they can use the other constructors.
|
Hi David, Great feedback! What about something like this in the S3BatchResponse.java class. public static S3BatchResponseBuilder fromS3BatchEvent(S3BatchEvent s3BatchEvent) {
return S3BatchResponse.builder()
.withInvocationId(s3BatchEvent.getInvocationId())
.withInvocationSchemaVersion(s3BatchEvent.getInvocationSchemaVersion());
} I think I'd be nervous adding any default behavior which might have a negative impact. I don't know this area of the platform that well, so I would definitely appreciate any feedback. Thanks, Mark |
The I think the question now is should
I'm on the fence. The API allows for multiple tasks and results, however it is certainly 1 task per event today. |
In my opinion no. If all that was required was the task id it would make sense. But the user will still have to add their own values for resultCode and resultString. So I don't think there is much to be gained. |
Changed the type of treatMissingKeysAs from String to ResultCode
I've also tested this now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks @msailes and @david-kingsbury-bah! :) |
Issue #, if available:
#177
Description of changes:
Event and response objects for S3 Batch operations.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.