-
Notifications
You must be signed in to change notification settings - Fork 481
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
Store entire Throwable in ErrorDetail class #1217
base: master
Are you sure you want to change the base?
Store entire Throwable in ErrorDetail class #1217
Conversation
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D43443611
@@ -232,6 +232,8 @@ private Entity createErrorEntity(String idempotentId, ErrorDetail error) throws | |||
@VisibleForTesting | |||
Entity createErrorEntity(String idempotentId, UUID jobId, ErrorDetail error) | |||
throws IOException { | |||
System.out.println("AQAQAQAQ"); |
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.
TODO delete this
… feature/errordetail-throwable
I am not sure why we need the Google Datastore dependencies, especially with the 1500 bytes limit. @jzacsh @seehamrun do you think we can drop the Datastore or tweak the limits? I am not aware how it is used in the Google deployment of DTP. |
Sorry for the late reply! We have the Datastore dependency because the GoogleCloud implementation of the executor is datastore based. So we use the datastore emulator in the tests for it, we cant really remove this dependency and we do use this code (or a version of it) in our internal Google implementation. I think the limit is only applicable for the fields that are used to index, so we can mark the
Whats the timeline you need this done by? We can look at modifying this for the specific field |
Hi @seehamrun and thanks for the reply! I am happy to make the change with marking the field as non-indexable. Do you think it needs to be tested on Google side specifically? |
Hm, maybe the best approach here is to wait for the Jackson's bugfix to serialize ...which might be coming quite soon, see this discussion. |
|
There is a place at CallableImporter where we leak the exception trace into raw logs by wrapping the
ErrorDetail
contents in anIOException
.This is bad for 3 reasons:
IOException
;As a first step towards fixing that, we want to modify
ErrorDetail
to store theThrowable
instance instead of its trace in theString
format.