Joe, Thanks for your detailed response and the separate email about moving NiFi forward. My intent in this PR was to decrease memory allocation of a FF and thereby decrease memory that needed to be gc’ed. Given your feedback in this email thread and some other PR feedback I got this morning, I have concluded this PR (https://github.com/apache/nifi/pull/8968) to avoid a single FF allocation is not worth the complication of the API and applying in so many places, so I am going to close the PR in a little while if no one minds.
Thanks, Jim > On Jun 15, 2024, at 12:59 PM, Joe Witt <joe.w...@gmail.com> wrote: > > Jim, > > Missing from this proposal is an explanation of the core 'why' or benefit > this provides. I could see two potential ones based on the comment and > this seems related to recent patterns of code changes in this similar > vein. > > 1. We avoid a temporary java object representing the flow file. > 2. It could reduce lines of code. > > If these are the only potential benefits I don't find them compelling. > The cost in a memory/garbage collection sense I would expect is nearly > immeasurably minor. While it would be great if it meant some considerable > improvement in code required or readability I dont see that in this case. > From the example processor change shown for GenerateTableFetch the code to > me looks less readable and less expressive. > > So then there are other things we'd consider which we cannot see just from > the PR. > > 1. Does this make the system run measurably faster such that it reduces CPU > usage, lowers GC? > 2. Does this drive down memory usage measurably/etc..? > 3. Does this make our codebase in general more maintainable/etc..? > > If you could share more on what led you to feel this is a helpful addition > that information would be great for the community to know in considering > this change. > > As it is right now I am not supportive. However, I want to be clear about > a couple things as while the feedback above is negative relative to the > proposal... > > It is excellent that you're looking into things and considering > opportunities to improve. > > It is excellent that you didn't take it so far but instead had an idea, > wrote it down in such a way others can review, and offer feedback. > > This PR and the related ones prior to it make me think there is value in a > thread that outlines things people can do who are truly motivated to really > help drive things forward. I'll share some of those thoughts in another > thread. > > The above is of course just my view. Will keep an eye for others' inputs. > > Thanks > > > > On Sat, Jun 15, 2024 at 9:24 AM Jim Steinebrey <jrsteineb...@gmail.com> > wrote: > >> Hi, >> >> https://github.com/apache/nifi/pull/8968 >> >> This PR is a proposal to see if the PMC agrees with this API change. >> I have not written any new unit tests for this PR yet until I get buy-in >> on it. >> >> I added two new create methods to the API of ProcessSession to allow the >> option of >> passing in ff attributes to be added to the flow file being created. This >> is not >> a breaking change because the original create methods are still there. >> >> ProcessSession >> FlowFile create(FlowFile parent, final Map<String, String> attributes); >> FlowFile create(final Map<String, String> attributes); >> >> Changing a core API is a very significant change but I hope people will >> see it >> as worthwhile to allow us to to avoid creating an extra FlowFile object in >> many >> places that FlowFiles are created. Not all places set attributes right >> after \ >> FF creation, but very many of them do and could benefit. >> >> There are 150+ places where these new methods can be used and I only >> changed the >> GenerateTableFetch processor to call them so you how the new methods are >> used. >> I expect using these new create methods has the potential to >> avoid noticeable transient memory allocation like the earlier API addition >> of >> SessionContext.isAutoTerminated did. >> >> Also if this PR is approved, myself and others can change processors to >> call these >> new methods in a future PRs (not as part of this PR). >> >> Regards, >> Jim Steinebrey
