add Retry and skip logic in AppleMediaImporter #1329
Conversation
yy9669
force-pushed
the
skipContentFailureAppleImport
branch
2 times, most recently
from
82e5bd7
to
6f189a7
Compare
|
Hi Xuyang, just saw this, I'll review it today |
|
|
||
| Collection<ErrorDetail> errors = idempotentExecutor.getRecentErrors(); | ||
| if (!errors.isEmpty() && executor instanceof RetryingInMemoryIdempotentImportExecutor) { // throw the error for retryExecutor to retry, only include the actual error message, but not the stack traces | ||
| throw new IOException(errors.iterator().hasNext() ? errors.iterator().next().exception().lines().findFirst().get() : ApplePhotosConstants.APPLE_PHOTOS_IMPORT_ERROR_PREFIX + " Unknown Error"); |
There was a problem hiding this comment.
Why do we not want to throw the entire stack trace? Is this logic necessary? Can we not just let the errors be thrown by the executor?
There was a problem hiding this comment.
If we let the error be thrown by idempotentExecutor midway of the import, then it will fail the whole batch even if only one file failed.
Here we throw after the importResult was updated, and the importResult will include all the files except for the failed one.
There was a problem hiding this comment.
I think the way your setting it up here makes sense, but is there a reason you are not including the stack trace?
There was a problem hiding this comment.
Oh just in order to make regex look simpler in Retry config
| monitor.severe( | ||
| () -> "Error importing album: ", | ||
| () -> "Fail to create album: ", | ||
| AuditKeys.jobId, jobId, | ||
| AuditKeys.albumId, mediaAlbum.getId(), | ||
| AuditKeys.errorCode, newPhotoAlbumResponse.getStatus().getCode()); | ||
|
|
||
| throw new IOException( | ||
| throw new IOException(getApplePhotosImportThrowingMessage( | ||
| String.format( | ||
| "Failed to create album, error code: %d", | ||
| newPhotoAlbumResponse.getStatus().getCode())); | ||
| "Fail to create album, error code: %d", | ||
| newPhotoAlbumResponse.getStatus().getCode()))); |
There was a problem hiding this comment.
Logging & throwing is unnecessary, we should usually only do one of the two.
If we log here, the client which received the error may double-log, or they may throw the error leaving the logs in the stack trace
|
I'm going to leave the actual review to @kateyeo because I don't understand some parts of this PR. |
yy9669
force-pushed
the
skipContentFailureAppleImport
branch
from
6f189a7
to
f168ec8
Compare
There was a problem hiding this comment.
Hi, looking over this and just wanting to make sure I'm understanding how your implementing retrying in this PR. By using the retrying executor on the entire call within importItem() the entire call would be retried if you throw a retry-able exception. As opposed to if you passed in a retrying executor to the individual importAlbums/importAllMedia calls which would allow skipping/retrying to be handled a the item level.
Let me know if you have it set up how you intended or if I'm misunderstanding anything
|
|
||
| Collection<ErrorDetail> errors = idempotentExecutor.getRecentErrors(); | ||
| if (!errors.isEmpty() && executor instanceof RetryingInMemoryIdempotentImportExecutor) { // throw the error for retryExecutor to retry, only include the actual error message, but not the stack traces | ||
| throw new IOException(errors.iterator().hasNext() ? errors.iterator().next().exception().lines().findFirst().get() : ApplePhotosConstants.APPLE_PHOTOS_IMPORT_ERROR_PREFIX + " Unknown Error"); |
There was a problem hiding this comment.
I think the way your setting it up here makes sense, but is there a reason you are not including the stack trace?
Yes the is intended, I want to retry the whole batch. Sometimes failure in one step is due to corruption in previous step, eg. fail to upload to Apple Content is because we got wrong upload url in previous step, so it's useful to retry from the first step instead of retrying current step multiple times. |
The major difficulty I met is that the IdempotentExecutor used by CallableImporter cannot be a retryable executor, so I have to use a different retryable executor in AppleMediaImporter itself.