<div dir="ltr">Those sound like great improvements!<div><br></div><div>For both fatal and non-fatal errors, in the case where the error raised is a PulpCodedException, it would be valuable to capture the extra user-facing data in a standard structure so it could be rendered in a user-friendly way.<br><div><br></div><div>In the diff, I see a mix of using the words "failed" and "errored". Assuming there's not an intentional difference, perhaps we should take this opportunity to standardize on one. "Errored" is a bit awkward to read and more awkward to say, so I'd lean toward going back to the pulp 2 state of just "error", or go with "failed". But my reasoning is only aesthetic, so really any of the choices would be fine.<br><div><br></div><div>Michael</div></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 15, 2016 at 5:46 PM, Brian Bouterse <span dir="ltr"><<a href="mailto:bbouters@redhat.com" target="_blank">bbouters@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">In porting the tasking system to work with the new models I have made some adjustments that I think are improvements. Please send feedback or leave it directly on the PR. I plan to turn this into documentation at some point as a separate effort. This applies to methods we will mark in the plugin API such as sync(), publish() which are run inside of a pulp task dedicated to calling into the plugin code.<br>
<br>
Here is a recap so everyone is aware and can give feedback:<br>
<br>
== Improvements over Fatal vs Non Fatal Exceptions in Tasking System ==<br>
- The tasking system allows non_fatal_errors to be recorded as the task runs. This is different than before which required any error recording to be done using the task result. @dkliban pointed out that recording as you go is better in case you later experience an unexpected, fatal exception. See the new plugin controller here[0].<br>
<br>
- Fatal exceptions are any exception raised during Task execution. This is roughly what we did before but we did a poor job of documenting it.<br>
<br>
- Fatal exceptions are recorded with the exception traceback in the 'result' attribute. Previously this was recorded in the 'errors' attribute which caused mixing/overwriting problems between fatal and non-fatal exceptions. In this new code, if you experience a fatal error the result *is* your exception. See the implementation here [1]<br>
<br>
- The errors field is renamed to non_fatal_errors to clearly indicate its semantic purpose to collect non_fatal_errors[2]. It is still a JSON field but it is now structured due to the use of the plugin controller [0]. It probably could be more structured, or less. Input would be great here.<br>
<br>
== Less work for plugin writers ==<br>
- spawned_tasks are now populated as you dispatch tasks from within a task. Previously it was the plugin writer's job to name any spawned tasks. This is one more responsibility moved to platform. See the implementation wherever this method is used [3]. If you want to dispatch tasks and not have them in the spawned_tasks list making it inherit from PulpTask[4] directly will do that.<br>
<br>
== Less code to Maintain ==<br>
- We no longer support sigterm handlers and are going to assume that a task can be killed at any moment. We deprecated this part of the old Plugin API on the 2.y line even. On 2.y, the tasking system used to have code which would "install" a sigterm handler callback for that specific task. That installation code is now deleted [5].<br>
<br>
- The Task.results field is a JSON field and it stores anything returned by the task [6]. We used to maintain an object called TaskResult which no longer has value with all the changes ^. Plugin writers can just return a JSON serializable python object as a task result.<br>
<br>
[0]: <a href="https://github.com/pulp/pulp/pull/2752/files#diff-d378ced2dc71e64a3ff4985690c331b4R5" rel="noreferrer" target="_blank">https://github.com/pulp/pulp/p<wbr>ull/2752/files#diff-d378ced2dc<wbr>71e64a3ff4985690c331b4R5</a><br>
[1]: <a href="https://github.com/pulp/pulp/pull/2752/files#diff-a5f1119f1229212c0027870093b9356cR418" rel="noreferrer" target="_blank">https://github.com/pulp/pulp/p<wbr>ull/2752/files#diff-a5f1119f12<wbr>29212c0027870093b9356cR418</a><br>
[2]: <a href="https://github.com/pulp/pulp/pull/2752/files#diff-f2e147f346d6cb888f808c77a58e84bdR134" rel="noreferrer" target="_blank">https://github.com/pulp/pulp/p<wbr>ull/2752/files#diff-f2e147f346<wbr>d6cb888f808c77a58e84bdR134</a><br>
[3]: <a href="https://github.com/pulp/pulp/pull/2752/files#diff-a5f1119f1229212c0027870093b9356cR426" rel="noreferrer" target="_blank">https://github.com/pulp/pulp/p<wbr>ull/2752/files#diff-a5f1119f12<wbr>29212c0027870093b9356cR426</a><br>
[4]: <a href="https://github.com/pulp/pulp/pull/2752/files#diff-a5f1119f1229212c0027870093b9356cR33" rel="noreferrer" target="_blank">https://github.com/pulp/pulp/p<wbr>ull/2752/files#diff-a5f1119f12<wbr>29212c0027870093b9356cR33</a><br>
[5]: <a href="https://github.com/pulp/pulp/pull/2752/files#diff-abee42f9c3a1b498c07cb513ca7cb974L629" rel="noreferrer" target="_blank">https://github.com/pulp/pulp/p<wbr>ull/2752/files#diff-abee42f9c3<wbr>a1b498c07cb513ca7cb974L629</a><br>
[6]: <a href="https://github.com/pulp/pulp/pull/2752/files#diff-a5f1119f1229212c0027870093b9356cR366" rel="noreferrer" target="_blank">https://github.com/pulp/pulp/p<wbr>ull/2752/files#diff-a5f1119f12<wbr>29212c0027870093b9356cR366</a><br>
<br>
-Brian<br>
<br>
______________________________<wbr>_________________<br>
Pulp-dev mailing list<br>
<a href="mailto:Pulp-dev@redhat.com" target="_blank">Pulp-dev@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/mailman<wbr>/listinfo/pulp-dev</a><br>
</blockquote></div><br></div>