[GitHub] [nifi] simonbence commented on a change in pull request #5437: NIFI-9265 Fixing path handling for HDFS processors when there are multiplied separators in the path

[GitBox <gi...@apache.org>]

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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org