turcsanyip commented on code in PR #8231:
URL: https://github.com/apache/nifi/pull/8231#discussion_r1449519643
##########
nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/ListS3.java:
##########
@@ -1189,4 +1180,17 @@ public List<ConfigVerificationResult> verify(final
ProcessContext context, final
return results;
}
+
+ /**
+ * Return whether to include the entity in the listing, based on the
minimum and maximum object age (if configured).
+ */
+ private boolean includeObjectInListing(final ProcessContext context, final
S3VersionSummary versionSummary) {
+ final Long minAgeMilliseconds =
context.getProperty(MIN_AGE).asTimePeriod(TimeUnit.MILLISECONDS);
+ final Long maxAgeMilliseconds = context.getProperty(MAX_AGE) != null ?
context.getProperty(MAX_AGE).asTimePeriod(TimeUnit.MILLISECONDS) : null;
+ final long lastModifiedTime =
versionSummary.getLastModified().getTime();
+ final long currentTime = System.currentTimeMillis();
Review Comment:
The current time should be evaluated only once per verification/listing.
Otherwise items may or may not be listed depending on the time the filter is
executed for the given item (even if 2 items have the same timestamp). So
`currentTime` should be a parameter of this method.
##########
nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/ListS3.java:
##########
@@ -1189,4 +1180,17 @@ public List<ConfigVerificationResult> verify(final
ProcessContext context, final
return results;
}
+
+ /**
+ * Return whether to include the entity in the listing, based on the
minimum and maximum object age (if configured).
+ */
+ private boolean includeObjectInListing(final ProcessContext context, final
S3VersionSummary versionSummary) {
Review Comment:
I think this method could be used in `listByTrackingTimestamps()` too.
##########
nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/ListS3.java:
##########
@@ -165,7 +165,7 @@ public class ListS3 extends AbstractS3Processor implements
VerifiableProcessor {
public static final PropertyDescriptor TRACKING_TIME_WINDOW = new
PropertyDescriptor.Builder()
.fromPropertyDescriptor(ListedEntityTracker.TRACKING_TIME_WINDOW)
- .dependsOn(INITIAL_LISTING_TARGET,
ListedEntityTracker.INITIAL_LISTING_TARGET_WINDOW)
+ .dependsOn(ListedEntityTracker.TRACKING_STATE_CACHE)
Review Comment:
Thanks for fixing the incorrect property dependency!
I would also change the order of the properties on the UI and use the same
sequence as in the other List processors:
- `TRACKING_STATE_CACHE`
- `TRACKING_TIME_WINDOW`
- `INITIAL_LISTING_TARGET`
`INITIAL_LISTING_TARGET` can be `required=true` because it is only shown
when it is required (due to `dependsOn`) and has a default value too. This way
these 3 properties would appear consistently on the UI.
##########
nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/ListS3.java:
##########
@@ -1189,4 +1180,17 @@ public List<ConfigVerificationResult> verify(final
ProcessContext context, final
return results;
}
+
+ /**
+ * Return whether to include the entity in the listing, based on the
minimum and maximum object age (if configured).
+ */
+ private boolean includeObjectInListing(final ProcessContext context, final
S3VersionSummary versionSummary) {
+ final Long minAgeMilliseconds =
context.getProperty(MIN_AGE).asTimePeriod(TimeUnit.MILLISECONDS);
+ final Long maxAgeMilliseconds = context.getProperty(MAX_AGE) != null ?
context.getProperty(MAX_AGE).asTimePeriod(TimeUnit.MILLISECONDS) : null;
Review Comment:
I feel a bit overkill to evaluate the properties for each listed item
separately. As Min/Max Age do not change while the processor is running, they
can be stored in instance fields of the processor object that can be set in
`onScheduled()`.
--
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]
