Fixed issue relating to cancelling scheduled flush and removing requests. #2636

16lim21 · 2021-08-02T22:27:16Z

Motivation and Context

Code from the last PR removed requests in a way that was very unintuitive and hacky. This PR attempts to solve that issue by not only making it more intuitive in how it removes requests, but hopefully also makes the removal of requests more thread safe.

Description

Originally, entries in the BatchBuffer were removed only when the entry's corresponding response was completed. Since a request and response were tied together in a BatchingExecutionContext, this required some hacky manipulation of an AtomicInteger (numRequests) to prevent excessive and inefficient flushes triggered by buffer sizes greater than the configured maxBatchItems. To fix this, requests are now removed when checking if a buffer can be flushed (look at canManualFlush() and canScheduledFlush() methods in BatchBuffer). By putting these methods inside a flushLock as well, it removed the need for a separate CancellableFlush and ScheduledFlush class since these synchronized blocks ensured that a manual flush could not occur while a scheduled flush had completed.

Other changes in this PR involved refactoring the code and cleaning it up to make it more readable and intuitive. More unit tests were also added.

Testing

In addition to tests implemented in the last PR, more JUnit unit tests were added to test the functionality of the supplementary classes involved with BatchManager. This specifically involved tests for BatchUtils and the IdentifiableMessage class. An additional multi-threading unit test was also added to test how the BatchManager might operate when faced with a combination of scheduled and manual flushes.

Types of changes

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

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
[] I have updated the Javadoc documentation accordingly
I have read the README document
I have added tests to cover my changes
All new and existing tests passed
A short description of the change has been added to the CHANGELOG
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

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

…-response correlation checking

… need for requestBufferCopy

…g to IdentifiableRequest

…s null but was not very efficient)

…lushes

…ls test

…to use IdentifiableMessage

…ectly

…s more intuitive

…synchronized block in batchBuffer

…HashMap

16lim21 · 2021-08-02T22:28:50Z

I forgot to sync my fork after the PR was approved so the number of commits in this PR is quite inflated. The changes since the last PR start with commit fcee2ad ("Removing isManual").

16lim21 · 2021-08-03T16:51:54Z

Code smells reported by sonarcloud mostly 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). There is one major code smell for unused method parameters but this is because I am passing class types to the builder() method like what is done in the Waiters class.

core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/batchmanager/BatchBuffer.java

core/sdk-core/src/test/java/software/amazon/awssdk/core/BatchOverrideConfigurationTest.java

...k-core/src/test/java/software/amazon/awssdk/core/internal/batchmanager/BatchManagerTest.java

zoewangg · 2021-08-04T01:03:52Z

...k-core/src/test/java/software/amazon/awssdk/core/internal/batchmanager/BatchManagerTest.java

@@ -159,14 +157,31 @@ public void scheduleFiveMessagesWithEachThreadToDifferentDestinations() {
        checkThreadedResponses(requests, responses, sendRequestFutures);
        executorService.shutdownNow();
    }
+
+    @Test
+    public void periodicallySendMessagesWithTenThreadsToSameDestination() {


What behavior is this test trying to verify?

This might have been more of a debugging test to see how periodically sending requests would work (since it would involve a combination of scheduling and manually flushing the buffers).

Hmm, sounds like this is not needed here if it's only used for debugging purpose

Should I still include this test since no other test verifies how periodically sending requests might work with the BatchManager?

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

...src/test/java/software/amazon/awssdk/core/internal/batchmanager/IdentifiableMessageTest.java

core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/batchmanager/BatchBuffer.java

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

…esting it.

…need for a separate BatchUtils

sonarqubecloud · 2021-08-04T21:41:51Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
24 Code Smells

82.6% Coverage
0.0% Duplication

Michael Li added 30 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

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

Cleaning up names and methods

cbfc34a

Michael Li added 16 commits July 29, 2021 18:40

Fixing minor issues in tests

a752923

Adding clear() method to batchingMap and toString method for debuggin…

b7c1ad5

…g to IdentifiableRequest

Hacky fix to removing requests properly (before was setting request a…

f5f2394

…s null but was not very efficient)

Adding tests for batchutils, IdentifiableRequest/Response, scheduledF…

9c9082c

…lushes

Adding batchOverrideConfiguration tests and integer overflow batchUti…

5f7a187

…ls test

Removing unnecessary methods in other classes

841cc5d

Removing IdentifiableResponses and using IdentifiableMessage instead

333ada6

Adding more IdentifiableMessage test and updating SQSIntegrationTest …

a13312b

…to use IdentifiableMessage

Renaming batchutilities package to batchmanager

3d65b2c

Exposed APIs in BatchingMap so nothing interacts with batchBuffer dir…

666a6e4

…ectly

Pass class types to builder instead of casting it

3300f8e

Adding a test to periodically send messages between multiple threads

7302e5f

Removing requests immediately when batchBuffer can be flushed which i…

d2581be

…s more intuitive

Cleaning up batchBuffer and batchingMap after previous change

c676fbf

Removing need for a cancellableFlush since requests are removed in a …

b0257cb

…synchronized block in batchBuffer

Removing logging and changing methods to return Map instead of Linked…

3b2508d

…HashMap

Merging changes and fixing checkstyle issue

a7965d7

16lim21 force-pushed the feature/master/automatic-request-batching branch from 88a82e9 to a7965d7 Compare August 3, 2021 15:40

zoewangg reviewed Aug 4, 2021

View reviewed changes

Michael Li added 3 commits August 4, 2021 11:38

Addressing half of the PR comments. Have questions on the rest

3c8d25c

Adding hashCode and equals method to batchOverrideConfiguration and t…

81248cf

…esting it.

Changing how nextBatchEntry works to be more intuitive. This removes …

3436043

…need for a separate BatchUtils

16lim21 force-pushed the feature/master/automatic-request-batching branch from 4275334 to 3436043 Compare August 4, 2021 19:47

Michael Li added 2 commits August 4, 2021 16:29

Fixing up TODOs

e3d70fc

Fixing checkstyle issue

400f832

zoewangg approved these changes Aug 5, 2021

View reviewed changes

zoewangg merged commit 00b2efc into aws:feature/master/automatic-request-batching Aug 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixed issue relating to cancelling scheduled flush and removing requests. #2636

Fixed issue relating to cancelling scheduled flush and removing requests. #2636

Uh oh!

16lim21 commented Aug 2, 2021

Uh oh!

16lim21 commented Aug 2, 2021

Uh oh!

16lim21 commented Aug 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zoewangg Aug 4, 2021

Uh oh!

16lim21 Aug 4, 2021

Uh oh!

zoewangg Aug 4, 2021

Uh oh!

16lim21 Aug 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Aug 4, 2021

Uh oh!

Uh oh!

Fixed issue relating to cancelling scheduled flush and removing requests. #2636

Fixed issue relating to cancelling scheduled flush and removing requests. #2636

Uh oh!

Conversation

16lim21 commented Aug 2, 2021

Motivation and Context

Description

Testing

Types of changes

Checklist

License

Uh oh!

16lim21 commented Aug 2, 2021

Uh oh!

16lim21 commented Aug 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zoewangg Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

16lim21 Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

zoewangg Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

16lim21 Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Aug 4, 2021

Uh oh!

Uh oh!