EndzeitBegins commented on PR #8620: URL: https://github.com/apache/nifi/pull/8620#issuecomment-2048209586
Thank you for taking a look @exceptionfactory. I know this is a large change in terms of lines changes, so I really appreciate that. While going through the interface declaration, I noticed that those functions are just a shorthand overload of their respective functions with more parameters. The behaviour of calling `commitAsync()` is defined equal to calling `commitAsync(Runnable onSuccess, Consumer<Throwable> onFailure)` with both parameters set to `null`, as both parameters are optional. In fact, the same applies to the "sibling" method `commitAsync(Runnable onSuccess)` which has an existing default implementation of `commitAsync(onSuccess, null);`. I figured adding a default for `commitAsync()` would further align both function overloads. A similar reasoning can be applied to `merge(Collection<FlowFile> sources, FlowFile destination)` who's behavior is defined akin to that of `merge(Collection<FlowFile> sources, FlowFile destination, byte[] header, byte[] footer, byte[] demarcator)` when passing `null` for all of `header`, `footer`, and `demarcator`. However, this time there is no other sibling overload with a default. As adding a default implementation is not a breaking change nor alters the behavior for any existing implementation I figured that's a sensible addition. However, you're right that it's out of scope of the original issue description. With this background, what are your thoughts on this? Should I revert the addition of those default implementations or do you follow the reasoning above? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
