Skip to content

Conversation

@joviegas
Copy link
Contributor

@joviegas joviegas commented Oct 17, 2025

Motivation and Context

The RequestBatchBuffer class had confusing method names that suggested it was responsible for flushing entries, when it actually just extracts them from the buffer. The actual flushing is done by RequestBatchManager. This was done to handle comment from #5418 (comment)

Modifications

Renamed three public methods in RequestBatchBuffer to better reflect what they actually do:

  • flushableRequests()extractBatchIfReady() - extracts entries when batch is full or size limit reached
  • flushableRequestsOnByteLimitBeforeAdd()extractBatchIfSizeExceeded() - extracts entries before adding a new request if size would be exceeded
  • flushableScheduledRequests()extractEntriesForScheduledFlush() - extracts entries during scheduled flush

Also renamed the private helper method extractFlushedEntries() to extractEntries() for consistency.

Updated all callers in BatchingMap, RequestBatchManager, and test files to use the new method names.

Testing

Ran the full SQS module test suite with mvn clean install -pl :sqs -P quick --am -rf :sqs.

License

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

@joviegas joviegas requested a review from a team as a code owner October 17, 2025 04:11
@joviegas joviegas added the changelog-not-required Indicate changelog entry is not required for a specific PR label Oct 17, 2025
flushBuffer(batchKey, flushableRequests);
while (!extractedEntries.isEmpty()) {
flushBuffer(batchKey, extractedEntries);
extractedEntries = requestsAndResponsesMaps.extractBatchIfReady(batchKey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this line is new, is this expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not intended , copy paste error from line 167

@joviegas joviegas enabled auto-merge October 17, 2025 18:46
@joviegas joviegas added this pull request to the merge queue Oct 23, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 23, 2025
@joviegas joviegas enabled auto-merge October 23, 2025 21:25
@sonarqubecloud
Copy link

@joviegas joviegas added this pull request to the merge queue Oct 23, 2025
Merged via the queue into master with commit 436a19b Oct 23, 2025
39 checks passed
@github-actions
Copy link

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

changelog-not-required Indicate changelog entry is not required for a specific PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants