[ENG-11516] Eliminate the request between metadata and download#490
[ENG-11516] Eliminate the request between metadata and download#490antkryt wants to merge 2 commits into
Conversation
cslzchen
left a comment
cslzchen
left a comment
There was a problem hiding this comment.
⭐
Looks great and will merge after local verification.
cslzchen
left a comment
cslzchen
left a comment
There was a problem hiding this comment.
Re-review this PR with local testing and raised two questions 🔥
| if version is None and storage and storage.get('data', None): | ||
| data = storage | ||
| else: | ||
| # Capture user_id for analytics if user is logged in |
There was a problem hiding this comment.
Let's add a comment explaining that the first option is to bypass the extra request and only falls back to make that request if storage returned is None.
In addition, I don't know when this fall back happens, we should add a sentry message to log. Note: don't log any credentials. Just the node and the path.
Another thing I missed earlier is this request affects osfstorage download in general instead of just for DAZ. This means, we need to extra careful.
# Capture user_id for analytics if user is logged in
Can you take a look at if our change will lead to missing analytics? What are the analytics this request is capturing? Is it captured on WB or more likely is it sending information to OSF for OSF to capture?
| if limit is not None: | ||
| query['orm'] = True | ||
| query['limit'] = limit | ||
| if after is not None: | ||
| query['after'] = after | ||
| elif kwargs.get('orm') is not None: | ||
| query['orm'] = kwargs['orm'] |
There was a problem hiding this comment.
This may be done in a future ticket but we need to decide and document the default behavior. As it is coded right now, the behavior is:
- RAW SQL is used unless
ormis explicitly specified orlimit(pagination, which requiresorm) is used
Another thought I have is, if ORM is equivalent to raw SQL, is there a strong reason to keep both, since it adds maintenance effort to sync both?
2 participants
Ticket
https://openscience.atlassian.net/browse/ENG-11516
Purpose
Pass storage meta to provider download method
Changes
Side effects
QA Notes
Deployment Notes