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

Improve eachBatch method #1179

Merged
merged 2 commits into from
Jun 12, 2020
Merged

Conversation

SebC99
Copy link
Contributor

@SebC99 SebC99 commented May 30, 2020

Currently, what's happening with the eachBatch Query method is this:

  • find X objects
  • execute a callback on these objects
  • find the next X objects
    The issue is that while executing the callback, nothing is done, and we have to wait for the next Find to succeed to start again the callback.

An easy improvement is to launch the next query in parallel of the execution of the callback, so that when the callback is done, the next batch is already ready to be consumed (or more advanced if the query is longer than the callback execution)

It's done by changing the order of the promises, and waiting for both the query and the callback on the previous batch to be resolved.

@SebC99
Copy link
Contributor Author

SebC99 commented May 30, 2020

I don't know why the tests are failing, I hope it has nothing to do with the modifications...

@SebC99
Copy link
Contributor Author

SebC99 commented May 30, 2020

Ok so I had to change the way I tested for the end of continueWhile as in the test suite, the find mock doesn't take into account the where clause on the objectId.
Instead of rewriting all the tests for eachBatch, each, map, reduce, and filter I've modified the last call of the promise.

@codecov
Copy link

codecov bot commented May 30, 2020

Codecov Report

Merging #1179 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1179   +/-   ##
=======================================
  Coverage   92.21%   92.22%           
=======================================
  Files          54       54           
  Lines        5268     5273    +5     
  Branches     1178     1180    +2     
=======================================
+ Hits         4858     4863    +5     
  Misses        410      410           
Impacted Files Coverage Δ
src/ParseQuery.js 96.23% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e6af5a...61f8ef9. Read the comment docs.

@SebC99 SebC99 force-pushed the queryBatchOptimized branch from 44f08f7 to 402ace4 Compare May 30, 2020 12:38
@SebC99
Copy link
Contributor Author

SebC99 commented May 30, 2020

(sorry for the force-pushed, I had wrongly pushed another commit with the "built" version I also have)

Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements. LGTM!

@dplewis
Copy link
Member

dplewis commented Jun 11, 2020

@SebC99 Sorry for the late reply? Can you rebase your branch?

@SebC99 SebC99 force-pushed the queryBatchOptimized branch from 402ace4 to 61f8ef9 Compare June 12, 2020 08:10
@SebC99
Copy link
Contributor Author

SebC99 commented Jun 12, 2020

Done :)

@dplewis dplewis merged commit 8a69f95 into parse-community:master Jun 12, 2020
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.

2 participants