Evergreen

Bug #1749475
Comment #18

Comment 18 for bug 1749475

Revision history for this message

Dan Wells (dbw2) wrote on 2019-02-27:

#18

Thanks, Mike. It helps to know which pieces are fundamental.

I am really fond of the concept of Action Trigger as a sort of generic Evergreen output processor, and I am glad to see it getting some attention. At this stage I am trying to firm up my mental model to better offer reasonable insights on the design.

For example, I ran into an issue testing the code with the idea of the 'preview' flag being part of the $user_data blob. The branch is trying to have the SendEmail reactor do double duty, generating both the previews and the real emails. This makes sense. The issue, though, is that SendEmail is used in quite a few events, and sometimes grouped, sometimes not. When used in a grouped event, $user_data will (I think) always be an array ref, but when used in a non-grouped event, it will be whatever the event firer wants it to be (typically a hash).

This poses a few problems in trying to use $user_data as a conduit to communicate with the reactor. First, we obviously need to cover more cases just to make the code function, that is, treat $user_data as possibly empty, possibly an array, possibly a hash, and seek out the 'preview' flag in each way. This is perhaps a little ugly, but doable. Second, though, it becomes clear that $user_data is not really a reliable way to affect the behavior of a reactor due to grouping. There is nothing (I think) to prevent events being grouped with (in this case) conflicting 'preview' flags. Perhaps the situation is rare and contrived, but it would still be better to have a fully determinate system which does not rely on the order of possibly unrelated events.

A quick solution would be to give up on the flag approach, and create a new Reactor (e.g. PreviewEmail) which is called in the right cases. This isn't too bad of an option, given that the SendEmail reactor code is so simple. However, we end up with partially duplicated code at the Reactor, and if we still care about fully supporting the old event definition, we also get an almost wholly duplicated event definition. This works fine, is sound, but not ideal.

From this discussion, we can imagine a few missing AT features which would be useful. First, it would be good to have some way for an event firer to communicate reliably with the Reactor, ideally in a way which is also surfaced at the grouping mechanism. Second, it seems like a general benefit could be had by making AT templates sharable across different events in some fashion. (This could also potentially apply generally to this feature quite nicely, of course.)

So, what say others? Is it worth pausing and shaking things up a little more? Does anything stand out as a natural direction to meet some of these weaknesses while also adding the necessary strengths? I know there have been other structural issues with AT over the years, so perhaps someone keenly affected by those can help to further illustrate the big picture.

I think it is worth raising this at the next developer meeting to see if there is broad interest in hashing something out. If not, it might be best to stick with a more isolated approach. In the meantime, any and all proposals or thoughts are welcome here!

Thanks, Mike.  It helps to know which pieces are fundamental.

I am really fond of the concept of Action Trigger as a sort of generic Evergreen output processor, and I am glad to see it getting some attention.  At this stage I am trying to firm up my mental model to better offer reasonable insights on the design.

For example, I ran into an issue testing the code with the idea of the 'preview' flag being part of the $user_data blob.  The branch is trying to have the SendEmail reactor do double duty, generating both the previews and the real emails.  This makes sense.  The issue, though, is that SendEmail is used in quite a few events, and sometimes grouped, sometimes not.  When used in a grouped event, $user_data will (I think) always be an array ref, but when used in a non-grouped event, it will be whatever the event firer wants it to be (typically a hash).

This poses a few problems in trying to use $user_data as a conduit to communicate with the reactor.  First, we obviously need to cover more cases just to make the code function, that is, treat $user_data as possibly empty, possibly an array, possibly a hash, and seek out the 'preview' flag in each way.  This is perhaps a little ugly, but doable.  Second, though, it becomes clear that $user_data is not really a reliable way to affect the behavior of a reactor due to grouping.  There is nothing (I think) to prevent events being grouped with (in this case) conflicting 'preview' flags.  Perhaps the situation is rare and contrived, but it would still be better to have a fully determinate system which does not rely on the order of possibly unrelated events.

A quick solution would be to give up on the flag approach, and create a new Reactor (e.g. PreviewEmail) which is called in the right cases.  This isn't too bad of an option, given that the SendEmail reactor code is so simple.  However, we end up with partially duplicated code at the Reactor, and if we still care about fully supporting the old event definition, we also get an almost wholly duplicated event definition.  This works fine, is sound, but not ideal.

From this discussion, we can imagine a few missing AT features which would be useful.  First, it would be good to have some way for an event firer to communicate reliably with the Reactor, ideally in a way which is also surfaced at the grouping mechanism.  Second, it seems like a general benefit could be had by making AT templates sharable across different events in some fashion.  (This could also potentially apply generally to this feature quite nicely, of course.)

So, what say others?  Is it worth pausing and shaking things up a little more?  Does anything stand out as a natural direction to meet some of these weaknesses while also adding the necessary strengths?  I know there have been other structural issues with AT over the years, so perhaps someone keenly affected by those can help to further illustrate the big picture.

I think it is worth raising this at the next developer meeting to see if there is broad interest in hashing something out.  If not, it might be best to stick with a more isolated approach.  In the meantime, any and all proposals or thoughts are welcome here!