-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat: Add dif to assemble response, Add error handling #7207
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
Conversation
Generated by 🚫 danger |
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 good generally. One thing though: Isn't it possible to load the ProjectDsymFile
along with the File
s (via joining) to reduce the number of DB requests?
src/sentry/api/bases/chunk.py
Outdated
elif len(file_blobs) == len(owned_blobs) == len(chunks) and file is not None: | ||
return self._create_file_response( | ||
ChunkFileState.OK | ||
file.headers.get('__state', ChunkFileState.OK) |
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.
That could use a constant, maybe
file=found_file | ||
).first() | ||
if dsym is not None: | ||
response['dif'] = serialize(dsym) |
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 should even be required if the state is OK
and log an error if the ProjectDsymFile
is missing since it indicates that something went wrong and we weren't able to write the correct state header.
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.
We can't check this here, since the original chunked file will be deleted, only the new dsym file is in the database without the __state
header at all.
So pretty much the API returns OK
if there is no state header but a file with owned blobs.
def _add_project_dsym_to_reponse(self, found_file, response): | ||
if found_file is not None: | ||
if found_file.headers.get('error', None) is not None: | ||
response['error'] = found_file.headers.get('error') |
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.
Maybe just a minor consistency thing, but: What if there is an error
header but the state is OK
? In that case it is weird, that you're adding that field.
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.
The database doesn't prevent this from happening, but there is no codepath setting OK
and error
. So when there is an error
state is also an error
.
In regards to your first comment. |
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.
wfm
No description provided.