exceptionfactory commented on code in PR #6159:
URL: https://github.com/apache/nifi/pull/6159#discussion_r941502092
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java:
##########
@@ -20,7 +20,9 @@
import com.azure.storage.file.datalake.DataLakeFileClient;
import com.azure.storage.file.datalake.DataLakeFileSystemClient;
import com.azure.storage.file.datalake.DataLakeServiceClient;
+import com.azure.storage.file.datalake.models.DataLakeRequestConditions;
import com.azure.storage.file.datalake.models.DataLakeStorageException;
+import com.google.common.annotations.VisibleForTesting;
Review Comment:
Use of Google Guava annotations should be avoided.
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java:
##########
@@ -190,4 +218,34 @@ static void uploadContent(DataLakeFileClient fileClient,
InputStream in, long le
fileClient.flush(length);
}
+
+ @VisibleForTesting
+ DataLakeFileClient renameFile(final String fileName, final String
directoryPath, final DataLakeFileClient fileClient, final boolean overwrite) {
+ try {
+ final DataLakeRequestConditions destinationCondition = new
DataLakeRequestConditions();
+ if (!overwrite) {
+ destinationCondition.setIfNoneMatch("*");
+ }
+ final String destinationPath = createPath(directoryPath, fileName);
+ return fileClient.renameWithResponse(null, destinationPath, null,
destinationCondition, null, null).getValue();
+ } catch (DataLakeStorageException dataLakeStorageException) {
+ getLogger().error("Error while renaming temp file " +
fileClient.getFileName() + " on Azure Data Lake Storage",
dataLakeStorageException);
+ removeTempFile(fileClient);
+ throw dataLakeStorageException;
+ }
+ }
+
+ private String createPath(final String baseDirectory, final String path) {
+ return StringUtils.isNotBlank(baseDirectory)
+ ? baseDirectory + "/" + path
+ : path;
+ }
+
+ private void removeTempFile(final DataLakeFileClient fileClient) {
+ try {
+ fileClient.delete();
+ } catch (Exception e) {
+ getLogger().error("Error while removing temp file " +
fileClient.getFileName() + " on Azure Data Lake Storage", e);
Review Comment:
Log messages should use placeholders instead of string concatenation.
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java:
##########
@@ -129,13 +129,24 @@ public class ListAzureDataLakeStorage extends
AbstractListAzureProcessor<ADLSFil
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor SHOW_TEMPORARY_FILES = new
PropertyDescriptor.Builder()
+ .name("show-temporary-files")
+ .displayName("Show temporary files")
+ .description("Whether temporary files, created during nifi upload
process should be shown." +
+ "These files are incomplete and removed after a completed
upload process.")
Review Comment:
Recommend the following adjustments:
```suggestion
.description("Whether to include temporary files when listing
the contents of configured directory paths.")
```
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java:
##########
@@ -83,11 +89,23 @@ public class PutAzureDataLakeStorage extends
AbstractAzureDataLakeStorageProcess
.allowableValues(FAIL_RESOLUTION, REPLACE_RESOLUTION,
IGNORE_RESOLUTION)
.build();
+ public static final PropertyDescriptor TEMP_FILE_DIRECTORY_PATH = new
PropertyDescriptor.Builder()
+ .name("temp-file-directory-path")
+ .displayName("Temp File Directory Path")
Review Comment:
The combination of `File`, `Directory`, and `Path` is a bit hard to
understand. It seems like this should be named something like `Temporary
Directory`, or perhaps `Base Temporary Path`. If I understand the
implementation correctly, this path is the prefix for the static hard-coded
`TEMP_FILE_DIRECTORY`, is that correct? It would be helpful to clarify the
wording.
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java:
##########
@@ -129,13 +129,24 @@ public class ListAzureDataLakeStorage extends
AbstractListAzureProcessor<ADLSFil
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor SHOW_TEMPORARY_FILES = new
PropertyDescriptor.Builder()
+ .name("show-temporary-files")
+ .displayName("Show temporary files")
Review Comment:
The word `Show` does not seem to fit well. What about `Include Temporary
Files`?
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/test/java/org/apache/nifi/processors/azure/storage/ITListAzureDataLakeStorage.java:
##########
@@ -57,6 +58,10 @@ public void setUp() {
uploadFile(testFile1);
testFiles.put(testFile1.getFilePath(), testFile1);
+ TestFile testTempFile1 = new TestFile("_$azuretempdirectory$",
"1234file1");
Review Comment:
All references to `$azuretempdirectory$` should be replaced with
String.format() and use of the shared static value for easier maintenance.
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java:
##########
@@ -190,4 +218,34 @@ static void uploadContent(DataLakeFileClient fileClient,
InputStream in, long le
fileClient.flush(length);
}
+
+ @VisibleForTesting
+ DataLakeFileClient renameFile(final String fileName, final String
directoryPath, final DataLakeFileClient fileClient, final boolean overwrite) {
+ try {
+ final DataLakeRequestConditions destinationCondition = new
DataLakeRequestConditions();
+ if (!overwrite) {
+ destinationCondition.setIfNoneMatch("*");
+ }
+ final String destinationPath = createPath(directoryPath, fileName);
+ return fileClient.renameWithResponse(null, destinationPath, null,
destinationCondition, null, null).getValue();
+ } catch (DataLakeStorageException dataLakeStorageException) {
+ getLogger().error("Error while renaming temp file " +
fileClient.getFileName() + " on Azure Data Lake Storage",
dataLakeStorageException);
Review Comment:
It seems unnecessary to indicate `Azure Data Lake Storage` given the name of
the Processor. Placeholders should be used instead of string concatenation.
```suggestion
getLogger().error("Renaming File [{}] failed",
fileClient.getFileName(), dataLakeStorageException);
```
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java:
##########
@@ -261,10 +272,12 @@ private List<ADLSFileInfo> performListing(final
ProcessContext context, final Lo
options.setRecursive(recurseSubdirectories);
final Pattern baseDirectoryPattern = Pattern.compile("^" +
baseDirectory + "/?");
+ final boolean includeTempFiles =
context.getProperty(SHOW_TEMPORARY_FILES).asBoolean();
final long minimumTimestamp = minTimestamp == null ? 0 :
minTimestamp;
final List<ADLSFileInfo> listing =
fileSystemClient.listPaths(options, null).stream()
.filter(pathItem -> !pathItem.isDirectory())
+ .filter(pathItem -> includeTempFiles ||
!pathItem.getName().contains("_$azuretempdirectory$"))
Review Comment:
The `contains()` call should reference the TEMP_FILE_DIRECTORY variable.
--
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
