Handled surface area review comments #5563

joviegas · 2024-09-05T00:07:59Z

Motivation and Context

Surface area review comments to remove some configs from public apis.
Rename API names for config

Modifications

Update defaults
Rename API name to make it user friendly

Testing

Updated existing test cases

Screenshots (if appropriate)

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 added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
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

zoewangg · 2024-09-05T21:28:18Z

...c/main/java/software/amazon/awssdk/services/sqs/batchmanager/BatchOverrideConfiguration.java

+        Validate.isTrue(this.maxBatchSize == null || this.maxBatchSize <= 10,
+                        "The maxBatchSize must be less than or equal to 10. A batch can contain up to 10 messages.");
+
+        this.sendMessageFrequency = Validate.isPositiveOrNull(builder.sendMessageFrequency,


I understand this is most voted, but should we do sendFrequency or sendRequestFrequency? My concern is that people may confuse it to be only associated with SendMessageRequest.

agree , I forgot about the SendMessageRequest API , I think sendRequestFrequency is clearer one

zoewangg · 2024-09-05T21:29:58Z

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

            return receiveBatchManagerMap.computeIfAbsent(generateBatchKey(request), key -> createReceiveBatchManager(request))
                                         .processRequest(request);
        } else {
-            log.debug(() -> "canBeRetrievedFromQueueBuffer failed, so skipping batching for request for Queue with URL: "
-                            + request.queueUrl());
+            log.debug(() -> String.format("Batching skipped. Reason: %s", ineligibleReason));


nit: maybe call checkBatchingEligibilty here, i.e,log.debug(() -> String.format("Batching skipped. Reason: %s", checkBatchingEligibility(request))) so that it only gets called if debug level is enabled.

we need to do the check at line number 50 , if we return a boolean there and String value in Debug we end up doing all those check twice.
Also duplicate functions one to return boolean true or false and other to get the reason for false

zoewangg · 2024-09-05T21:31:32Z

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

                                            : RECEIVE_MESSAGE_ATTRIBUTE_NAMES_DEFAULT;

-        this.adaptivePrefetching = overrideConfiguration != null && overrideConfiguration.adaptivePrefetching() != null
-                                   ? overrideConfiguration.adaptivePrefetching()
+        this.adaptivePrefetching = builder.adaptivePrefetching != null


Since we removed adaptivePrefetching from the public config pojo, should we remove it from this builder as well?

sure , removed adaptivePrefetching and its usage.
Had kept it just for testing old behavior that was in V1

sonarqubecloud · 2024-09-05T23:20:47Z

Quality Gate failed

Failed conditions
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

* 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 <70000000+davidh44@users.noreply.github.com> * Update services/sqs/src/test/java/software/amazon/awssdk/services/sqs/batchmanager/SqsAsyncBatchManagerBuilderTest.java Co-authored-by: David Ho <70000000+davidh44@users.noreply.github.com> * Update services/sqs/src/test/java/software/amazon/awssdk/services/sqs/batchmanager/SqsAsyncBatchManagerBuilderTest.java Co-authored-by: David Ho <70000000+davidh44@users.noreply.github.com> --------- Co-authored-by: David Ho <70000000+davidh44@users.noreply.github.com> * Updating Timeouts in gets so that we dont wait infinitely --------- Co-authored-by: David Ho <70000000+davidh44@users.noreply.github.com>

joviegas added 3 commits August 29, 2024 18:25

Initial version

befb86c

Intermediate changes

36bd1a1

Surface API review comments

4d3f21c

joviegas requested a review from a team as a code owner September 5, 2024 00:08

joviegas force-pushed the joviegas/handle-surface-area-review-comments branch 3 times, most recently from c6f41b1 to 22034e1 Compare September 5, 2024 18:17

Update after internal poll

d2e59b9

joviegas force-pushed the joviegas/handle-surface-area-review-comments branch from 22034e1 to d2e59b9 Compare September 5, 2024 18:23

joviegas added 2 commits September 5, 2024 12:41

ResponseCOnfiguration construction updated

a81119b

RequestOverride configuration check added to Bypass batch manager

b19b34a

joviegas force-pushed the joviegas/handle-surface-area-review-comments branch from c234ad5 to b19b34a Compare September 5, 2024 20:41

zoewangg reviewed Sep 5, 2024

View reviewed changes

Handled review comments

8a62d2d

zoewangg approved these changes Sep 5, 2024

View reviewed changes

joviegas closed this Sep 5, 2024

joviegas reopened this Sep 5, 2024

joviegas merged commit bff5b97 into feature/master/sqs-batch-manager Sep 5, 2024
20 of 25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handled surface area review comments #5563

Handled surface area review comments #5563

joviegas commented Sep 5, 2024 •

edited

Loading

zoewangg Sep 5, 2024

joviegas Sep 5, 2024

zoewangg Sep 5, 2024

joviegas Sep 5, 2024

zoewangg Sep 5, 2024

joviegas Sep 5, 2024

sonarqubecloud bot commented Sep 5, 2024

Handled surface area review comments #5563

Handled surface area review comments #5563

Conversation

joviegas commented Sep 5, 2024 • edited Loading

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

zoewangg Sep 5, 2024

Choose a reason for hiding this comment

joviegas Sep 5, 2024

Choose a reason for hiding this comment

zoewangg Sep 5, 2024

Choose a reason for hiding this comment

joviegas Sep 5, 2024

Choose a reason for hiding this comment

zoewangg Sep 5, 2024

Choose a reason for hiding this comment

joviegas Sep 5, 2024

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 5, 2024

Quality Gate failed

joviegas commented Sep 5, 2024 •

edited

Loading