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
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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