exceptionfactory commented on code in PR #7603:
URL: https://github.com/apache/nifi/pull/7603#discussion_r1293703019
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/DeduplicateRecord.java:
##########
@@ -544,34 +534,28 @@ private void sendOrRemove(ProcessSession session,
}
}
- private String executeDynamicRecordPaths(ProcessContext context, Record
record, FlowFile flowFile) {
+ private String evaluateKeyFromDynamicProperties(ProcessContext context,
Record record, FlowFile flowFile) {
final List<String> fieldValues = new ArrayList<>();
for (final PropertyDescriptor propertyDescriptor : dynamicProperties) {
final String value =
context.getProperty(propertyDescriptor).evaluateAttributeExpressions(flowFile).getValue();
final RecordPath recordPath = recordPathCache.getCompiled(value);
final RecordPathResult result = recordPath.evaluate(record);
final List<FieldValue> selectedFields =
result.getSelectedFields().collect(Collectors.toList());
+ // Add the name of the dynamic property
fieldValues.add(propertyDescriptor.getName());
+ // Add any non-null record field values
fieldValues.addAll(selectedFields.stream()
- .map(f -> f.getValue().toString())
- .collect(toList())
+ .map(f -> f.getValue() != null ? f.getValue().toString() :
"")
Review Comment:
This appears to add an empty string, is there a particular reason for this
approach as opposed to filtering null values?
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/MockCacheService.java:
##########
@@ -25,21 +25,37 @@
import java.util.HashMap;
import java.util.Map;
-final class MockCacheService<K, V> extends AbstractControllerService
implements DistributedMapCacheClient {
- private Map storage;
+final class MockCacheService extends AbstractControllerService implements
DistributedMapCacheClient {
+ private Map<Object, Object> storage;
public MockCacheService() {
storage = new HashMap<>();
}
+ /**
+ * Puts the value to the cache with the specified key, if it doesn't
already exist
+ * @param key the key for into the map
+ * @param value the value to add to the map if and only if the key is
absent
+ * @param keySerializer key serializer
+ * @param valueSerializer value serializer
+ * @return true if the value was added to the cache, false if it already
exists
+ * @param <K>
+ * @param <V>
+ * @throws IOException
Review Comment:
These JavaDoc elements are missing descriptions, resulting in Checkstyle
warnings:
```
Warning:
src/test/java/org/apache/nifi/processors/standard/MockCacheService.java:[42]
(javadoc) NonEmptyAtclauseDescription: At-clause should have a non-empty
description.
Warning:
src/test/java/org/apache/nifi/processors/standard/MockCacheService.java:[43]
(javadoc) NonEmptyAtclauseDescription: At-clause should have a non-empty
description.
Warning:
src/test/java/org/apache/nifi/processors/standard/MockCacheService.java:[44]
(javadoc) NonEmptyAtclauseDescription: At-clause should have a non-empty
description.
```
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/DeduplicateRecord.java:
##########
@@ -625,40 +609,17 @@ public
DistributedMapCacheClientWrapper(DistributedMapCacheClient client) {
@Override
public boolean contains(String value) {
try {
- return client.containsKey(value, STRING_SERIALIZER);
+ return !client.putIfAbsent(value, "", STRING_SERIALIZER,
STRING_SERIALIZER);
Review Comment:
This appears to change the behavior, updating the value instead of checking
whether the key exists. Can you elaborate on the reasons for this change?
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/DeduplicateRecord.java:
##########
@@ -395,7 +391,7 @@ public void onScheduled(final ProcessContext context) {
private FilterWrapper getFilter(ProcessContext context) {
if (useInMemoryStrategy) {
boolean useHashSet = context.getProperty(FILTER_TYPE).getValue()
- .equals(context.getProperty(HASH_SET_VALUE.getValue()));
+
.equals(context.getProperty(HASH_SET_VALUE.getValue()).getValue());
Review Comment:
Is the additional `getValue()` call necessary?
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/DeduplicateRecord.java:
##########
@@ -625,40 +609,17 @@ public
DistributedMapCacheClientWrapper(DistributedMapCacheClient client) {
@Override
public boolean contains(String value) {
try {
- return client.containsKey(value, STRING_SERIALIZER);
+ return !client.putIfAbsent(value, "", STRING_SERIALIZER,
STRING_SERIALIZER);
} catch (IOException e) {
throw new ProcessException("Distributed Map lookup failed", e);
}
}
@Override
public void put(String value) {
- /*
- * This needs to be a noop because this process will be used
upstream of the systems that would write the records
- * that power the map cache.
- */
+ // Do nothing as the key is queried and put to the cache
atomically in the `contains` method.
}
}
private static final Serializer<String> STRING_SERIALIZER = (value,
output) -> output.write(value.getBytes(StandardCharsets.UTF_8));
- private static final Serializer<Boolean> BOOLEAN_SERIALIZER = (value,
output) -> output.write((byte) (value ? 1 : 0));
Review Comment:
Is this Boolean Serializer not used?
--
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]
