I'll incorporate the structured saving of a PulpCodedException. That is a good idea. I'll move all the language to "failed" which sounds natural instead of "Errored".

Other suggestions are welcome.

-Brian

On 09/15/2016 09:04 PM, Michael Hrivnak wrote:
Those sound like great improvements!

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.

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.

Michael

On Thu, Sep 15, 2016 at 5:46 PM, Brian Bouterse <bbout...@redhat.com
<mailto:bbout...@redhat.com>> wrote:

    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.

    Here is a recap so everyone is aware and can give feedback:

    == Improvements over Fatal vs Non Fatal Exceptions in Tasking System ==
    - 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].

    - 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.

    - 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]

    - 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.

    == Less work for plugin writers ==
    - 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.

    == Less code to Maintain ==
    - 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].

    - 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.

    [0]:
    
https://github.com/pulp/pulp/pull/2752/files#diff-d378ced2dc71e64a3ff4985690c331b4R5
    
<https://github.com/pulp/pulp/pull/2752/files#diff-d378ced2dc71e64a3ff4985690c331b4R5>
    [1]:
    
https://github.com/pulp/pulp/pull/2752/files#diff-a5f1119f1229212c0027870093b9356cR418
    
<https://github.com/pulp/pulp/pull/2752/files#diff-a5f1119f1229212c0027870093b9356cR418>
    [2]:
    
https://github.com/pulp/pulp/pull/2752/files#diff-f2e147f346d6cb888f808c77a58e84bdR134
    
<https://github.com/pulp/pulp/pull/2752/files#diff-f2e147f346d6cb888f808c77a58e84bdR134>
    [3]:
    
https://github.com/pulp/pulp/pull/2752/files#diff-a5f1119f1229212c0027870093b9356cR426
    
<https://github.com/pulp/pulp/pull/2752/files#diff-a5f1119f1229212c0027870093b9356cR426>
    [4]:
    
https://github.com/pulp/pulp/pull/2752/files#diff-a5f1119f1229212c0027870093b9356cR33
    
<https://github.com/pulp/pulp/pull/2752/files#diff-a5f1119f1229212c0027870093b9356cR33>
    [5]:
    
https://github.com/pulp/pulp/pull/2752/files#diff-abee42f9c3a1b498c07cb513ca7cb974L629
    
<https://github.com/pulp/pulp/pull/2752/files#diff-abee42f9c3a1b498c07cb513ca7cb974L629>
    [6]:
    
https://github.com/pulp/pulp/pull/2752/files#diff-a5f1119f1229212c0027870093b9356cR366
    
<https://github.com/pulp/pulp/pull/2752/files#diff-a5f1119f1229212c0027870093b9356cR366>

    -Brian

    _______________________________________________
    Pulp-dev mailing list
    Pulp-dev@redhat.com <mailto:Pulp-dev@redhat.com>
    https://www.redhat.com/mailman/listinfo/pulp-dev
    <https://www.redhat.com/mailman/listinfo/pulp-dev>



_______________________________________________
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev

Reply via email to