Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix for issue #865: composite addon error manipulation #978

Merged
merged 6 commits into from Feb 20, 2013

Conversation

Projects
None yet
3 participants
Collaborator

caridy commented Feb 12, 2013

This PR attempts to solve issue #865 and #25.

An important child fails thru a call to ac.error, as a result, the parent should also fail. For this use-case, we are proposing a flag at the child level configuration:

{
    "type": "ParentMojit",
    "config": {
        "children": {
            "slot1": {
                "type": "ReallyImportant",
                "propagateFailure": true,
                "config": {}
            },
            "slot2": {
                "type": "NotImportant",
                "propagateFailure": false,
                "config": {}
            },
            "slot3": {
                "type": "NotSoImportantWithDefaultSetting",
                "config": {}
            }
        }
    }
}

In the example above, slot2 and slot3 can fail, in which case the data passed into the parent view will be an empty string. And that's the default behavior. In the other hand, if the mojit in slot1 fails, the error will be propagated, and ParentMojit will also fail by calling ac.error() and piping the original error from child on slot1 due to the special configuration propagateFailure, which is set to true. This give us the control needed to define important childs.

Collaborator

caridy commented Feb 16, 2013

this is ready for review. /cc @drewfish

@drewfish drewfish commented on an outdated diff Feb 19, 2013

lib/app/addons/ac/composite.common.js
},
+ abort: function (err) {
+ var adapter = this.adapter;
+ // an error occur in a child marked with propagateFailure flag
+ adapter.error(err);
+ // cleaning up the house
+ this.queue.results = [];
+ // cancel any pending job by invalidating the callback of the queue
+ this.queue.skip = true;
@drewfish

drewfish Feb 19, 2013

Contributor

The skip property doesn't appear to be part of the Parallel API. Can we do this.skip instead?

@drewfish drewfish commented on an outdated diff Feb 19, 2013

lib/app/addons/ac/composite.common.js
- // Make a new "command" that works in the context of this
- // composite
- newCommand = {
- instance: child,
- // use action in child spec or default to index
- action: child.action || 'index',
- context: command.context,
- params: child.params || command.params
- };
+ return {
@drewfish

drewfish Feb 19, 2013

Contributor

Might be nice to add a comment here that this datastructure is part of the buffer argument to the callback of this.queue.done. (It took me a little bit to figure this out, in part because the documentation for Y.Parallel is so thin.)

@drewfish drewfish commented on an outdated diff Feb 19, 2013

lib/app/addons/ac/composite.common.js
-
- if (buffer[name].meta) {
- meta = Y.mojito.util.metaMerge(meta,
- buffer[name].meta);
- }
- }
- }
+ meta.children = children;
+
+ for (child in children) {
+ if (children.hasOwnProperty(child)) {
+ this.add(child, children[child]);
+ }
+ }
+
+ this.queue.done(function (buffer) {
@drewfish

drewfish Feb 19, 2013

Contributor

Can we rename the buffer argument to results or childResults or something like that? buffer isn't very descriptive.

@drewfish

drewfish Feb 19, 2013

Contributor

Hmm... perhaps it might be clearer to use this.queue.results instead of buffer.

@drewfish drewfish commented on an outdated diff Feb 19, 2013

lib/app/addons/ac/composite.common.js
},
+ abort: function (err) {
+ var adapter = this.adapter;
+ // an error occur in a child marked with propagateFailure flag
+ adapter.error(err);
+ // cleaning up the house
+ this.queue.results = [];
+ // cancel any pending job by invalidating the callback of the queue
+ this.queue.skip = true;
+ // invalidate this.adapter.done/error
+ adapter.error = adapter.done = function () {};
@drewfish

drewfish Feb 19, 2013

Contributor

I wonder if we can put do some useful logging in this replacement, instead of just no-op.

(The situation is made a little more confusing because ac.composite doesn't actually call this.adapter.done() itself, but instead calls the cb argument to ac.composite.execute(). However, we do call this.adapter.error() whenever a child fails and propagates that failure.)

Contributor

drewfish commented Feb 19, 2013

Looks pretty good, but the error handling could be a little cleaner I think. Instead of this.queue.skip perhaps use this.failed. Then, it might be a little clearer how/when we should call this.adapter.error() and when we shouldn't.

@drewfish drewfish commented on an outdated diff Feb 19, 2013

lib/app/addons/ac/composite.common.js
},
+ abort: function (err) {
+ var adapter = this.adapter;
+ // an error occur in a child marked with propagateFailure flag
+ adapter.error(err);
+ // cleaning up the house
+ this.queue.results = [];
@drewfish

drewfish Feb 19, 2013

Contributor

I understand why we're doing this, but I don't think this is the best approach. Perhaps instead we can do this.queue = null if that works.

In fact, we should be clearing out this.queue regardless of whether we succeed or fail.

Collaborator

caridy commented Feb 20, 2013

@drewfish few notes after the last commit:

  • this.failed in now in place, but this.queue is not destroyed (to null). The reason for that is people can try to add childs at any time using addChild public method. Whether that's before of after an error occur, it does not matter.
  • I made _onChildFailure private, we don't want people to use that. Also, I figured that by calling adapter.error() in the first place should invalidate adapter.done and any succesive call, so we are covered there.
  • Finally, the failFast thing, I don't think we need that. If there is an explicit propagation flag, an error should fail fast. If there is not such as flag in any child, then there is not need to worry about errors, not need to aggregate them, and there is no way to consume them, so, I think we can leave that out.
Contributor

drewfish commented Feb 20, 2013

+1

@caridy caridy added a commit that referenced this pull request Feb 20, 2013

@caridy caridy Merge pull request #978 from caridy/composite-errors
Fixes issue #865. Fixes issue #25: `mojito-composite-addon` error propagation by flagging a child with `propagateFailure` flag.
52c2092

@caridy caridy merged commit 52c2092 into YahooArchive:develop Feb 20, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment