-
Notifications
You must be signed in to change notification settings - Fork 914
Refactoring the core BatchManager to change how it handles batch entry failures. #2664
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
Merged
zoewangg
merged 10 commits into
aws:feature/master/automatic-request-batching
from
16lim21:feature/master/automatic-request-batching
Aug 18, 2021
Merged
Refactoring the core BatchManager to change how it handles batch entry failures. #2664
zoewangg
merged 10 commits into
aws:feature/master/automatic-request-batching
from
16lim21:feature/master/automatic-request-batching
Aug 18, 2021
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Allows for either an exception or response to be returned by the responseMapper. Also adding tests and option to specify batchKey and batchBuffer limits.
zoewangg
reviewed
Aug 17, 2021
core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/batchmanager/BatchBuffer.java
Outdated
Show resolved
Hide resolved
core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/batchmanager/BatchBuffer.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/software/amazon/awssdk/core/internal/batchmanager/DefaultBatchManager.java
Outdated
Show resolved
Hide resolved
core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/batchmanager/BatchingMap.java
Outdated
Show resolved
Hide resolved
...-core/src/main/java/software/amazon/awssdk/core/batchmanager/BatchOverrideConfiguration.java
Outdated
Show resolved
Hide resolved
zoewangg
reviewed
Aug 18, 2021
...core/src/main/java/software/amazon/awssdk/core/internal/batchmanager/BatchConfiguration.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/software/amazon/awssdk/core/internal/batchmanager/BatchConfiguration.java
Outdated
Show resolved
Hide resolved
...src/test/java/software/amazon/awssdk/core/internal/batchmanager/IdentifiableMessageTest.java
Outdated
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed! |
zoewangg
approved these changes
Aug 18, 2021
aws-sdk-java-automation
added a commit
that referenced
this pull request
Aug 17, 2023
…5ba5b698e Pull request: release <- staging/c38bff8a-3e2b-4588-989c-5f85ba5b698e
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Currently failures in a batch entry are handled by returning an empty response of the same type as a successful response. Instead, we want to be able to handle failed batch entries by filling the returned response with an appropriate exception. Separately, the core
BatchManager
currently allows for an unlimited number ofbatchKey
s and an unlimited number of requests to be buffered for eachbatchKey
. We should limit this to prevent excessive memory usage and give customers the option to override these limits with a configurable value.Description
In this PR, I am modifying the
SqsBatchFunctions
to return an exception for each batch entry that failed. The coreBatchManager
now either expects a normal response or an Exception, and when it encounters an exception, it completes the corresponding response exceptionally. I also modified theBatchOverrideConfiguration
to accept configurable values for theBatchKey
limit andBatchBuffer
limit. Currently, exceeding this limit just returns an exception that is handled by the coreBatchManager
(handles it by completing the corresponding response exceptionally). However, how to properly handle this situation should definitely be changed. I propose that when thebatchKey
limit is reached, thebatchingMap
should remove the least recently used emptybatchKey
. Handling thebatchBuffer
limit should not remove any requests that are already buffered, instead it could maybe wait until more space opens up or return an exception directly to the customer to notify that theBatchBuffer
is full.Testing
Tests were refactored to test if exceptions were properly returned on batch entry failures. Additional tests were also added to test the functionality of the
batchKey
limit andbatchBuffer
limit being reached/exceeded.Types of changes
Checklist
mvn install
succeedsLicense