You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/05/25 12:16:07 UTC
[GitHub] [nifi] timeabarna opened a new pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
timeabarna opened a new pull request #5098:
URL: https://github.com/apache/nifi/pull/5098
Modify PutAzureDataLakeStorage processor to use temp file
https://issues.apache.org/jira/browse/NIFI-8248
#### Description of PR
Enables the usage of a temporary file instead of immediate overwrite
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [ ] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] jfrazee commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r675242859
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -114,30 +119,23 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final DataLakeServiceClient storageClient = getStorageClient(context, flowFile);
final DataLakeFileSystemClient fileSystemClient = storageClient.getFileSystemClient(fileSystem);
final DataLakeDirectoryClient directoryClient = fileSystemClient.getDirectoryClient(directory);
- final DataLakeFileClient fileClient;
+ DataLakeFileClient fileClient;
final String conflictResolution = context.getProperty(CONFLICT_RESOLUTION).getValue();
boolean overwrite = conflictResolution.equals(REPLACE_RESOLUTION);
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
try {
- fileClient = directoryClient.createFile(fileName, overwrite);
-
- final long length = flowFile.getSize();
- if (length > 0) {
- try (final InputStream rawIn = session.read(flowFile); final BufferedInputStream bufferedIn = new BufferedInputStream(rawIn)) {
- uploadContent(fileClient, bufferedIn, length);
- } catch (Exception e) {
- removeTempFile(fileClient);
- throw e;
- }
- }
+ fileClient = directoryClient.createFile(tempFilePrefix + fileName, true);
Review comment:
Updating my comment here. A random prefix will be preferable to a random suffix, and given that conflicting names can lead to problems we probably shouldn't give absolute control to the user. We should do something like the following (which is based on the approach in Hadoop):
```java
// The temp files should be tucked away in a directory because of the impact
// they could have on file listings. Hadoop uses "_$azuretmpfolder$" statically
// but I think it could be useful to be configurable. The default should be
// prefaced with . or _ though.
final String tempFileFolder = context.getProperty(TEMP_FILE_FOLDER).getValue();
final String tempFilePrefix = tempFileFolder + "/" + UUID.randomUUID().toString();
final String tempFilePath = tempFilePrefix + fileName;
```
--
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
[GitHub] [nifi] jfrazee commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r675229270
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -114,30 +119,23 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final DataLakeServiceClient storageClient = getStorageClient(context, flowFile);
final DataLakeFileSystemClient fileSystemClient = storageClient.getFileSystemClient(fileSystem);
final DataLakeDirectoryClient directoryClient = fileSystemClient.getDirectoryClient(directory);
- final DataLakeFileClient fileClient;
+ DataLakeFileClient fileClient;
final String conflictResolution = context.getProperty(CONFLICT_RESOLUTION).getValue();
boolean overwrite = conflictResolution.equals(REPLACE_RESOLUTION);
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
try {
- fileClient = directoryClient.createFile(fileName, overwrite);
-
- final long length = flowFile.getSize();
- if (length > 0) {
- try (final InputStream rawIn = session.read(flowFile); final BufferedInputStream bufferedIn = new BufferedInputStream(rawIn)) {
- uploadContent(fileClient, bufferedIn, length);
- } catch (Exception e) {
- removeTempFile(fileClient);
- throw e;
- }
- }
+ fileClient = directoryClient.createFile(tempFilePrefix + fileName, true);
Review comment:
Since tempFilePrefix is more or less static, this opens up the possibility for some weird behavior if two processors, nodes, threads, clusters try to write a file with the same name.
Two solutions come to mind:
1. Add a random suffix
2. Make temp file prefix a processor property and allow for EL so the user can account for that if needed
Also, because of this, I think it'd be good to open a follow-on issue to add some lease handling. I wouldn't want that to hold back this change though.
--
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
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r673960599
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -129,6 +131,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
+ .name("azure-temp-file-prefix")
+ .displayName("Temp File Prefix for Azure")
Review comment:
@exceptionfactory @jfrazee I've refactored the code using built-in prefix and added boolean property, thanks in advance for checking this PR again
--
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
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r668069231
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -129,6 +131,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
+ .name("azure-temp-file-prefix")
+ .displayName("Temp File Prefix for Azure")
Review comment:
@timeabarna Changing the approach to use a Boolean property with hard-coded temporary prefixes seems to be a better way to go if @jfrazee agrees that temporary files always follow the same naming convention.
--
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
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r655949768
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -129,6 +131,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
+ .name("azure-temp-file-prefix")
+ .displayName("Temp File Prefix for Azure")
Review comment:
@exceptionfactory and @jfrazee Thank you very much for your help. I've implemented the requested changes however during the unit tests it has brought up another perspective I would like to share.
Combining the file filter with the temp prefix would mean we would like to have a regex which would like to include only the specified files and exclude some specified files, I think this is very confusing for the user and may not be a best practice.
As we need to provide the option for prefix override it means that sometimes separate Regex expressions must be concatenated. I have no knowledge regarding our Users' Regex experience level however combining two separate, conflicting scoped (for example a negative lookahead and a wildcard word match) Regex Expression can be challenging even for Developers.
My last concern is what if the Processor already has a path filter set. It would override the new default filter and temp files started appear in the listing which may impact the existing flows.
So for the best User experience I would like to recommend to keep the prefix property which serves as the default value and the override possibility however change it to accept a Regex expression. This way our Users have the control to change only the filter they have to in a straightforward and convenient way.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r646977533
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/pom.xml
##########
@@ -78,6 +78,11 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>com.azure</groupId>
+ <artifactId>azure-core-http-netty</artifactId>
+ <version>1.7.0</version>
Review comment:
Is there a reason for using this version as opposed to the more recent version `1.9.2`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] jfrazee commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r657454369
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -84,12 +89,24 @@
.allowableValues(FAIL_RESOLUTION, REPLACE_RESOLUTION, IGNORE_RESOLUTION)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
Review comment:
Unlike the hidden file filter, I think this comment still applies. I need to verify, but I don't think the ABFS driver will ignore anything in a listing without a prefix so it'll be useful to specify a location outside the target directory.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r663661098
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -129,6 +131,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
+ .name("azure-temp-file-prefix")
+ .displayName("Temp File Prefix for Azure")
Review comment:
@exceptionfactory @jfrazee So if I understand you correctly, the prefix property should be removed from both put and list processors, having a built-in prefix instead. List processor should have a boolean property whether temp files should be included in the listing. This way there is no option to change the temp file name and location though. Is my understanding correct?
--
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
[GitHub] [nifi] jfrazee commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r675229270
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -114,30 +119,23 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final DataLakeServiceClient storageClient = getStorageClient(context, flowFile);
final DataLakeFileSystemClient fileSystemClient = storageClient.getFileSystemClient(fileSystem);
final DataLakeDirectoryClient directoryClient = fileSystemClient.getDirectoryClient(directory);
- final DataLakeFileClient fileClient;
+ DataLakeFileClient fileClient;
final String conflictResolution = context.getProperty(CONFLICT_RESOLUTION).getValue();
boolean overwrite = conflictResolution.equals(REPLACE_RESOLUTION);
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
try {
- fileClient = directoryClient.createFile(fileName, overwrite);
-
- final long length = flowFile.getSize();
- if (length > 0) {
- try (final InputStream rawIn = session.read(flowFile); final BufferedInputStream bufferedIn = new BufferedInputStream(rawIn)) {
- uploadContent(fileClient, bufferedIn, length);
- } catch (Exception e) {
- removeTempFile(fileClient);
- throw e;
- }
- }
+ fileClient = directoryClient.createFile(tempFilePrefix + fileName, true);
Review comment:
Since tempFilePrefix is more or less static, this opens up the possibility for some weird behavior if two processors, nodes, threads, clusters try to write a file with the same name.
Two solutions come to mind:
1. ~Add a random suffix~
2. Make temp file prefix a processor property and allow for EL so the user can account for that if needed
Also, because of this, I think it'd be good to open a follow-on issue to add some lease handling. I wouldn't want that to hold back this change though.
--
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
[GitHub] [nifi] jfrazee commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r675242859
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -114,30 +119,23 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final DataLakeServiceClient storageClient = getStorageClient(context, flowFile);
final DataLakeFileSystemClient fileSystemClient = storageClient.getFileSystemClient(fileSystem);
final DataLakeDirectoryClient directoryClient = fileSystemClient.getDirectoryClient(directory);
- final DataLakeFileClient fileClient;
+ DataLakeFileClient fileClient;
final String conflictResolution = context.getProperty(CONFLICT_RESOLUTION).getValue();
boolean overwrite = conflictResolution.equals(REPLACE_RESOLUTION);
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
try {
- fileClient = directoryClient.createFile(fileName, overwrite);
-
- final long length = flowFile.getSize();
- if (length > 0) {
- try (final InputStream rawIn = session.read(flowFile); final BufferedInputStream bufferedIn = new BufferedInputStream(rawIn)) {
- uploadContent(fileClient, bufferedIn, length);
- } catch (Exception e) {
- removeTempFile(fileClient);
- throw e;
- }
- }
+ fileClient = directoryClient.createFile(tempFilePrefix + fileName, true);
Review comment:
Updating my comment here. A random prefix will be preferable to a random suffix, and given that conflicting names can lead to problems we probably shouldn't give absolute control to the user. We should do something like the following (which is loosely based on the approach in Hadoop):
```java
// The temp files should be tucked away in a directory because of the impact
// they could have on file listings. Hadoop uses "_$azuretmpfolder$" statically
// but I think it could be useful to be configurable. The default should be
// prefaced with . or _ though.
final String tempFileFolder = context.getProperty(TEMP_FILE_FOLDER).getValue();
final String tempFilePrefix = tempFileFolder + "/" + UUID.randomUUID().toString();
final String tempFilePath = tempFilePrefix + fileName;
```
--
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
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r667630683
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -129,6 +131,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
+ .name("azure-temp-file-prefix")
+ .displayName("Temp File Prefix for Azure")
Review comment:
@exceptionfactory @jfrazee Can you please confirm my understanding, so I can start modifying the code accordingly? Thanks in advance
--
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
[GitHub] [nifi] timeabarna commented on pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#issuecomment-847820183
@jfrazee Hello Joey, file upload with renaming has been created. When you have some time can you please check? Thanks in advance for your help.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r655949768
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -129,6 +131,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
+ .name("azure-temp-file-prefix")
+ .displayName("Temp File Prefix for Azure")
Review comment:
@exceptionfactory and @jfrazee Thank you very much for your help. I've implemented the requested changes however during the unit tests it has brought up another perspective I would like to share.
Combining the file filter with the temp prefix would mean we would like to have a regex which would like to include only the specified files and exclude some specified files, I think this is very confusing for the user and may not be a best practice.
As we need to provide the option for prefix override it means that sometimes separate Regex expressions must be concatenated. I have no knowledge regarding our Users' Regex experience level however combining two separate, conflicting scoped (for example a negative lookahead and a wildcard word match) Regex Expression can be challenging even for Developers.
My last concern is what if the Processor already has a path filter set. It would override the new default filter and temp files started appear in the listing which may impact the existing flows.
So for the best User experience I would like to recommend to keep the prefix property which serves as the default value and the override possibility however change it to accept a Regex expression. This way our Users have the control to change only the filter they have to in a straightforward and convenient way.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] jfrazee commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r677512004
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -226,9 +239,12 @@ protected String getPath(ProcessContext context) {
options.setRecursive(recurseSubdirectories);
Pattern baseDirectoryPattern = Pattern.compile("^" + baseDirectory + "/?");
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
Review comment:
The renaming should still be atomic.
Are you asking if we should support a temporary name prefix and a temporary directory? I'd go with one or another and not both. I prefer a location because in a high volume ingest scenario there could be a lot of temporary files.
I'd have to look closer at PutHDFS. I don't think that's ideal behavior though if there's a possibility of multiple writers.
--
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
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r675387450
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -226,9 +239,12 @@ protected String getPath(ProcessContext context) {
options.setRecursive(recurseSubdirectories);
Pattern baseDirectoryPattern = Pattern.compile("^" + baseDirectory + "/?");
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
Review comment:
@jfrazee @exceptionfactory Let me summarise my understanding on your recommendations as it seems that the final solution would be a mix of the old things :)
List processor:
- Modify listing to hide any file and folder which names starting with either _ or .
- Keep the boolean property which is turning on the filtering of the above
Put processor:
- Create a property for tempfolder .
- Modify rename to put the temp file in the provided tempfolder under a generated UUID with the existing file name
My questions:
1. As renaming will be location related not name related, is this still atomic?
2. Should tempfolder property used as name only, location only or both? This requires different validation.
3. What happens if a Customer would like to use Put processor as in PutHDFS, which is putting the temp file in the same directory as the file is located, using . + filename as naming? Should we allow this?
--
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
[GitHub] [nifi] jfrazee commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r657442804
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -129,6 +131,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
+ .name("azure-temp-file-prefix")
+ .displayName("Temp File Prefix for Azure")
Review comment:
> @jfrazee may have a better understanding of the runtime environment, but do temporary files always follow a standard pattern? If so, perhaps having a Boolean property for filtering temporary files would be easier to understand and provide the desired behavior.
I may not have been thinking clearly because I was thinking of files like _SUCCESS and _temporary which are used more like dotted files in HDFS. I think it'll be unlikely to see those though unless someone just did a bulk transfer from HDFS to ADLS so it's probably a non-concern unless someone says they need it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r673961741
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/pom.xml
##########
@@ -78,6 +78,11 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>com.azure</groupId>
+ <artifactId>azure-core-http-netty</artifactId>
+ <version>1.7.0</version>
Review comment:
Dependency removed, raies azure.core and cosmos versions instead
--
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
[GitHub] [nifi] jfrazee commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r675229270
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -114,30 +119,23 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final DataLakeServiceClient storageClient = getStorageClient(context, flowFile);
final DataLakeFileSystemClient fileSystemClient = storageClient.getFileSystemClient(fileSystem);
final DataLakeDirectoryClient directoryClient = fileSystemClient.getDirectoryClient(directory);
- final DataLakeFileClient fileClient;
+ DataLakeFileClient fileClient;
final String conflictResolution = context.getProperty(CONFLICT_RESOLUTION).getValue();
boolean overwrite = conflictResolution.equals(REPLACE_RESOLUTION);
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
try {
- fileClient = directoryClient.createFile(fileName, overwrite);
-
- final long length = flowFile.getSize();
- if (length > 0) {
- try (final InputStream rawIn = session.read(flowFile); final BufferedInputStream bufferedIn = new BufferedInputStream(rawIn)) {
- uploadContent(fileClient, bufferedIn, length);
- } catch (Exception e) {
- removeTempFile(fileClient);
- throw e;
- }
- }
+ fileClient = directoryClient.createFile(tempFilePrefix + fileName, true);
Review comment:
Since tempFilePrefix is more or less static, this opens up the possibility for some weird behavior if two processors, nodes, threads, clusters try to write a file with the same name.
Two solutions come to mind:
1. ~Add a random suffix~
2. ~Make temp file prefix a processor property and allow for EL so the user can account for that if needed~
Also, because of this, I think it'd be good to open a follow-on issue to add some lease handling. I wouldn't want that to hold back this change though.
--
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
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r657424889
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -129,6 +131,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
+ .name("azure-temp-file-prefix")
+ .displayName("Temp File Prefix for Azure")
Review comment:
Thanks for following up and noting those concerns @timeabarna, I can see the challenge of defining complex regular expressions to filter temporary files and other files. Looking at other processors, `ListSFTP` includes a Boolean property for `Ignored Dotted Files` as well as properties for filtering files and paths using regular expressions. It seems like the question is whether it is necessary to support filtering both temporary files and other files. On the one hand, having one filter property avoids confusion from potentially conflicting patterns, but on the other hand, creating the right expression could be challenging.
@jfrazee may have a better understanding of the runtime environment, but do temporary files always follow a standard pattern? If so, perhaps having a Boolean property for filtering temporary files would be easier to understand and provide the desired behavior.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] github-actions[bot] commented on pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#issuecomment-978517492
We're marking this PR as stale due to lack of updates in the past few months. If after another couple of weeks the stale label has not been removed this PR will be closed. This stale marker and eventual auto close does not indicate a judgement of the PR just lack of reviewer bandwidth and helps us keep the PR queue more manageable. If you would like this PR re-opened you can do so and a committer can remove the stale tag. Or you can open a new PR. Try to help review other PRs to increase PR review bandwidth which in turn helps yours.
--
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
[GitHub] [nifi] jfrazee commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r675244787
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -226,9 +239,12 @@ protected String getPath(ProcessContext context) {
options.setRecursive(recurseSubdirectories);
Pattern baseDirectoryPattern = Pattern.compile("^" + baseDirectory + "/?");
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
Review comment:
@exceptionfactory @timeabarna There's temp file handling to be considered in both the List and the Put so wanted to summarize here as well. IMO, List should probably filter _ and . by default, but it's configurable so still up to the user. The temp files written by Put should probably default to being written in a hidden directory and have random prefixes, but with the directory configurable so it could potentially be not in the same directory.
What do you think?
--
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
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r673962304
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureTempFilePrefixValidator.java
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.processors.azure.storage.utils;
+
+import org.apache.nifi.components.PropertyValue;
+import org.apache.nifi.components.ValidationContext;
+import org.apache.nifi.components.ValidationResult;
+import org.apache.nifi.components.Validator;
+
+import java.util.regex.Pattern;
+
+public class AzureTempFilePrefixValidator implements Validator {
+ private static final Pattern TEMP_FILE_PREFIX_PATTERN = Pattern.compile("^[^\"\\\\/:|<>*?]*$");
+
+ @Override
+ public ValidationResult validate(String subject, String input, ValidationContext context) {
+ PropertyValue prefixProperty = context.newPropertyValue(input);
+ final String prefix;
+ if (prefixProperty.isExpressionLanguagePresent()) {
+ try {
+ prefix = prefixProperty.evaluateAttributeExpressions().getValue();
+ } catch (final Exception e) {
+ return new ValidationResult.Builder()
+ .subject(subject)
+ .input(input)
+ .valid(false)
+ .explanation("Failed to evaluate the Attribute Expression Language due to " + e.toString())
+ .build();
+ }
+ } else {
+ prefix = prefixProperty.getValue();
+ }
+
+ if (TEMP_FILE_PREFIX_PATTERN.matcher(prefix).matches()) {
+ return new ValidationResult.Builder()
+ .subject(subject)
+ .input(prefix)
+ .valid(true)
+ .build();
+ } else {
+ return new ValidationResult.Builder()
+ .subject(subject)
+ .input(prefix)
+ .valid(false)
+ .explanation("Prefix cannot contain \" / \\ < > : | * ? characters.")
+ .build();
+ }
+ }
+}
Review comment:
Validator no longer needed, so it has been removed
--
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
[GitHub] [nifi] MuazmaZ commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
MuazmaZ commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r648759607
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureTempFilePrefixValidator.java
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.processors.azure.storage.utils;
+
+import org.apache.nifi.components.PropertyValue;
+import org.apache.nifi.components.ValidationContext;
+import org.apache.nifi.components.ValidationResult;
+import org.apache.nifi.components.Validator;
+
+import java.util.regex.Pattern;
+
+public class AzureTempFilePrefixValidator implements Validator {
+ private static final Pattern TEMP_FILE_PREFIX_PATTERN = Pattern.compile("^[^\"\\\\/:|<>*?]*$");
+
+ @Override
+ public ValidationResult validate(String subject, String input, ValidationContext context) {
+ PropertyValue prefixProperty = context.newPropertyValue(input);
+ final String prefix;
+ if (prefixProperty.isExpressionLanguagePresent()) {
+ try {
+ prefix = prefixProperty.evaluateAttributeExpressions().getValue();
+ } catch (final Exception e) {
+ return new ValidationResult.Builder()
+ .subject(subject)
+ .input(input)
+ .valid(false)
+ .explanation("Failed to evaluate the Attribute Expression Language due to " + e.toString())
+ .build();
+ }
+ } else {
+ prefix = prefixProperty.getValue();
+ }
+
+ if (TEMP_FILE_PREFIX_PATTERN.matcher(prefix).matches()) {
+ return new ValidationResult.Builder()
+ .subject(subject)
+ .input(prefix)
+ .valid(true)
+ .build();
+ } else {
+ return new ValidationResult.Builder()
+ .subject(subject)
+ .input(prefix)
+ .valid(false)
+ .explanation("Prefix cannot contain \" / \\ < > : | * ? characters.")
+ .build();
+ }
+ }
+}
Review comment:
In addition to this validation, a possible validation could be: The following file names are not allowed: LPT1, LPT2, LPT3, LPT4, LPT5, LPT6, LPT7, LPT8, LPT9, COM1, COM2, COM3, COM4, COM5, COM6, COM7, COM8, COM9, PRN, AUX, NUL, CON, CLOCK$, dot character (.), and two-dot characters (..).
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -192,4 +211,29 @@ static void uploadContent(DataLakeFileClient fileClient, InputStream in, long le
fileClient.flush(length);
}
+
+ private DataLakeFileClient renameFile(String fileName, String directoryPath, DataLakeFileClient fileClient, boolean overwrite) {
+ try {
+ final DataLakeRequestConditions destinationCondition = new DataLakeRequestConditions();
+ if (!overwrite) {
+ destinationCondition.setIfNoneMatch("*");
+ }
+ String destinationPath = StringUtils.isNotEmpty(directoryPath)
+ ? directoryPath + "/" + fileName
+ : fileName;
+ return fileClient.renameWithResponse(null, destinationPath, null, destinationCondition, null, null).getValue();
+ } catch (DataLakeStorageException dataLakeStorageException) {
+ getLogger().error("Error while renaming temp file on Azure Data Lake Storage", dataLakeStorageException);
Review comment:
What behavior is expected for the temp file if rename is unsuccessful?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r674702806
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -226,9 +239,12 @@ protected String getPath(ProcessContext context) {
options.setRecursive(recurseSubdirectories);
Pattern baseDirectoryPattern = Pattern.compile("^" + baseDirectory + "/?");
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
Review comment:
@exceptionfactory thanks for your remarks. The idea was to help providing a fix quickly in case the Customer needs to change the prefix for example they are already using the prefix or the path would be too long because of the additional characters of the prefix. Is there any other way the prefix could be changed without a property or code changing? I am open for ideas.
--
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
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r673961077
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -84,12 +89,24 @@
.allowableValues(FAIL_RESOLUTION, REPLACE_RESOLUTION, IGNORE_RESOLUTION)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
Review comment:
Renaming is the standard behaviour now
--
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
[GitHub] [nifi] github-actions[bot] closed pull request #5098: WIP:NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #5098:
URL: https://github.com/apache/nifi/pull/5098
--
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
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r646976688
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -129,6 +131,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
+ .name("azure-temp-file-prefix")
+ .displayName("Temp File Prefix for Azure")
Review comment:
Thanks for pointing out the other filter properties @jfrazee. I agree with you, it sounds like it would be cleaner to set a default value for the File Filter property to exclude temporary files as opposed to introducing a new property.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r674831997
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -226,9 +239,12 @@ protected String getPath(ProcessContext context) {
options.setRecursive(recurseSubdirectories);
Pattern baseDirectoryPattern = Pattern.compile("^" + baseDirectory + "/?");
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
Review comment:
Thanks for the clarification @timeabarna.
It seems like there are multiple competing use cases here, so thank you for working through the feedback.
On the one hand, the purpose of the Temporary File Prefix seems to be something internal to NiFi so that the PutAzureDataLakeStorage can write files with a temporary name that ListAzureDataLakeStorage will not pick up. For that use case, having an internal shared value, that cannot be changed, seems like the most logical approach.
One the other hand, if other components or services outside of NiFi may write files that could be considered temporary, then perhaps more configuration is necessary using a standard processor property descriptor. However, since ListAzureDataLakeStorage already has a `File Filter` property, it seems like that could be used to exclude temporary files from other sources.
If the goal is to support maximum configurability in `PutAzureDataLakeStorage` when writing temporary files, then it should be a processor property. However, if the temporary file prefix is specific enough to NiFi, using something like `AzureDataLakeStorage.` or perhaps prefixed with a period as `.AzureDataLakeStorage.`, then that seems like it should cover the intended use cases without needing to be configured.
Perhaps @jfrazee has some additional thoughts.
--
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
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r673960746
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -226,9 +239,11 @@ protected String getPath(ProcessContext context) {
options.setRecursive(recurseSubdirectories);
Pattern baseDirectoryPattern = Pattern.compile("^" + baseDirectory + "/?");
+ String tempFilePrefix = context.getProperty(TEMP_FILE_PREFIX).evaluateAttributeExpressions().getValue();
Review comment:
final keyword has been added.
--
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
[GitHub] [nifi] jfrazee commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r675222719
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -114,30 +119,23 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final DataLakeServiceClient storageClient = getStorageClient(context, flowFile);
final DataLakeFileSystemClient fileSystemClient = storageClient.getFileSystemClient(fileSystem);
final DataLakeDirectoryClient directoryClient = fileSystemClient.getDirectoryClient(directory);
- final DataLakeFileClient fileClient;
+ DataLakeFileClient fileClient;
final String conflictResolution = context.getProperty(CONFLICT_RESOLUTION).getValue();
boolean overwrite = conflictResolution.equals(REPLACE_RESOLUTION);
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
Review comment:
I think the same discussion as in List applies to this, but I think in this spot it's a good candidate to be a processor property.
--
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
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r675387450
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -226,9 +239,12 @@ protected String getPath(ProcessContext context) {
options.setRecursive(recurseSubdirectories);
Pattern baseDirectoryPattern = Pattern.compile("^" + baseDirectory + "/?");
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
Review comment:
@jfrazee @exceptionfactory Let me summarise my understanding on your recommendations as it seems that the final solution would be a mix of the old and new things :)
List processor:
- Modify listing to hide any file and folder which names starting with either _ or .
- Keep the boolean property which is turning on the filtering of the above
Put processor:
- Create a property for tempfolder .
- Modify rename to put the temp file in the provided tempfolder under a generated UUID with the existing file name
My questions:
1. As renaming will be location related not name related, is this still atomic?
2. Should tempfolder property used as name only, location only or both? This requires different validation.
3. What happens if a Customer would like to use Put processor as in PutHDFS, which is putting the temp file in the same directory as the file is located, using . + filename as naming? Should we allow this?
--
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
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r675387450
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -226,9 +239,12 @@ protected String getPath(ProcessContext context) {
options.setRecursive(recurseSubdirectories);
Pattern baseDirectoryPattern = Pattern.compile("^" + baseDirectory + "/?");
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
Review comment:
@jfrazee @exceptionfactory Let me summarise my understanding on your recommendations as it seems that the final solution would be a mix of the old and new things :)
List processor:
- Modify listing to hide any file and folder which names starting with either _ or .
- Keep the boolean property which is turning off the filtering of the above
Put processor:
- Create a property for temp folder .
- Modify rename to put the temp file in the provided temp folder under a generated UUID with the existing file name
My questions:
1. As renaming will be location related not name related, is this still atomic?
2. Should temp folder property used as name only, location only or both? This requires different validation.
3. What happens if a Customer would like to use Put processor as in PutHDFS, which is putting the temp file in the same directory as the file is located, using . + filename as naming? Should we allow this?
--
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
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r645581206
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -192,4 +211,29 @@ static void uploadContent(DataLakeFileClient fileClient, InputStream in, long le
fileClient.flush(length);
}
+
+ private DataLakeFileClient renameFile(String fileName, String directoryPath, DataLakeFileClient fileClient, boolean overwrite) {
Review comment:
Recommend adding `final` to these parameters.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -129,6 +131,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
+ .name("azure-temp-file-prefix")
+ .displayName("Temp File Prefix for Azure")
Review comment:
The concept seems straightforward, but given the behavior, would it make sense to rename this to something more general? Without reading the description, at first it sounds like the property will be used to create temporary files as opposed to filtering them. What do you think about renaming this property and associated variables to something like `Filter File Prefix`? Taking the concept one step further, would it make sense to change this to a Regular Expression pattern instead of a prefix?
In either case the `for Azure` portion of the property display name seems unnecessary since this component is already specific to Azure.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -226,9 +239,11 @@ protected String getPath(ProcessContext context) {
options.setRecursive(recurseSubdirectories);
Pattern baseDirectoryPattern = Pattern.compile("^" + baseDirectory + "/?");
+ String tempFilePrefix = context.getProperty(TEMP_FILE_PREFIX).evaluateAttributeExpressions().getValue();
Review comment:
Recommend adding `final` to this variable declaration.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -192,4 +211,29 @@ static void uploadContent(DataLakeFileClient fileClient, InputStream in, long le
fileClient.flush(length);
}
+
+ private DataLakeFileClient renameFile(String fileName, String directoryPath, DataLakeFileClient fileClient, boolean overwrite) {
+ try {
+ final DataLakeRequestConditions destinationCondition = new DataLakeRequestConditions();
+ if (!overwrite) {
+ destinationCondition.setIfNoneMatch("*");
+ }
+ String destinationPath = StringUtils.isNotEmpty(directoryPath)
+ ? directoryPath + "/" + fileName
+ : fileName;
+ return fileClient.renameWithResponse(null, destinationPath, null, destinationCondition, null, null).getValue();
+ } catch (DataLakeStorageException dataLakeStorageException) {
+ getLogger().error("Error while renaming temp file on Azure Data Lake Storage", dataLakeStorageException);
+ removeTempFile(fileClient);
+ throw dataLakeStorageException;
+ }
+ }
+
+ private void removeTempFile(DataLakeFileClient fileClient) {
+ try {
+ fileClient.delete();
+ } catch (Exception e) {
+ getLogger().error("Error while removing temp file on Azure Data Lake Storage", e);
Review comment:
Are there any additional details worth including from `DataLakeFileClient` in the error message, such as the file name?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r673962910
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -192,4 +211,29 @@ static void uploadContent(DataLakeFileClient fileClient, InputStream in, long le
fileClient.flush(length);
}
+
+ private DataLakeFileClient renameFile(String fileName, String directoryPath, DataLakeFileClient fileClient, boolean overwrite) {
+ try {
+ final DataLakeRequestConditions destinationCondition = new DataLakeRequestConditions();
+ if (!overwrite) {
+ destinationCondition.setIfNoneMatch("*");
+ }
+ String destinationPath = StringUtils.isNotEmpty(directoryPath)
+ ? directoryPath + "/" + fileName
+ : fileName;
+ return fileClient.renameWithResponse(null, destinationPath, null, destinationCondition, null, null).getValue();
+ } catch (DataLakeStorageException dataLakeStorageException) {
+ getLogger().error("Error while renaming temp file on Azure Data Lake Storage", dataLakeStorageException);
Review comment:
@MuazmaZ thanks for your help, temp file will remain on the server and will be overwritten at next try.
--
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
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r673961741
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/pom.xml
##########
@@ -78,6 +78,11 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>com.azure</groupId>
+ <artifactId>azure-core-http-netty</artifactId>
+ <version>1.7.0</version>
Review comment:
Dependency removed, raiesed azure.core and cosmos versions instead
--
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
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r674471757
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -226,9 +239,12 @@ protected String getPath(ProcessContext context) {
options.setRecursive(recurseSubdirectories);
Pattern baseDirectoryPattern = Pattern.compile("^" + baseDirectory + "/?");
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
Review comment:
Direct calls to `System.getProperty()` should be avoided. Can you explain the reason for getting the value of a system property?
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -102,6 +104,7 @@
"where the previous node left off, without duplicating the data.")
public class ListAzureDataLakeStorage extends AbstractListProcessor<ADLSFileInfo> {
+ public static final String TEMP_FILE_PREFIX = "nifitemp_";
Review comment:
Although this variable is `public`, it does not appear to be referenced outside of the processor, and `PutAzureDataLakeStorage` has the same value. It seems like this value should be abstracted to a shared location.
--
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
[GitHub] [nifi] jfrazee commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r675221806
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -226,9 +239,12 @@ protected String getPath(ProcessContext context) {
options.setRecursive(recurseSubdirectories);
Pattern baseDirectoryPattern = Pattern.compile("^" + baseDirectory + "/?");
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
Review comment:
I don't think there's much precedent for exposing config to processors through system properties. I can definitely see how it'd be convenient, but we can achieve this with the file filter so it seems redundant here.
--
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
[GitHub] [nifi] timeabarna commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r663661098
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -129,6 +131,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
+ .name("azure-temp-file-prefix")
+ .displayName("Temp File Prefix for Azure")
Review comment:
@exceptionfactory @jfrazee So if I understand you correctly, the prefix property should be removed from both put and list processors, having a built-in prefix instead. List processor should have a boolean property whether temp files should be included in the listing. Is this correct?
--
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
[GitHub] [nifi] jfrazee commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r675242859
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -114,30 +119,23 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final DataLakeServiceClient storageClient = getStorageClient(context, flowFile);
final DataLakeFileSystemClient fileSystemClient = storageClient.getFileSystemClient(fileSystem);
final DataLakeDirectoryClient directoryClient = fileSystemClient.getDirectoryClient(directory);
- final DataLakeFileClient fileClient;
+ DataLakeFileClient fileClient;
final String conflictResolution = context.getProperty(CONFLICT_RESOLUTION).getValue();
boolean overwrite = conflictResolution.equals(REPLACE_RESOLUTION);
+ final String tempFilePrefix = defaultIfBlank(System.getProperty("tempFilePrefix"), TEMP_FILE_PREFIX);
try {
- fileClient = directoryClient.createFile(fileName, overwrite);
-
- final long length = flowFile.getSize();
- if (length > 0) {
- try (final InputStream rawIn = session.read(flowFile); final BufferedInputStream bufferedIn = new BufferedInputStream(rawIn)) {
- uploadContent(fileClient, bufferedIn, length);
- } catch (Exception e) {
- removeTempFile(fileClient);
- throw e;
- }
- }
+ fileClient = directoryClient.createFile(tempFilePrefix + fileName, true);
Review comment:
Updating my comment here. A random prefix will be preferable to a random suffix, and given that conflicting names can lead to problems we probably shouldn't give absolute control to the user.
```java
// The temp files should be tucked away in a directory because of the impact
// they could have on file listings. Hadoop uses "_$azuretmpfolder$" statically
// but I think it could be useful to be configurable. The default should be
// prefaced with . or _ though.
final String tempFileFolder = context.getProperty(TEMP_FILE_FOLDER).getValue();
final String tempFilePrefix = tempFileFolder + "/" + UUID.randomUUID().toString();
final String tempFilePath = tempFilePrefix + fileName;
```
--
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
[GitHub] [nifi] jfrazee commented on a change in pull request #5098: NIFI-8248 Modify PutAzureDataLakeStorage processor to use temp file
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5098:
URL: https://github.com/apache/nifi/pull/5098#discussion_r645674083
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -84,12 +89,24 @@
.allowableValues(FAIL_RESOLUTION, REPLACE_RESOLUTION, IGNORE_RESOLUTION)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
Review comment:
I think it's important to default this to being enabled if not required but with a choice of the location to write to.
Not using a temp file opens up a risk for data corruption and there shouldn't be a noticeable performance penalty so I don't think there's a compelling case for keeping the previous behavior. Moreover, one of the goals of ADLS Gen2 was specifically to allow efficient write then rename so this improvement is for the most part the "right thing" to do.
I don't think we can change it to `required(true)` but we can treat a null `getValue()` as enabled so there shouldn't be an upgrade issue.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/test/java/org/apache/nifi/processors/azure/storage/ITPutAzureDataLakeStorage.java
##########
@@ -150,6 +240,21 @@ public void testPutBigFile() throws Exception {
assertSuccess(DIRECTORY, FILE_NAME, fileData);
}
+ @Ignore
+ // ignore excessive test with larger file size
Review comment:
There was a recent issue that would have been caught with big file tests and since this is an IT, I think it's fair to not ignore. At this size it runs quick anyway.
```suggestion
```
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -192,4 +211,29 @@ static void uploadContent(DataLakeFileClient fileClient, InputStream in, long le
fileClient.flush(length);
}
+
+ private DataLakeFileClient renameFile(String fileName, String directoryPath, DataLakeFileClient fileClient, boolean overwrite) {
+ try {
+ final DataLakeRequestConditions destinationCondition = new DataLakeRequestConditions();
+ if (!overwrite) {
+ destinationCondition.setIfNoneMatch("*");
+ }
+ String destinationPath = StringUtils.isNotEmpty(directoryPath)
Review comment:
```suggestion
String destinationPath = StringUtils.isNotBlank(directoryPath)
```
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java
##########
@@ -84,12 +89,24 @@
.allowableValues(FAIL_RESOLUTION, REPLACE_RESOLUTION, IGNORE_RESOLUTION)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
Review comment:
I haven't tested yet, but it looks like the validator will prevent using directories for this?
There are often downstream users -- Hadoop, Spark, etc. -- that I believe will assume temp files are going to be written to a directory and not individual files at the root. I need to double check that that's the case, but I think it'd be useful to align our default with those.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java
##########
@@ -129,6 +131,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor TEMP_FILE_PREFIX = new PropertyDescriptor.Builder()
+ .name("azure-temp-file-prefix")
+ .displayName("Temp File Prefix for Azure")
Review comment:
@exceptionfactory There are already overall filter properties. This is adding a filter specifically for the temp files that `PutAzureDataLakeStorage` is creating/using so it's reasonably named, but your question got me thinking though, why do we need another filter property?
I think it'd be clearer and simpler to include a default filter for hidden files with instructions to the user to set the filter if they want to override that.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org