Implementing BatchManager in sdk-core #2613

16lim21 · 2021-07-21T18:18:05Z

Motivation and Context

The general batching solution in sdk-core will eventually allow for feature parity with v1 SQS buffered client as requested here. This will also streamline batch write operations for services that already support batch requests (ex. SQS with sendMessageBatch, Kinesis with putRecords).

Description

Implemented the core batch manager with generic functionality to automatically batch requests client-side. This PR mainly implements the sendRequest method that automatically buffers outgoing requests and flushes them when (a) a maxBatchItems limit is reached or (b) a timeout occurs as specified by the maxBatchOpenInMs parameter.

Testing

Changes were tested with two sets of junit tests. The first is a set of generic tests that are implemented in sdk-core. It passes arbitrary functions to the batch manager and is used to test the general functionality of the batch manager. This includes tests to see whether requests are flushed when reaching the maxBatchItems limit, whether timeouts work, and whether or not multi-threading would break the batch manager. A second set of tests are implemented in SQS to show how the batch manager might interact with an AWS service. These tests are essentially the same as the generic tests but the functions provided are SQS specific (ie they actually send the requests to SQS).

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

License

I confirm that this pull request can be released under the Apache 2 license

… code as well

…threading

…-explanatory. Also refactored sdk-core tests

...sdk-core/src/main/java/software/amazon/awssdk/core/internal/batchutilities/BatchManager.java

.../src/main/java/software/amazon/awssdk/core/internal/batchutilities/BatchAndSendFunction.java

...sdk-core/src/main/java/software/amazon/awssdk/core/internal/batchutilities/BatchManager.java

...-core/src/test/java/software/amazon/awssdk/core/internal/batchutilities/BatchBufferTest.java

services/sqs/src/test/java/software/amazon/awssdk/services/sqs/BatchBufferSqsTest.java

…-response correlation checking

… need for requestBufferCopy

zoewangg

I think batchKey is better than batchGroupId, WDYT?
Looks like GetBatchGroupIdFunction is missing from the PR. Let's fix it. Also, how about naming it BatchKeyMapper?

...sdk-core/src/main/java/software/amazon/awssdk/core/internal/batchutilities/BatchManager.java

...core/src/main/java/software/amazon/awssdk/core/internal/batchutilities/BatchingGroupMap.java

…uration to manage configuration values

…sh was a manual flush. Also adding batchUtils

…nd remove request methods

zoewangg

Overall LGTM. Good job! Just left a few very minor comments.

...sdk-core/src/main/java/software/amazon/awssdk/core/internal/batchutilities/BatchManager.java

...ain/java/software/amazon/awssdk/core/internal/batchutilities/BatchOverrideConfiguration.java

...sdk-core/src/main/java/software/amazon/awssdk/core/internal/batchutilities/BatchManager.java

...-core/src/test/java/software/amazon/awssdk/core/internal/batchutilities/BatchBufferTest.java

...ain/java/software/amazon/awssdk/core/internal/batchutilities/BatchOverrideConfiguration.java

…p names etc.)

sonarcloud · 2021-07-28T20:13:34Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
75 Code Smells

69.9% Coverage
0.0% Duplication

16lim21 · 2021-07-28T20:40:05Z

Code smells reported by sonarcloud are mostly not related to the change. The code smells that do relate to the changes made in this PR have to do with how I name some generics but those follow the style of the V2 SDK (ex. RequestT as opposed to just T).

…1a6a0606f Pull request: release <- staging/0d1d0d73-fb64-4c0d-82bd-9b21a6a0606f

axel-op · 2024-05-21T09:09:06Z

...core/src/main/java/software/amazon/awssdk/core/internal/batchutilities/CancellableFlush.java

+    private volatile boolean isCancelled = false;
+    private volatile boolean hasExecuted = false;


You don't need to make these variables volatile if you only access them using synchronized blocks.

See Java - is volatile required with synchronized? on StackOverflow and volatile fields in the Java Language Specification.

Michael Li added 22 commits July 13, 2021 14:28

Adding batchBuffer and testing code

8abe56f

Adding sdk-core tests and fixing request to response mapping

55d6849

Updating sdk-core tests with object to mimic batch request entry

f6e485a

Adding wrapper classes for functions

fa01f37

Making some changes for thread safety and updating tests

7a2c0b9

Refactoring code and how requests are cleared. Simplifying scheduling…

be0d218

… code as well

Changing scheduled flush code. Need to test in next commit

d89d35f

Adding more test cases and scheduling periodically instead of just once

7191141

Refactoring responseMap into a wrapper class

6efeb64

Fixing issues related to periodic scheduling. Need to test with multi…

aec0d5c

…threading

Using map of currentIds instead of a singular currentId.

db83454

Fixed race conditions related to cancelling scheduled buffer flush

2c55e01

Resetting cancellableFlush's flags after flush is completed

4b77e0b

Multithreading test works. need to clean up code and recommit

fe6950a

Found another way to reset cancellableFlush flags. Also cleaning up code

1e3d0d8

Updating request batching tests with multi threading tests

ec2f127

Renamed to batchManager and cleaned up variable names to be more self…

1f9bf88

…-explanatory. Also refactored sdk-core tests

Refactored sdk-core tests

b11b1b9

Fixing checkstyle issues

581a620

Fixing checkstyle issues in the tests as well as refactoring some tests

043619d

Updating and adding SQS batching tests and removing xml.bind

102424b

Using existing Md5Utils for Sqs batch tests

4b6ea92

zoewangg reviewed Jul 22, 2021

View reviewed changes

Michael Li added 7 commits July 22, 2021 11:58

Naming and style changes to address PR comments

d1efa49

Refactoring tests to use maps instead of arrays for better request-to…

4b4cb85

…-response correlation checking

Name changes, refactoring code and addressing minor PR comments

2528a65

Refactoring SQS integration tests to use identifiable responses

d76a4ff

Combining request and response maps into one BatchingMap

cff589a

Renaming generics as RequestT, ResponseT etc.

e8089e6

Refactoring tests to calculate batchGroupId from request and removing…

f478261

… need for requestBufferCopy

zoewangg reviewed Jul 25, 2021

View reviewed changes

Michael Li added 13 commits July 26, 2021 11:17

Cleaning up names and methods

cbfc34a

Adding the batchGroupId function that I forgot last time

5c94dd2

Refactoring batchManager to use builder pattern and an overrideConfig…

915be6f

…uration to manage configuration values

Removing need for separate executor (just use scheduled executor)

e387b21

Adding javadocs and renaming variables/classes to be more intuitive

0b17fcb

Refactoring incrementing currentId to a separate BatchUtils class

7849b42

Refactoring currentIds map and scheduledFlushes map into the BatchBuffer

660803b

Removing need to check if flush has executed but need to check if flu…

d9a40c3

…sh was a manual flush. Also adding batchUtils

Small changes to address github PR comments

826fab3

Removing completionService and just using CompletableFuture.supplyAsync

424dd62

Adding code to cover exception thrown from the sendAndBatch function

d2725ec

Adding exception test and fixing batchingExcution locking

89d55f1

Adding lock object back into batchingExecutionContext for both read a…

137b340

…nd remove request methods

zoewangg reviewed Jul 28, 2021

View reviewed changes

Michael Li added 2 commits July 28, 2021 11:25

Addressing minor github PR comments (making classes final, cleaning u…

7ac8916

…p names etc.)

Just renaming cancelScheduledFlushIfNeeded method

52e6316

zoewangg approved these changes Jul 28, 2021

View reviewed changes

zoewangg merged commit 329566c into aws:feature/master/automatic-request-batching Jul 28, 2021

aws-sdk-java-automation added a commit that referenced this pull request Jul 6, 2023

Merge pull request #2613 from aws/staging/0d1d0d73-fb64-4c0d-82bd-9b2…

72d5210

…1a6a0606f Pull request: release <- staging/0d1d0d73-fb64-4c0d-82bd-9b21a6a0606f

axel-op reviewed May 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing BatchManager in sdk-core #2613

Implementing BatchManager in sdk-core #2613

16lim21 commented Jul 21, 2021

zoewangg left a comment

zoewangg left a comment

sonarcloud bot commented Jul 28, 2021

16lim21 commented Jul 28, 2021

axel-op May 21, 2024

		private volatile boolean isCancelled = false;
		private volatile boolean hasExecuted = false;

Implementing BatchManager in sdk-core #2613

Implementing BatchManager in sdk-core #2613

Conversation

16lim21 commented Jul 21, 2021

Motivation and Context

Description

Testing

Types of changes

Checklist

License

zoewangg left a comment

Choose a reason for hiding this comment

zoewangg left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 28, 2021

16lim21 commented Jul 28, 2021

axel-op May 21, 2024

Choose a reason for hiding this comment