-
Notifications
You must be signed in to change notification settings - Fork 4
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
DT-966: Partition TDR Snapshot Query #2712
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me.
…uery # Conflicts: # src/libs/ajax/TerraDataRepo.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok, but it'd be better to perform the API calls in parallel.
Also, I've mentioned this before, but this fix changes one API call to multiple calls, and could run into scale issues as the number of datasets grows. A solution that wouldn't have this issue is to add a POST version of this API to TDR.
src/libs/ajax/TerraDataRepo.js
Outdated
roleMap: {}, | ||
errors: [] | ||
}; | ||
for await (const sublist of partitionedIdentifiers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes each async call synchronously. Since each call is independent, they could all be made in parallel which should make this code faster. Would it be hard to change this to use a Promise.all()
construct instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes each async call synchronously. Since each call is independent, they could all be made in parallel which should make this code faster. Would it be hard to change this to use a
Promise.all()
construct instead?
Yes, I think that shouldn't be too much of a lift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pshapiro4broad - PTAL, the requests are now all executed in parallel.
I created https://broadworkbench.atlassian.net/browse/DT-974 to cover this suggestion. |
Addresses
https://broadworkbench.atlassian.net/browse/DT-966
Summary
This PR breaks up the TDR snapshot query and re-combines the results to keep the url length manageable.
Have you read Terra's Contributing Guide lately? If not, do that first.