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

Fixed batchExecutor to not be useless #346

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

m417z
Copy link
Contributor

@m417z m417z commented Mar 20, 2020

Promise.all() doesn't trigger the promises to start their work, creating the promise itself does. Reference:
https://stackoverflow.com/a/40639551

Currently, all batchExecutor does is waiting for the promises in chunks, but they're still all executed in parallel. This pull request changes the code so that instead of (already running) promises, promise generator functions are passed, so that the promises are created (and actually executed) in chunks.

Another unfixed issue is that batchExecutor is used recursively, so if the batch limit is, say, 50, and the tree depth is 3, the limit is actually 50^3=125000 since each batchExecutor instance manages its own limit.

@themikejr
Copy link

👍 this would be great to have!

@GeoDirk
Copy link

GeoDirk commented May 20, 2020

+1

How can we make this happen?

@Lusitaniae
Copy link

Would be nice to solve merge conflicts

@m417z m417z force-pushed the fixed-batchexecutor branch from d964a3f to f1e9cf2 Compare July 9, 2021 10:10
@m417z m417z force-pushed the fixed-batchexecutor branch from f1e9cf2 to 97eb493 Compare July 9, 2021 10:13
@Lusitaniae
Copy link

@jloosli Could you possibly give extra people write access?

@m417z and @Endran look like reasonable options as previous contributors.

@m417z
Copy link
Contributor Author

m417z commented Jul 9, 2021

@Lusitaniae done. Hopefully @jloosli will merge it soon to avoid more conflicts in the future.
@jloosli is there anything preventing from merging this pull request?

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.

4 participants