exceptionfactory commented on code in PR #10825:
URL: https://github.com/apache/nifi/pull/10825#discussion_r2742227457
##########
nifi-extension-bundles/nifi-iceberg-bundle/nifi-iceberg-aws/src/main/java/org/apache/nifi/services/iceberg/aws/S3IcebergFileIOProvider.java:
##########
@@ -102,12 +102,32 @@ public class S3IcebergFileIOProvider extends
AbstractControllerService implement
)
.build();
+ static final PropertyDescriptor ENDPOINT_OVERRIDE = new
PropertyDescriptor.Builder()
+ .name("Endpoint Override URL")
+ .description("Endpoint URL to use instead of the AWS default
including scheme, host, port, and path. " +
+ "The AWS libraries select an endpoint URL based on the AWS
region, but this property overrides " +
+ "the selected endpoint URL, allowing use with other
S3-compatible endpoints.")
+ .expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
Review Comment:
I recommend avoiding the use of the `ENVIRONMENT` scope for now, it could be
added later if needed.
##########
nifi-extension-bundles/nifi-iceberg-bundle/nifi-iceberg-aws/src/main/java/org/apache/nifi/services/iceberg/aws/S3IcebergFileIOProvider.java:
##########
@@ -102,12 +102,32 @@ public class S3IcebergFileIOProvider extends
AbstractControllerService implement
)
.build();
+ static final PropertyDescriptor ENDPOINT_OVERRIDE = new
PropertyDescriptor.Builder()
+ .name("Endpoint Override URL")
+ .description("Endpoint URL to use instead of the AWS default
including scheme, host, port, and path. " +
+ "The AWS libraries select an endpoint URL based on the AWS
region, but this property overrides " +
+ "the selected endpoint URL, allowing use with other
S3-compatible endpoints.")
Review Comment:
Please reformat using a multiline string.
##########
nifi-extension-bundles/nifi-iceberg-bundle/nifi-iceberg-aws/src/main/java/org/apache/nifi/services/iceberg/aws/S3IcebergFileIOProvider.java:
##########
@@ -102,12 +102,32 @@ public class S3IcebergFileIOProvider extends
AbstractControllerService implement
)
.build();
+ static final PropertyDescriptor ENDPOINT_OVERRIDE = new
PropertyDescriptor.Builder()
+ .name("Endpoint Override URL")
+ .description("Endpoint URL to use instead of the AWS default
including scheme, host, port, and path. " +
+ "The AWS libraries select an endpoint URL based on the AWS
region, but this property overrides " +
+ "the selected endpoint URL, allowing use with other
S3-compatible endpoints.")
+ .expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
+ .required(false)
+ .addValidator(StandardValidators.URL_VALIDATOR)
+ .build();
+
+ static final PropertyDescriptor USE_PATH_STYLE_ACCESS = new
PropertyDescriptor.Builder()
+ .name("Use Path Style Access")
+ .description("Path-style access can be enforced by setting this
property to true. Set it to true if your endpoint does not support " +
+ "virtual-hosted-style requests, only path-style requests.")
+ .allowableValues("true", "false")
+
.defaultValue(String.valueOf(S3FileIOProperties.PATH_STYLE_ACCESS_DEFAULT))
+ .build();
Review Comment:
A Boolean validator should be added
##########
nifi-extension-bundles/nifi-iceberg-bundle/nifi-iceberg-aws/src/main/java/org/apache/nifi/services/iceberg/aws/S3IcebergFileIOProvider.java:
##########
@@ -102,12 +102,32 @@ public class S3IcebergFileIOProvider extends
AbstractControllerService implement
)
.build();
+ static final PropertyDescriptor ENDPOINT_OVERRIDE = new
PropertyDescriptor.Builder()
+ .name("Endpoint Override URL")
Review Comment:
I recommend naming this `Endpoint URL` to align more closely with the
underlying Iceberg property.
```suggestion
static final PropertyDescriptor ENDPOINT_URL = new
PropertyDescriptor.Builder()
.name("Endpoint URL")
```
##########
nifi-extension-bundles/nifi-iceberg-bundle/nifi-iceberg-aws/src/main/java/org/apache/nifi/services/iceberg/aws/S3IcebergFileIOProvider.java:
##########
@@ -102,12 +102,32 @@ public class S3IcebergFileIOProvider extends
AbstractControllerService implement
)
.build();
+ static final PropertyDescriptor ENDPOINT_OVERRIDE = new
PropertyDescriptor.Builder()
+ .name("Endpoint Override URL")
+ .description("Endpoint URL to use instead of the AWS default
including scheme, host, port, and path. " +
+ "The AWS libraries select an endpoint URL based on the AWS
region, but this property overrides " +
+ "the selected endpoint URL, allowing use with other
S3-compatible endpoints.")
+ .expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
+ .required(false)
+ .addValidator(StandardValidators.URL_VALIDATOR)
+ .build();
+
+ static final PropertyDescriptor USE_PATH_STYLE_ACCESS = new
PropertyDescriptor.Builder()
+ .name("Use Path Style Access")
Review Comment:
I recommend renaming this to `Path Style Access` to align more closely to
the Iceberg property name.
```suggestion
static final PropertyDescriptor PATH_STYLE_ACCESS = new
PropertyDescriptor.Builder()
.name("Path Style Access")
```
##########
nifi-extension-bundles/nifi-iceberg-bundle/nifi-iceberg-aws/src/main/java/org/apache/nifi/services/iceberg/aws/S3IcebergFileIOProvider.java:
##########
@@ -102,12 +102,32 @@ public class S3IcebergFileIOProvider extends
AbstractControllerService implement
)
.build();
+ static final PropertyDescriptor ENDPOINT_OVERRIDE = new
PropertyDescriptor.Builder()
+ .name("Endpoint Override URL")
+ .description("Endpoint URL to use instead of the AWS default
including scheme, host, port, and path. " +
+ "The AWS libraries select an endpoint URL based on the AWS
region, but this property overrides " +
+ "the selected endpoint URL, allowing use with other
S3-compatible endpoints.")
+ .expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
+ .required(false)
+ .addValidator(StandardValidators.URL_VALIDATOR)
+ .build();
+
+ static final PropertyDescriptor USE_PATH_STYLE_ACCESS = new
PropertyDescriptor.Builder()
+ .name("Use Path Style Access")
+ .description("Path-style access can be enforced by setting this
property to true. Set it to true if your endpoint does not support " +
+ "virtual-hosted-style requests, only path-style requests.")
+ .allowableValues("true", "false")
+
.defaultValue(String.valueOf(S3FileIOProperties.PATH_STYLE_ACCESS_DEFAULT))
Review Comment:
This can be marked as `required(true)` since a default value is provided.
##########
nifi-extension-bundles/nifi-iceberg-bundle/nifi-iceberg-aws/src/main/java/org/apache/nifi/services/iceberg/aws/S3IcebergFileIOProvider.java:
##########
@@ -102,12 +102,32 @@ public class S3IcebergFileIOProvider extends
AbstractControllerService implement
)
.build();
+ static final PropertyDescriptor ENDPOINT_OVERRIDE = new
PropertyDescriptor.Builder()
+ .name("Endpoint Override URL")
+ .description("Endpoint URL to use instead of the AWS default
including scheme, host, port, and path. " +
+ "The AWS libraries select an endpoint URL based on the AWS
region, but this property overrides " +
+ "the selected endpoint URL, allowing use with other
S3-compatible endpoints.")
+ .expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
+ .required(false)
+ .addValidator(StandardValidators.URL_VALIDATOR)
+ .build();
+
+ static final PropertyDescriptor USE_PATH_STYLE_ACCESS = new
PropertyDescriptor.Builder()
+ .name("Use Path Style Access")
+ .description("Path-style access can be enforced by setting this
property to true. Set it to true if your endpoint does not support " +
+ "virtual-hosted-style requests, only path-style requests.")
Review Comment:
```suggestion
.description("Path-style access can be enforced by setting this
property to true. Set it to true if the configured endpoint does not support " +
"virtual-hosted-style requests, only path-style
requests.")
```
##########
nifi-extension-bundles/nifi-iceberg-bundle/nifi-iceberg-aws/src/main/java/org/apache/nifi/services/iceberg/aws/S3IcebergFileIOProvider.java:
##########
@@ -102,12 +102,32 @@ public class S3IcebergFileIOProvider extends
AbstractControllerService implement
)
.build();
+ static final PropertyDescriptor ENDPOINT_OVERRIDE = new
PropertyDescriptor.Builder()
+ .name("Endpoint Override URL")
+ .description("Endpoint URL to use instead of the AWS default
including scheme, host, port, and path. " +
+ "The AWS libraries select an endpoint URL based on the AWS
region, but this property overrides " +
+ "the selected endpoint URL, allowing use with other
S3-compatible endpoints.")
+ .expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
+ .required(false)
+ .addValidator(StandardValidators.URL_VALIDATOR)
+ .build();
+
+ static final PropertyDescriptor USE_PATH_STYLE_ACCESS = new
PropertyDescriptor.Builder()
+ .name("Use Path Style Access")
+ .description("Path-style access can be enforced by setting this
property to true. Set it to true if your endpoint does not support " +
+ "virtual-hosted-style requests, only path-style requests.")
+ .allowableValues("true", "false")
Review Comment:
Recommend using `Boolean.TRUE.toString()`
--
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]
