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
