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

S3 batch #179

Merged
merged 4 commits into from Oct 28, 2020
Merged

S3 batch #179

merged 4 commits into from Oct 28, 2020

Conversation

msailes
Copy link
Collaborator

@msailes msailes commented Oct 21, 2020

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.

Copy link

@david-kingsbury-bah david-kingsbury-bah left a 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;

Choose a reason for hiding this comment

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

Suggested change
private List<Result> result;
private List<Result> results;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.

@david-kingsbury-bah
Copy link

david-kingsbury-bah commented Oct 21, 2020

Since we have the ResultCode enum now, I'm thinking S3BatchOpResponse.treatMissingKeysAs should be of type ResultCode.

private ResultCode treatMissingKeysAs;

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.

private ResultCode treatMissingKeysAs = ResultCode.PermanentFailure;

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.

    public S3BatchResponse(final S3BatchEvent request) {
        this.invocationSchemaVersion = request.getInvocationSchemaVersion();
        this.invocationId = request.getInvocationId();
        this.results = Collections.singletonList(
                Result.builder()
                        .withTaskId(request.getTasks().get(0).getTaskId())
                        .build()
        );
    }

@msailes
Copy link
Collaborator Author

msailes commented Oct 21, 2020

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

@david-kingsbury-bah
Copy link

The fromS3BatchEvent method looks great!

I think the question now is should fromS3BatchEvent go as far as my example and create a single Result with the Task ID set.

  • S3 Batch Ops expects the response result Task ID to match what was passed in through the request (S3BatchEvent). I suspect this allows S3 Batch Ops to do many concurrent invocations and still be able to gather and sort results.
  • S3 Batch Ops only sends 1 task, and expects only 1 result. Here is a snippet from their docs
    # S3 batch operations currently only passes a single task at a time in the array of tasks.
    task = event['tasks'][0]
    

I'm on the fence. The API allows for multiple tasks and results, however it is certainly 1 task per event today.

@msailes
Copy link
Collaborator Author

msailes commented Oct 21, 2020

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
@msailes
Copy link
Collaborator Author

msailes commented Oct 22, 2020

I've also tested this now.

Copy link
Contributor

@carlzogh carlzogh left a comment

Choose a reason for hiding this comment

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

LGTM!

@carlzogh carlzogh merged commit d6d2a27 into aws:master Oct 28, 2020
@carlzogh
Copy link
Contributor

Thanks @msailes and @david-kingsbury-bah! :)

raupachz pushed a commit to raupachz/aws-lambda-java-libs that referenced this pull request Dec 1, 2020
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.

None yet

3 participants