Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Batch] Patch TaskOperations.add_collection with convenience functionality #3217
[Batch] Patch TaskOperations.add_collection with convenience functionality #3217
Changes from 2 commits
5d90777
70e39ec
dca1f06
4da57a8
9f9277a
bcf405f
e04778e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It seems to me that self._tasks_to_add will only ever have something in it if both a client error occurred and an early termination due to an over-sized request body - is that the case?
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.
At this line, self._tasks_to_add can be non-empty iff there was an early termination.
self._failures can be non-empty iff there were client-errors to report back to the user.
self._tasks_to_add => contains tasks not yet added
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.
Yes - so as I said. I'm not saying it's an issue - just clarifying.
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.
Sounds a bad idea, you loose the initial stacktrace by doing that. There should be a way to re-design this.
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 was wondering about moving this line into the else statement below - but I see it can be used to raise any unexpected/unrecognized exceptions that occurred during requests.
I guess it comes down to which exception you wish to take priority in raising to the user - either an error stored in task_workflow_manager.error, and a request exception (if both happened to have occurred).
Your call. :)
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 feel like if there was an unexpected exception it should be given priority as we would want to explain why we didn't finish serving the request.
Since we are losing the original stack trace from re-raising it anyways, one idea would be to add it to the custom error raised that way the only exception need to be caught would be CreateTasksErrorException. Then we could effectively communicate client failures + tasks not submitted + earlyTerminationErrors in one blob..
Thoughts?
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.
@matthchr @xingwu1 as well
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.
Given that you already have the error structure in place to report back to the user which tasks have/have not been submitted - I think it would make sense to reuse this for other the 'Unrecognized error' scenario... especially something retryable like an intermittent connection error (that may have impacted a percentage of requests - yet will be raised as a single error)