turcsanyip commented on code in PR #5944:
URL: https://github.com/apache/nifi/pull/5944#discussion_r875193626
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java:
##########
@@ -93,6 +92,21 @@ public class PutAzureBlobStorage extends
AbstractAzureBlobProcessor {
"will fail if the container does not exist.")
.build();
+ private static final List<PropertyDescriptor> properties =
Collections.unmodifiableList(Arrays.asList(
Review Comment:
Upper case names should be used for `static` `final` fields => `PROPERTIES`
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/DeleteAzureBlobStorage.java:
##########
@@ -64,24 +64,32 @@ public class DeleteAzureBlobStorage extends
AbstractAzureBlobProcessor {
.required(true)
.build();
+ private static final List<PropertyDescriptor> properties =
Collections.unmodifiableList(Arrays.asList(
+ AzureStorageUtils.CONTAINER_WITH_DEFAULT_VALUE,
+ BLOB,
+ AzureStorageUtils.STORAGE_CREDENTIALS_SERVICE,
+ AzureStorageUtils.ACCOUNT_NAME,
+ AzureStorageUtils.ACCOUNT_KEY,
+ AzureStorageUtils.PROP_SAS_TOKEN,
+ AzureStorageUtils.ENDPOINT_SUFFIX,
+ AzureStorageUtils.PROXY_CONFIGURATION_SERVICE,
+ DELETE_SNAPSHOTS_OPTION));
Review Comment:
I would consider to move this property up, just after BLOB because it is
related to the deleted blob.
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java:
##########
@@ -271,29 +275,32 @@ public static AzureStorageCredentialsDetails
createStorageCredentialsDetails(Pro
public static Collection<ValidationResult>
validateCredentialProperties(ValidationContext validationContext) {
final List<ValidationResult> results = new ArrayList<>();
- final String storageCredentials =
validationContext.getProperty(STORAGE_CREDENTIALS_SERVICE).getValue();
- final String accountName =
validationContext.getProperty(ACCOUNT_NAME).getValue();
- final String accountKey =
validationContext.getProperty(ACCOUNT_KEY).getValue();
- final String sasToken =
validationContext.getProperty(PROP_SAS_TOKEN).getValue();
- final String endpointSuffix =
validationContext.getProperty(ENDPOINT_SUFFIX).getValue();
+ final boolean credentialsSet =
validationContext.getProperty(STORAGE_CREDENTIALS_SERVICE).isSet();
+ final boolean accountNameSet =
StringUtils.isNotBlank(validationContext.getProperty(ACCOUNT_NAME).getValue());
+ final boolean accountKeySet =
StringUtils.isNotBlank(validationContext.getProperty(ACCOUNT_KEY).getValue());
+ final boolean sasTokenSet =
StringUtils.isNotBlank(validationContext.getProperty(PROP_SAS_TOKEN).getValue());
+ final boolean endpointSuffixSet =
StringUtils.isNotBlank(validationContext.getProperty(ENDPOINT_SUFFIX).getValue());
- if (!((StringUtils.isNotBlank(storageCredentials) &&
StringUtils.isBlank(accountName) && StringUtils.isBlank(accountKey) &&
StringUtils.isBlank(sasToken))
- || (StringUtils.isBlank(storageCredentials) &&
StringUtils.isNotBlank(accountName) && StringUtils.isNotBlank(accountKey) &&
StringUtils.isBlank(sasToken))
- || (StringUtils.isBlank(storageCredentials) &&
StringUtils.isNotBlank(accountName) && StringUtils.isBlank(accountKey) &&
StringUtils.isNotBlank(sasToken)))) {
+ final boolean onlyCredentialsServiceSet = credentialsSet &&
!accountNameSet && !accountKeySet && !sasTokenSet;
+ final boolean accountNameAndKeySet = !credentialsSet && !sasTokenSet
&& accountNameSet && accountKeySet;
+ final boolean accountNameAndSasTokenSet = !credentialsSet &&
!accountKeySet && accountNameSet && sasTokenSet;
Review Comment:
Following the naming concept of `onlyCredentialsServiceSet`, these variables
could be named `onlyAccountNameAndKeySet` and `onlyAccountNameAndSasTokenSet`.
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java:
##########
@@ -93,6 +92,21 @@ public class PutAzureBlobStorage extends
AbstractAzureBlobProcessor {
"will fail if the container does not exist.")
.build();
+ private static final List<PropertyDescriptor> properties =
Collections.unmodifiableList(Arrays.asList(
+ AzureStorageUtils.CONTAINER,
+ BLOB_NAME,
+ CREATE_CONTAINER,
+ AzureStorageUtils.STORAGE_CREDENTIALS_SERVICE,
+ AzureStorageUtils.ACCOUNT_NAME,
+ AzureStorageUtils.ACCOUNT_KEY,
+ AzureStorageUtils.PROP_SAS_TOKEN,
+ AzureStorageUtils.ENDPOINT_SUFFIX,
+ AzureStorageUtils.PROXY_CONFIGURATION_SERVICE,
+ AzureBlobClientSideEncryptionUtils.CSE_KEY_TYPE,
+ AzureBlobClientSideEncryptionUtils.CSE_KEY_ID,
+ AzureBlobClientSideEncryptionUtils.CSE_SYMMETRIC_KEY_HEX
Review Comment:
I would consider to move these properties up, just after `CREATE_CONTAINER`.
They belong to the "upload process / data", similar to `CREATE_CONTAINER`.
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureBlobStorage.java:
##########
@@ -140,6 +144,15 @@ protected Map<String, String> createAttributes(BlobInfo
entity, ProcessContext c
attributes.put("mime.type", entity.getContentType());
attributes.put("lang", entity.getContentLanguage());
+ final String entityName = entity.getBlobName();
+ if (entityName.contains("/")) {
+ attributes.put(CoreAttributes.FILENAME.key(),
StringUtils.substringAfterLast(entityName, "/"));
+ attributes.put(CoreAttributes.PATH.key(),
StringUtils.substringBeforeLast(entityName, "/"));
+ } else {
+ attributes.put(CoreAttributes.FILENAME.key(), entityName);
+ attributes.put(CoreAttributes.PATH.key(), ".");
Review Comment:
The blob is in the "root directory" in this case. Shouldn't it be `'/'`
instead?
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureBlobStorage.java:
##########
@@ -99,20 +103,20 @@ public class ListAzureBlobStorage extends
AbstractListProcessor<BlobInfo> {
.build();
private static final List<PropertyDescriptor> PROPERTIES =
Collections.unmodifiableList(Arrays.asList(
- LISTING_STRATEGY,
- AbstractListProcessor.RECORD_WRITER,
- AzureStorageUtils.CONTAINER,
- AzureStorageUtils.STORAGE_CREDENTIALS_SERVICE,
- AzureStorageUtils.ACCOUNT_NAME,
- AzureStorageUtils.ACCOUNT_KEY,
- AzureStorageUtils.PROP_SAS_TOKEN,
- AzureStorageUtils.ENDPOINT_SUFFIX,
- PROP_PREFIX,
- AzureStorageUtils.PROXY_CONFIGURATION_SERVICE,
- ListedEntityTracker.TRACKING_STATE_CACHE,
- ListedEntityTracker.TRACKING_TIME_WINDOW,
- ListedEntityTracker.INITIAL_LISTING_TARGET
- ));
+ AzureStorageUtils.CONTAINER,
+ LISTING_STRATEGY,
+ AbstractListProcessor.RECORD_WRITER,
+ AzureStorageUtils.STORAGE_CREDENTIALS_SERVICE,
+ AzureStorageUtils.ACCOUNT_NAME,
+ AzureStorageUtils.ACCOUNT_KEY,
+ AzureStorageUtils.PROP_SAS_TOKEN,
+ AzureStorageUtils.ENDPOINT_SUFFIX,
+ PROP_PREFIX,
+ AzureStorageUtils.PROXY_CONFIGURATION_SERVICE,
+ ListedEntityTracker.TRACKING_STATE_CACHE,
+ ListedEntityTracker.TRACKING_TIME_WINDOW,
+ ListedEntityTracker.INITIAL_LISTING_TARGET
Review Comment:
I think property ordering could be improved further:
- `PROP_PREFIX`: just after `CONTAINER`
- `ListedEntityTracker.*`: following LISTING_STRATEGY
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/FetchAzureBlobStorage.java:
##########
@@ -85,6 +79,22 @@ public class FetchAzureBlobStorage extends
AbstractAzureBlobProcessor {
.required(false)
.build();
+ private static final List<PropertyDescriptor> properties =
Collections.unmodifiableList(Arrays.asList(
+ AzureStorageUtils.CONTAINER_WITH_DEFAULT_VALUE,
+ BLOB,
+ AzureStorageUtils.STORAGE_CREDENTIALS_SERVICE,
+ AzureStorageUtils.ACCOUNT_NAME,
+ AzureStorageUtils.ACCOUNT_KEY,
+ AzureStorageUtils.PROP_SAS_TOKEN,
+ AzureStorageUtils.ENDPOINT_SUFFIX,
+ RANGE_START,
+ RANGE_LENGTH,
+ AzureBlobClientSideEncryptionUtils.CSE_KEY_TYPE,
+ AzureBlobClientSideEncryptionUtils.CSE_KEY_ID,
+ AzureBlobClientSideEncryptionUtils.CSE_SYMMETRIC_KEY_HEX,
Review Comment:
I would consider to move these properties up, just after `BLOB`. They are
related to the blob being fetched.
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureBlobStorage_v12.java:
##########
@@ -131,14 +132,14 @@ public class ListAzureBlobStorage_v12 extends
AbstractListProcessor<BlobInfo> {
.build();
private static final List<PropertyDescriptor> PROPERTIES =
Collections.unmodifiableList(Arrays.asList(
- STORAGE_CREDENTIALS_SERVICE,
- CONTAINER,
- BLOB_NAME_PREFIX,
- RECORD_WRITER,
- LISTING_STRATEGY,
- TRACKING_STATE_CACHE,
- TRACKING_TIME_WINDOW,
- INITIAL_LISTING_TARGET
+ CONTAINER,
+ LISTING_STRATEGY,
+ RECORD_WRITER,
+ STORAGE_CREDENTIALS_SERVICE,
+ BLOB_NAME_PREFIX,
+ TRACKING_STATE_CACHE,
+ TRACKING_TIME_WINDOW,
+ INITIAL_LISTING_TARGET
Review Comment:
In my opinion, the original ordering was better.
The concept was:
1. credential
2. what to list (container and prefix)
3. listing strategy and its related properties
We can change the order of these groups (e.g. move the credential down,
though I always set it first) but I would not split [container + prefix] and
[listing strategy + related properties].
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/FetchAzureBlobStorage_v12.java:
##########
@@ -109,11 +100,11 @@ public class FetchAzureBlobStorage_v12 extends
AbstractAzureBlobProcessor_v12 {
.build();
private static final List<PropertyDescriptor> PROPERTIES =
Collections.unmodifiableList(Arrays.asList(
- STORAGE_CREDENTIALS_SERVICE,
- CONTAINER,
- BLOB_NAME,
- RANGE_START,
- RANGE_LENGTH
+ CONTAINER,
+ BLOB_NAME,
+ STORAGE_CREDENTIALS_SERVICE,
+ RANGE_START,
+ RANGE_LENGTH
Review Comment:
Similar to List / Put, the concept was:
- credential first
- container
- blob (and related)
The credential property can be moved down but I would not split the blob and
range properties because they belong together.
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage_v12.java:
##########
@@ -92,10 +97,10 @@ public class PutAzureBlobStorage_v12 extends
AbstractAzureBlobProcessor_v12 {
.build();
private static final List<PropertyDescriptor> PROPERTIES =
Collections.unmodifiableList(Arrays.asList(
- STORAGE_CREDENTIALS_SERVICE,
- AzureStorageUtils.CONTAINER,
- CREATE_CONTAINER,
- BLOB_NAME
+ AzureStorageUtils.CONTAINER,
+ BLOB_NAME,
+ CREATE_CONTAINER,
+ STORAGE_CREDENTIALS_SERVICE
Review Comment:
Similar to the List processor, the original concept was:
- credential first
- container
- blob
I would rather not split `CONTAINER` + `CREATE_CONTAINER`.
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureDataLakeStorage.java:
##########
@@ -123,33 +122,34 @@ public class ListAzureDataLakeStorage extends
AbstractListProcessor<ADLSFileInfo
public static final PropertyDescriptor PATH_FILTER = new
PropertyDescriptor.Builder()
.name("path-filter")
.displayName("Path Filter")
- .description(String.format("When '%s' is true, then only
subdirectories whose paths match the given regular expression will be scanned",
RECURSE_SUBDIRECTORIES.getDisplayName()))
+ .description("Only subdirectories whose paths match the given
regular expression will be scanned")
.required(false)
.addValidator(StandardValidators.REGULAR_EXPRESSION_WITH_EL_VALIDATOR)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .dependsOn(RECURSE_SUBDIRECTORIES, "true")
.build();
private static final List<PropertyDescriptor> PROPERTIES =
Collections.unmodifiableList(Arrays.asList(
- ADLS_CREDENTIALS_SERVICE,
- FILESYSTEM,
- DIRECTORY,
- RECURSE_SUBDIRECTORIES,
- FILE_FILTER,
- PATH_FILTER,
- RECORD_WRITER,
- LISTING_STRATEGY,
- TRACKING_STATE_CACHE,
- TRACKING_TIME_WINDOW,
- INITIAL_LISTING_TARGET));
+ FILESYSTEM,
+ DIRECTORY,
+ ADLS_CREDENTIALS_SERVICE,
+ FILE_FILTER,
+ RECURSE_SUBDIRECTORIES,
+ PATH_FILTER,
Review Comment:
I believe `DIRECTORY` and `RECURSE_SUBDIRECTORIES` belong together and
should not be split. Also `FILE_FILTER` and `PATH_FILTER`.
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/FetchAzureDataLakeStorage.java:
##########
@@ -78,12 +79,18 @@ public class FetchAzureDataLakeStorage extends
AbstractAzureDataLakeStorageProce
.defaultValue("0")
.build();
+ private static final List<PropertyDescriptor> properties =
Collections.unmodifiableList(Arrays.asList(
+ FILESYSTEM_WITH_DEFAULT,
+ DIRECTORY_WITH_DEFAULT,
+ FILE,
+ ADLS_CREDENTIALS_SERVICE,
+ RANGE_START,
+ RANGE_LENGTH,
+ NUM_RETRIES
Review Comment:
Similar to `FetchAzureBlobStorage_v12`: `FILE` and `RANGE_*`properties
should not be split, I believe.
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureBlobStorage.java:
##########
@@ -63,12 +57,22 @@
import org.apache.nifi.processors.azure.storage.utils.BlobInfo.Builder;
import org.apache.nifi.serialization.record.RecordSchema;
+import java.io.IOException;
+import java.net.URISyntaxException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import java.util.Optional;
@PrimaryNodeOnly
@TriggerSerially
@Tags({ "azure", "microsoft", "cloud", "storage", "blob" })
-@SeeAlso({ FetchAzureBlobStorage.class, PutAzureBlobStorage.class,
DeleteAzureBlobStorage.class })
+@SeeAlso({ ListAzureBlobStorage_v12.class, FetchAzureBlobStorage.class,
PutAzureBlobStorage.class, DeleteAzureBlobStorage.class })
Review Comment:
It is a good idea to refer the v12 List processor here. Similar references
could be added to the Put / Delete / Fetch processors too.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
