Codegenerate BatchManager API under AsyncClient and Initail Interfaces for BatchManager #5321

joviegas · 2024-06-21T16:49:17Z

Motivation and Context

PR similar to #2666. But following are considered after design reviews

The BatchManager Implementation will be hand written , thus we donot need codegenrated BatchFunctions and code generated Default batch Manager.
We will proivide batchManager() api only to AsyncClient , since returning completable future for sync Api doesnot make sense for Sync API , instead sync API can directly call send/recieve with custom BatchedRequest in its request.

Modifications

Customization

  "batchManagerSupported": true

Will result in code

Addition of scheduledExecutor similar to that of waiters()
Java code as below

    @Override
    public BatchManagerTestAsyncBatchManager batchManager() {
        return BatchManagerTestAsyncBatchManager.builder().client(this).scheduledExecutor(executorService).build();
    }

Testing

Added Junits
Also tested with SQS , made sure other service not impacted

License

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

… Batchmanager interfaces and Implementations

cenedhryn · 2024-06-21T18:22:36Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/AsyncClientClass.java

@@ -155,6 +157,10 @@ protected void addFields(TypeSpec.Builder type) {
            type.addField(AwsJsonProtocolFactory.class, "jsonProtocolFactory", PRIVATE, FINAL);
        }

+        if (model.getCustomizationConfig().getBatchManagerSupported() || model.hasWaiters()) {


Can we extract this into one conditional method like shouldAddScheduledExecutor and refer to it in two places?

cenedhryn · 2024-06-21T18:28:17Z

codegen/src/test/java/software/amazon/awssdk/codegen/poet/client/AsyncClientClassTest.java

@@ -92,6 +93,12 @@ public void asyncClientCustomPackageName() {
        assertThat(syncClientCustomServiceMetaData, generatesTo("test-custompackage-async.java"));
    }

+    @Test
+    public void asyncClientBatchManager() {
+        ClassSpec syncClientCustomServiceMetaData = createAsyncClientClass(batchManagerModels());


Nit: syncClientCustomServiceMetaData

cenedhryn · 2024-06-21T18:29:04Z

...sources/software/amazon/awssdk/codegen/poet/client/sra/test-aws-json-async-client-class.java

@@ -134,6 +134,7 @@ final class DefaultJsonAsyncClient implements JsonAsyncClient {

    private final SdkClientConfiguration clientConfiguration;

+


Nit: unnecessary extra line

cenedhryn · 2024-06-21T18:40:55Z

...sqs/src/main/java/software/amazon/awssdk/services/sqs/batchmanager/SqsAsyncBatchManager.java

+         * Sets a custom {@link software.amazon.awssdk.services.sqs.SqsClient} for polling resources. This client must be closed
+         * by the caller.


We should describe what happens if the user does not supply this parameter.

When we call sqsClient.batchManager() the sqsManager will be created from this client, thus it will be nevew null.
However, since SQSBatchManager is public API user can use builder directly , in this case client cannot be null , added a validation check in the builder

...sqs/src/main/java/software/amazon/awssdk/services/sqs/batchmanager/SqsAsyncBatchManager.java

cenedhryn · 2024-06-21T18:42:45Z

...va/software/amazon/awssdk/services/sqs/internal/batchmanager/BatchOverrideConfiguration.java

+        }
+
+        /**
+         * Define the the maximum number of messages that are batched together in a single request.


Is there a maximum number for the maximum number? What is the default?

There is no Max number but there is a Default of 10 https://github.com/aws/aws-sdk-java/blob/5b96018acb3eee64d64b8fb87a289e08f3b14694/aws-java-sdk-sqs/src/main/java/com/amazonaws/services/sqs/buffered/QueueBufferConfig.java#L29

Updated the TODO to make sure we add these values in upcoming PRs

...va/software/amazon/awssdk/services/sqs/internal/batchmanager/BatchOverrideConfiguration.java

cenedhryn · 2024-06-21T18:46:04Z

...va/software/amazon/awssdk/services/sqs/internal/batchmanager/BatchOverrideConfiguration.java

+         * @param maxBatchOpenInMs The new maxBatchOpenInMs value.
+         * @return This object for method chaining.
+         */
+        public Builder maxBatchOpenInMs(Duration maxBatchOpenInMs) {


The most common term to denote milliseconds is Millis. Can we change?

Or just remove InMs since it's a Duration ;) We should probably discuss naming in API surface area review because I'm not sure what maxBatchOpen means...

Taken from https://github.com/aws/aws-sdk-java/blob/5b96018acb3eee64d64b8fb87a289e08f3b14694/aws-java-sdk-sqs/src/main/java/com/amazonaws/services/sqs/buffered/QueueBufferConfig.java#L37-L44
However , will add a TODO to reveiw the names in API surface review

zoewangg · 2024-06-21T19:49:42Z

Can we use the existing additionalBuilderMethods customization config if we are going to handwrite the sqs batch manager anyway?

https://github.com/aws/aws-sdk-java-v2/blob/master/services/s3/src/main/resources/codegen-resources/customization.config#L291

zoewangg · 2024-06-21T19:51:18Z

...sqs/src/main/java/software/amazon/awssdk/services/sqs/batchmanager/SqsAsyncBatchManager.java

+     * @param request The SendMessageRequest to be buffered.
+     * @return CompletableFuture of the corresponding {@link SendMessageResponse}.
+     */
+    default CompletableFuture<SendMessageResponse> sendMessage(SendMessageRequest request) {


Probably out of scope of this PR, but we should probably add consumer builder overload (can be addressed in separate PR)

Good point , will mention it as TODO as class level comment

zoewangg · 2024-06-21T19:52:08Z

...va/software/amazon/awssdk/services/sqs/internal/batchmanager/BatchOverrideConfiguration.java

+ * permissions and limitations under the License.
+ */
+
+package software.amazon.awssdk.services.sqs.internal.batchmanager;


We should move this out of internal package.

zoewangg · 2024-06-21T19:53:28Z

...va/software/amazon/awssdk/services/sqs/internal/batchmanager/BatchOverrideConfiguration.java

+         * @param maxBatchOpenInMs The new maxBatchOpenInMs value.
+         * @return This object for method chaining.
+         */
+        public Builder maxBatchOpenInMs(Duration maxBatchOpenInMs) {


Or just remove InMs since it's a Duration ;) We should probably discuss naming in API surface area review because I'm not sure what maxBatchOpen means...

sonarqubecloud · 2024-06-21T22:38:54Z

Quality Gate failed

Failed conditions
35.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

joviegas · 2024-06-27T17:16:40Z

Can we use the existing additionalBuilderMethods customization config if we are going to handwrite the sqs batch manager anyway?

https://github.com/aws/aws-sdk-java-v2/blob/master/services/s3/src/main/resources/codegen-resources/customization.config#L291

These builder Methods are Static APIs on the Client , where as the BatchManager API are non-static methods which are invoked on the client , where client internally uses "this" to build batch Manager.

…s for BatchManager (#5321) * Codegenerate BatchManager API under AsyncClient and Add empty initial Batchmanager interfaces and Implementations * Addressed review comments

* Codegenerate BatchManager API under AsyncClient and Initail Interfaces for BatchManager (#5321) * Codegenerate BatchManager API under AsyncClient and Add empty initial Batchmanager interfaces and Implementations * Addressed review comments * Added Internal classes required for BatchManager Implementation * Revert "Added Internal classes required for BatchManager Implementation" This reverts commit 318969b. * Internal classes and RequestBatchManager Impelementation (#5418) * Added Internal classes required for BatchManager Implementation * Added Batch Send Implementation * Handled review comments * Handled review comments * Handled review comments * Made RequestBatchManager class a Abstract class * Checkstyle issues * Removed unused methods * New lines removed * Made public static to private state for sqsBatch functions * Constants added * Sonar cloud issues fixed * commit to check why test on codebuild * Increased Timeouts for get * Added abstract methods * Handled comments to remove Builders * Handled comments to take care when batchmanager closed while pending requests * Handled comments * Checkstyle issue * Added Consumer builders args for existing APIs of BatchManager (#5514) * Receive Batch Manager Implementation (#5488) * Add Recieve Buffer Queue And its related configuration * Update ReceiveBatch manager * Recieve Batch Manager Implementation * Receive Batch Manager Implemetation * Handled review comments * Checkstyle failure * Flsky test case fixed * Flaky test case fixed * Hamdled review comments * Handled comments * Removed ReceiveMessageCompletableFuture * SdkClosable implemented * Added ReceiveMessageBatchManager class for completeness * Checkstyle issues * Null checks * Handled comments from Zoe * Updated the defaults to 50ms same as V1 after surface area review * Revert "Updated the defaults to 50ms same as V1 after surface area review" This reverts commit e7d2295. * Bytes Based batching for SendMessageRequest Batching (#5540) * Initial changes * Initial changes 2 * Byte Based batching for SendMessage API * Byte Based batching for SendMessage API * Handled comments * Checkstyle issue * Add User Agent for Sqs Calls made using Automatic Batching Manager (#5546) * Add User Agent for Sqs Calls made using Automatic Batching Manager as hll/abm * Review comments * Update comments from PR 5488 (#5550) * Update comments of PR 5488 * Update comments from PR 5488 * Handled surface area review comments (#5563) * Initial version * Intermediate changes * Update after internal poll * ResponseCOnfiguration construction updated * RequestOverride configuration check added to Bypass batch manager * Handled review comments * Removed TODO since validations are handled in BatchPverrideConfiguration * Fix issue where the Scheduled Timeout was incorrectly completing the futures with empty messages (#5571) * Fix issue where the Scheduled Timeout was incorrectly completing the futures with empty messages * Handled review comments * Integ test for Automatic Request Batching (#5576) * feat(sqs): add BatchManager for client-side request batching to Amazon SQS The new BatchManager allows for simple request batching using client-side buffering, improving cost efficiency and reducing the number of requests sent to Amazon SQS. The client-side buffering supports up to 10 requests per batch and is supported by the SqsAsyncClient. Batched requests, along with receive message polling, help to increase throughput. * Add check for scheduledExecutor such that it not null when creating SqsAsyncBatchManager (#5582) * Add check for scheduledExecutor such that it not null when creating SqsAsyncBatchManager * Update services/sqs/src/test/java/software/amazon/awssdk/services/sqs/batchmanager/SqsAsyncBatchManagerBuilderTest.java Co-authored-by: David Ho <[email protected]> * Update services/sqs/src/test/java/software/amazon/awssdk/services/sqs/batchmanager/SqsAsyncBatchManagerBuilderTest.java Co-authored-by: David Ho <[email protected]> * Update services/sqs/src/test/java/software/amazon/awssdk/services/sqs/batchmanager/SqsAsyncBatchManagerBuilderTest.java Co-authored-by: David Ho <[email protected]> --------- Co-authored-by: David Ho <[email protected]> * Updating Timeouts in gets so that we dont wait infinitely --------- Co-authored-by: David Ho <[email protected]>

joviegas requested a review from a team as a code owner June 21, 2024 16:49

joviegas force-pushed the joviegas/sqs-batch-manager-codegen branch 2 times, most recently from 604534e to 40b2eb3 Compare June 21, 2024 17:12

Codegenerate BatchManager API under AsyncClient and Add empty initial…

e1cda0a

… Batchmanager interfaces and Implementations

joviegas force-pushed the joviegas/sqs-batch-manager-codegen branch from 40b2eb3 to e1cda0a Compare June 21, 2024 18:10

cenedhryn reviewed Jun 21, 2024

View reviewed changes

zoewangg reviewed Jun 21, 2024

View reviewed changes

Addressed review comments

fc1d90c

cenedhryn approved these changes Jun 27, 2024

View reviewed changes

joviegas merged commit d2a4d45 into feature/master/sqs-batch-manager Jun 28, 2024
14 of 17 checks passed

joviegas mentioned this pull request Aug 5, 2024

Internal classes and RequestBatchManager Impelementation #5418

Merged

1 task

joviegas deleted the joviegas/sqs-batch-manager-codegen branch May 29, 2025 15:06

		@@ -134,6 +134,7 @@ final class DefaultJsonAsyncClient implements JsonAsyncClient {

		private final SdkClientConfiguration clientConfiguration;

		* Sets a custom {@link software.amazon.awssdk.services.sqs.SqsClient} for polling resources. This client must be closed
		* by the caller.

Codegenerate BatchManager API under AsyncClient and Initail Interfaces for BatchManager #5321

Codegenerate BatchManager API under AsyncClient and Initail Interfaces for BatchManager #5321

Uh oh!

Conversation

joviegas commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Modifications

Testing

License

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoewangg commented Jun 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joviegas Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 21, 2024

Quality Gate failed

Uh oh!

joviegas commented Jun 27, 2024

Uh oh!

Uh oh!

Uh oh!

joviegas commented Jun 21, 2024 •

edited

Loading

joviegas Jun 21, 2024 •

edited

Loading