I think that's a good approach which actually addresses the underlying issue. Thank you Joe, Mark and all others.
As far as I know the default last resort behaviour of rollback + yield, that a lot processors exhibit, is due to them being based on AbstractProcessor. Does it make sense to incorporate the outlined approach into AbstractProcessor instead of UpdateAttribute? This way, other processors can undergo the same (opt-in) behavioural change without having to re-implement it on a per processor basis. Every processor in question only would need to add the property declared by AbstractProcessor to its list of properties. Processors that do not include the property wouldn't support to configure the behaviour and thus default to the current behaviour instead. I think that would provide a good middle ground, both for users that want a explicit failure relationship and for those that rather want a simpler flow and ensure those kinds of errors won't happen another way. What do you think? Lucas Joe Witt <joe.w...@gmail.com> schrieb am Sa., 10. Feb. 2024, 00:06: > Lots of good commentary and great focus on minimizing impact to the users > while fixing what is admittedly not the most desired behavior for some > cases as it relates to the very popular UpdateAttribute. > > We do not want to enforce that all processors have failure relationships. > Presumably that notion is specific to processors which take an input > flowfile. Even then though we have many examples where having a failure > relationship does not add value. A few examples such as DistributeLoad, > DuplicateFlowFile and others which have concepts such as 'unmatched' etc.. > Certainly most things can and should but we dont need a strict policy > here. We should still let people building processors be thoughtful about > what is best. > > UpdateAttribute specifically... There is history to why it works the way it > does. Things changed and it didn't evolve or couldn't because its use was > so widespread and we didnt want to create too much pain for users. But > because of 2.0 and some improvements like the ability to code up migration > behaviors on a per extension basis we can work our way out of this without > causing pain for the users. > > For NiFi 1.x it should stay as is. > For NiFi 2.x a solution is outlined in NIFI-6344. It reads: > Given the new capabilities for migrating configs in NiFi 2.0 we can fix > this. > > Add a property to UpdateAttribute that is 'Failure Strategy' and the > options are 'rollback' or 'route to failure'. If that property is set with > rollback it behaves like it does now and I recommend that remain the > default. If that property is set to 'route to failure' then we add a > relationship which needs to be set which is of course called 'failure'. For > flows being migrated from a version before this behavior was available to a > version that has this capability we just set the value of this parameter to > our default. > > This lets existing flows migrate over just fine. It lets us give users a > failure path for the cases they want one. It lets us keep the vast majority > of flows and uses of this where failure is not relevant stay clean. And it > handles migration. > > The processor needs to be updated to catch the exceptions and then follow > this logic. Today it just lets it fly to the framework which causes the > processor to yield and penalizes the flowfile for the default time. When > now catching the problem we should just avoid yielding and instead penalize > the specific offending flowfile which lets everything else operate super > fast. > > Thanks to Mark Payne for the chat on this. > > This can be done at any time by anyone that wants to take it on. It is not > a blocker for nifi 2.x. The migration capabilities give us really nice > options for many cases we've hit over the years going forward. > > Thanks > > > On Fri, Feb 9, 2024 at 2:53 PM Adam Taft <a...@adamtaft.com> wrote: > > > I mean, this really speaks to the principal that (in my humble opinion) > All > > Processors Shalt Have Failure Relationships as best practice. > > > > So I think the decision is really, how much pain do we want to impose on > > NiFi 2.0 adopters. UpdateAttribute and RouteOnAttribute are used > literally > > everywhere (especially on large installations), and it would be quite the > > chore(!) to update these if we were to make a non-compatible change for > > 2.0. But 2.0 is also a very logical time/place to make such backwards > > breaking changes to UpdateAttribute, RouteOnAttribute and/or other > > processors that are missing failure relationships. > > > > I just fundamentally have never liked the lack of control for flowfiles > > getting requeued if a processor exception is left uncaught. You are > almost > > always just going to repeat the failure condition, it's rarely useful to > > retry without some sort of deliberate flow manager action. The > enhancements > > to relationships (to enable retries, backoffs, etc.) even reinforces my > > point of view that requeuing is almost always wrong. > > > > So I think the decision here is how much pain is this going to cause for > > NiFi 2.0 adoption? And if it's too much pain, then we should do something > > in a backwards compatible kind of way (like offering a dynamic failure > > relationship for those target processors). And if it's not too much pain, > > just add failure relationships directly to the processors and rip the > > bandaid off. > > > > A solution like a "try/catch" option to the expression language is not > that > > exciting to me. I do believe that adoption will be hindered if we make a > > breaking change because of the high usage of Update and Route processors. > > And so 'm concluding that a dynamic relationship approach, leaving the EL > > as designed, is the best path forward. But I'm hoping there's even a more > > clever solution out there waiting for someone to bring forward. > > > > I do believe whatever we do should be a 2.0 thing. Let's not consider any > > changes on this for 1.x (unless it somehow enables better adoption). > > > > /Adam > > > > On Fri, Feb 9, 2024 at 1:27 PM u...@moosheimer.com <u...@moosheimer.com> > > wrote: > > > > > Yes, that certainly involves a lot of effort. > > > I wonder whether it's a good idea to fix a possible "design flaw" with > a > > > construct that is neither consistent nor easy to handle. > > > > > > I also think the argument that you have to adjust a lot in the flow is > > > questionable. > > > With Release 2.0, so much has to be adapted (cron jobs, database schema > > > controller etc.) that it hardly matters whether you adapt the > > > UpdateAttribute processors or not. > > > > > > The effort to adapt and test everything is already present with the > > > upgrade to 2.0. > > > Doesn't Release 2.0 actually offer a really good opportunity to adapt > > > everything that has been put off for a long time? > > > > > > Who knows when the next opportunity will come? With Release 3.0? Or not > > at > > > all, because NiFi is becoming more and more widespread and the customer > > > lobby is getting bigger and bigger? Cloudera will find it increasingly > > > difficult to pack "large-scale" customizations into a release. > > > > > > Whether we wait 3-4 months or 5-6 months for the 2.0 release doesn't > > > really matter, does it? > > > But what do I know ... the NiFi experts and full-time maintainers will > > > certainly do the right thing. > > > > > > -- Uwe > > > > > > > Am 09.02.2024 um 22:04 schrieb Michael Moser <moser...@gmail.com>: > > > > > > > > These are great ideas! I do love Adam's dynamic relationship idea > > over > > > > creating a failure relationship that is auto-terminated. This would > > make > > > > flow migrations in Registry and NiFi easier. > > > > > > > > After some more pondering, I (slowly) realized that this problem > > affects > > > > more than UpdateAttribute, though. You can easily get expression > > > language > > > > (explang) exceptions anywhere that explang is used. I'm sure all of > us > > > can > > > > create a RouteOnAttribute configuration that causes an explang > > exception > > > > which rolls back the flowfile. If we spend so much effort on a > > solution > > > it > > > > would be a shame for that to only apply to UpdateAttribute. > > > > > > > > For this reason I would favor Matt's idea of a try() or trycatch() > > > explang > > > > method. How it might work isn't intuitive, though. Would we have to > > > make > > > > it aware of a data type to return? RouteOnAttribute expects boolean > > but > > > > UpdateAttribute expects string (or convertible to string). > > > > > > > > I can see why the rollback/admin yield solution has been status quo > for > > > so > > > > long. > > > > > > > > -- Mike > > > > > > > > > > > > > > > > > > > >> On Fri, Feb 9, 2024 at 10:03 AM u...@moosheimer.com < > > u...@moosheimer.com> > > > >> wrote: > > > >> > > > >> 👍 > > > >> --Uwe > > > >> > > > >>>> Am 09.02.2024 um 13:50 schrieb Mike Thomsen < > mikerthom...@gmail.com > > >: > > > >>> > > > >>> How about a third option which is to provide three options: > > > >>> > > > >>> 1) Default - status quo, exceptions cause it to yield > > > >>> 2) Exception = moves forward to success w/ an error attribute, an > > error > > > >> log > > > >>> statement that triggers a bulletin, etc to let data manages know > > what's > > > >>> happening. > > > >>> 3) Exception = moves to a failure relationship that is otherwise > > > >>> autoterminated > > > >>> > > > >>>> On Thu, Feb 8, 2024 at 7:12 PM Matt Burgess <mattyb...@apache.org > > > > > >> wrote: > > > >>>> > > > >>>> Mike's option #2 seems solid but would take a lot of work and > there > > > will > > > >>>> always be inputs we don't account for. I support that work but in > > code > > > >>>> sometimes we just do a "catch(Throwable)" just so it doesn't blow > > up. > > > >> What > > > >>>> about a subjectless "try" or "trycatch" function you can wrap > around > > > >> your > > > >>>> whole expression? If no exception is thrown, the evaluated value > > will > > > be > > > >>>> returned but if one is thrown, you can provide some alternate > value > > > that > > > >>>> you can check downstream. As this is optional it would retain the > > > >> current > > > >>>> behavior unless you use it, and then it takes the place of all > those > > > >>>> ifElse(isXYZValid()) calls we'd need throughout the expression. > > > >>>> > > > >>>> Regards, > > > >>>> Matt > > > >>>> > > > >>>> > > > >>>> On Wed, Feb 7, 2024 at 8:11 PM Phillip Lord < > phillord0...@gmail.com > > > > > > >>>> wrote: > > > >>>> > > > >>>>> IMO... UpdateAttribute has been around since the beginning of > > time, I > > > >>>> can't > > > >>>>> see adding a failure relationship. At the same time I understand > > the > > > >> want > > > >>>>> for such exceptions to be handled more gracefully rather than > > rolling > > > >>>> back > > > >>>>> indefinitely. > > > >>>>> I'd vote in favor of considering Moser's option #2... and being > > able > > > to > > > >>>>> implement an "if this then that" logic within your flow. > > > >>>>> > > > >>>>> Also just thinking... for every UA failure you have to consider a > > > good > > > >>>>> failure-management strategy, which MIGHT add a lot of noise to > the > > > >> flow. > > > >>>>> Something that might otherwise easily be identified in a > downstream > > > >>>>> component and/or database/etc. > > > >>>>> > > > >>>>> My 2 cents ** > > > >>>>> Phil > > > >>>>> > > > >>>>> > > > >>>>> > > > >>>>> > > > >>>>> > > > >>>>>> On Wed, Feb 7, 2024 at 5:18 PM Adam Taft <a...@adamtaft.com> > > wrote: > > > >>>>> > > > >>>>>> Or better, the failure relationship just doesn't even exist > until > > > the > > > >>>>>> property "Has Failure Relationship" is set to True. This > involves > > > >>>>> updating > > > >>>>>> UpdateAttribute to have dynamic relationships (the failure > > > >>>> relationships > > > >>>>>> appearing on true), which isn't hard to do in processor code. > > > >>>>>> > > > >>>>>> This has the advantage of being backwards compatible for > existing > > > >> users > > > >>>>> and > > > >>>>>> allows the failure relationship to exist for new configurations. > > > >>>>> Obviously > > > >>>>>> the processor would need an update to catch Expression Language > > > >>>>> exceptions > > > >>>>>> and then route conditionally to failure. > > > >>>>>> > > > >>>>>> Just thinking out loud. > > > >>>>>> /Adam > > > >>>>>> > > > >>>>>> > > > >>>>>> > > > >>>>>> On Wed, Feb 7, 2024 at 1:48 PM u...@moosheimer.com < > > > u...@moosheimer.com > > > >>> > > > >>>>>> wrote: > > > >>>>>> > > > >>>>>>> Hi Mike, > > > >>>>>>> > > > >>>>>>> How about the option of introducing a new property that decides > > > >>>> whether > > > >>>>>> to > > > >>>>>>> route to the 'failure' relationship in the event of an error? > > > >>>>>>> If this property is set to false, then the 'failure' > relationship > > > is > > > >>>>>>> automatically set to 'terminate' (since nothing is routed there > > > >>>>> anyway). > > > >>>>>>> > > > >>>>>>> Then everyone can decide whether and where they want to use > this > > > new > > > >>>>>>> feature or not. > > > >>>>>>> All other options would still be possible with such a solution. > > > >>>>>>> > > > >>>>>>> -- Uwe > > > >>>>>>> > > > >>>>>>>> Am 07.02.2024 um 22:15 schrieb Michael Moser < > > moser...@gmail.com > > > >: > > > >>>>>>>> > > > >>>>>>>> Hi Dan, > > > >>>>>>>> > > > >>>>>>>> This has been discussed in the past, as you found with those > two > > > >>>> Jira > > > >>>>>>>> tickets. Personally, I'm still not sure whether a new failure > > > >>>>>>> relationship > > > >>>>>>>> on UpdateAttribute in 2.0 is a good approach. I have heard > from > > > >>>> some > > > >>>>>>>> dataflow managers that would not want to go through their > entire > > > >>>>> graph > > > >>>>>>> when > > > >>>>>>>> upgrading to 2.0 and update every UpdateAttribute > configuration. > > > >>>>>>>> > > > >>>>>>>> I have heard some alternatives to a 'failure' relationship > that > > I > > > >>>>> would > > > >>>>>>>> like to share as options. > > > >>>>>>>> > > > >>>>>>>> 1) Add a new property to UpdateAttribute that controls > whether a > > > >>>>>> flowfile > > > >>>>>>>> that causes an expression language exception either yields and > > > >>>> rolls > > > >>>>>>> back, > > > >>>>>>>> or silently fails to update the attribute and sends the > flowfile > > > to > > > >>>>>>>> success. I personally don't like this, because the use case > for > > > >>>>>> "silent > > > >>>>>>>> failure" seems really like a rarely needed edge case. > > > >>>>>>>> > > > >>>>>>>> 2) Identify all expression language methods that can throw an > > > >>>>> exception > > > >>>>>>> and > > > >>>>>>>> document that fact in the Expression Language Guide (some > > methods > > > >>>>>> already > > > >>>>>>>> mention they can throw an "exception bulletin"). Then > implement > > > >>>> new > > > >>>>>>>> expression methods to check if an expression could fail, and > use > > > >>>> that > > > >>>>>> in > > > >>>>>>>> UpdateAttribute advanced rules. For example, if the format() > > and > > > >>>>>>>> formatInstant() methods can fail on a negative number, we > > create a > > > >>>>> new > > > >>>>>>>> method such as isValidMilliseconds(). This already exists for > > > some > > > >>>>>>> cases, > > > >>>>>>>> such as isJson() which can do a quick check of some value > before > > > >>>>>> calling > > > >>>>>>>> jsonPathDelete() on it. > > > >>>>>>>> > > > >>>>>>>> I'm curious to hear more thoughts on this. > > > >>>>>>>> > > > >>>>>>>> -- Mike > > > >>>>>>>> > > > >>>>>>>> > > > >>>>>>>> > > > >>>>>>>>> On Wed, Jan 31, 2024 at 11:02 AM Dan S <dsti...@gmail.com> > > > wrote: > > > >>>>>>>>> > > > >>>>>>>>> My team is requesting a failure relationship for > > UpdateAttribute > > > >>>> as > > > >>>>>>> seen in > > > >>>>>>>>> NIFI-5448 <https://issues.apache.org/jira/browse/NIFI-5448> > > and > > > >>>>>>> NIFI-6344 > > > >>>>>>>>> <https://issues.apache.org/jira/browse/NIFI-6344> as we are > > > >>>>>>>>> experiencing the same problem where a NIFI Expression > Language > > is > > > >>>>>>> throwing > > > >>>>>>>>> an exception. In the PR for NIFI-5448 it was mentioned this > > > >>>> feature > > > >>>>>>> would > > > >>>>>>>>> have to wait until NIFI 2.0.0. I wanted to know if there is > any > > > >>>>> active > > > >>>>>>> work > > > >>>>>>>>> regarding this and whether eventually there will be a failure > > > >>>>>>> relationship > > > >>>>>>>>> added to UpdateAttribute? > > > >>>>>>>>> > > > >>>>>>> > > > >>>>>>> > > > >>>>>> > > > >>>>> > > > >>>> > > > >> > > > > > > > > >
