Support Passing in FF Attribute to Flow File create methods

[Jim Steinebrey <jr...@gmail.com>]

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

Re: Support Passing in FF Attribute to Flow File create methods

[Joe Witt <jo...@gmail.com>]

Jim

Ok - understood.  The trade-off that appears to be getting made to my eyes
looks like less expressive code.  If it were substantially valuable in
terms of cpu, heap, io then I think it makes the case more compelling.
I've not validated this with true testing but my intuition for this example
is that it would be barely measurable.  If you have different benchmark
results to share that would be interesting to learn more about.

Thanks

On Mon, Jun 17, 2024 at 8:23 AM Jim Steinebrey <jr...@gmail.com>
wrote:

> 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 <jo...@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 <jr...@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
>
>

Re: Support Passing in FF Attribute to Flow File create methods

[Jim Steinebrey <jr...@gmail.com>]

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 <jo...@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 <jr...@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

Re: Support Passing in FF Attribute to Flow File create methods

[Joe Witt <jo...@gmail.com>]

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 <jr...@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