Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: scan manager scan limit should reset each invocation #374

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

ethan-anthology
Copy link

There is currently a bug in scan manager. When running a scan or find operation without totalSegments and with limit in the ScanOptions, the itemsFetchedSoFarTotalParallelCount variable is updated with the number of items scanned but is not reset to 0.

This results in no items being scanned in subsequent calls to the scan (or find) method.
Example:

getScanManager().find(SomeEntity, {limit: 1}); // Succeeds

getScanManager().find(SomeEntity, {limit: 1}); // Returns { items: [] }, even with remaining items to scan in the table

Object.assign(getScanManager(), {itemsFetchedSoFarTotalParallelCount: 0}); // Temporary fix

getScanManager().find(SomeEntity, {limit: 1}); // Succeeds

The existing implementation of parallelScan correctly starts by setting the variable to 0 while the implementation of scan does not:

 // start with 0
 this.itemsFetchedSoFarTotalParallelCount = 0;

...

 const allPromisesResponse = await Promise.all(
      parallelScanOptions.map(options =>
        this.toLimited(this.scan<Entity>(options, metadataOptions))
      )
    );

this.scan is called (multiple times) from within parallelScan, thus we can't just add a this.itemsFetchedSoFarTotalParallelCount = 0; at the start of scan, rather I migrated the existing scan method to an internal _scan method. The new _scan method is unchanged from the original scan method, and the updated (public) scan method calls this.itemsFetchedSoFarTotalParallelCount = 0; followed by calling _scan.

This leaves the parallelScan working as it currently does, and fixes the issue when using the normal scan method.

(First time contributing here, thank you so much for the awesome library! Let me know if any changes are needed to get this pr approved. Thanks!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant