simonbence commented on a change in pull request #5437:
URL: https://github.com/apache/nifi/pull/5437#discussion_r724967483
##########
File path:
nifi-nar-bundles/nifi-extension-utils/nifi-hadoop-utils/src/main/java/org/apache/nifi/processors/hadoop/AbstractHadoopProcessor.java
##########
@@ -674,39 +681,33 @@ protected Path getNormalizedPath(ProcessContext context,
PropertyDescriptor prop
}
protected Path getNormalizedPath(final String rawPath) {
Review comment:
I disagree with this approach. `getNormalizedPath` with the `String`
argument is indeed being called only at the `FetchHDFS` currently, but calling
the method with also containing the `Optional` would be strange from the
perspective of the caller: `FetchHDFS` needs to know only that he needs to
provide a path, nothing more. Other users might call the method with
`ProcessContext` and so, but it is more like for the convinience of the caller.
In this sense, consider `AbstractHadoopProcessor` as an API for the children
classes. The "additional" `Optional` parameter I consider an implementation
detail, which should not be published towards possible callers. It intends to
help the method to provide as much information during the `warn` as possible.
Removing it would cost us some beneficial information to share when a possible
issue arises. But to be honest, this is not even a new behaviour I just merged
the two`getNormalizedPath` implementations, in order to reduce duplicated code
(which with the new `replace` would be even more)
--
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
