-
Notifications
You must be signed in to change notification settings - Fork 80
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 Overflow in download analysis metadata #3159
fix Overflow in download analysis metadata #3159
Conversation
qiita_db/handlers/analysis.py
Outdated
finally: | ||
del chunk | ||
# pause the coroutine so other handlers can run | ||
await gen.sleep(0.000000001) # 1 nanosecond |
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.
wouldn't this be taken care of by the await
on 84?
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.
Nop, in fact, if you check the code in the reference, the magic actually happens here; without this everything goes 🍌 ... at least based on that link.
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.
for the large data case, it shouldn't be necessary, but since this code is used on small targets it seems reasonable. it's ashame the await None
doesn't work. it's possible yield None
does but it may depend on aspects of decoration that i dont understand
if mf_fp is not None: | ||
df = qdb.metadata_template.util.load_template_to_dataframe( | ||
mf_fp, index='#SampleID') | ||
response = df.to_dict(orient='index') | ||
response = dumps(df.to_dict(orient='index')) |
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.
i think all of this makes sense, but I think it should be possible to do this outside of the TRN context, and close the transaction?
qiita_db/handlers/analysis.py
Outdated
await self.flush() | ||
except StreamClosedError: | ||
break | ||
finally: |
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.
I think the intention here is for this to be else
?
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.
I literally borrow this code from the link in the notes ...
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.
sure but it doesn't seem necessary here as this method essentialy returns right after StreamClosedError
is caught. i don't really care though, so up to you
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.
You are right, I guess the difference is how the original code is creating the chunks vs. how it's done here ... I'll remove.
@wasade, finally passing; could you take another look? Thank you. |
Thanks @wasade ! |
No description provided.