NIFI-3404 Add lookup processor for enrichments/joins to reference data #1830

jfrazee · 2017-05-19T19:31:22Z

Note: This is overlaid on NIFI-3339/#1450.

ijokarumawak · 2017-05-22T05:14:11Z

Reviewing...

jfrazee · 2017-05-22T05:25:45Z

@ijokarumawak FYI, I think we need to make sure we see what happens to NIFI-3946/#1833 before we merge this. By all means start having a look though.

ijokarumawak

Thank you @jfrazee for your contribution, these enhancements will be really helpful!

Although this PR is waiting for others to be finalized, I went ahead, and took a look through changes. Please check comments.

ijokarumawak · 2017-05-22T07:18:56Z

...e/nifi-lookup-services/src/main/java/org/apache/nifi/lookup/SimpleKeyValueLookupService.java

@@ -41,7 +48,9 @@ protected PropertyDescriptor getSupportedDynamicPropertyDescriptor(final String
            .name(propertyDescriptorName)
            .required(false)
            .dynamic(true)
-            .addValidator(Validator.VALID)
+            .addValidator(StandardValidators.createAttributeExpressionLanguageValidator(AttributeExpression.ResultType.STRING, true))
+            .addValidator(StandardValidators.ATTRIBUTE_KEY_PROPERTY_NAME_VALIDATOR)


I think we can remove ATTRIBUTE_KEY_PROPERTY_NAME_VALIDATOR. The dynamic properties of this processor are not representing FlowFile attribute names. It is a little bit misleading.

ijokarumawak · 2017-05-22T07:39:24Z

...e/nifi-lookup-services/src/main/java/org/apache/nifi/lookup/SimpleKeyValueLookupService.java

-            .addValidator(Validator.VALID)
+            .addValidator(StandardValidators.createAttributeExpressionLanguageValidator(AttributeExpression.ResultType.STRING, true))
+            .addValidator(StandardValidators.ATTRIBUTE_KEY_PROPERTY_NAME_VALIDATOR)
+            .expressionLanguageSupported(true)


This change enabled EL support, however, cacheConfiguredValues method is not updated to evaluate EL.

You're right, I was trying to merge changes in from a VolatileLookupService I wrote elsewhere and didn't get that updated. The use for this is a little marginal so I'll remove it and we can see if the need comes back.

ijokarumawak · 2017-05-22T07:45:32Z

...ard-services/nifi-lookup-service-api/src/main/java/org/apache/nifi/lookup/LookupService.java

+     * @throws LookupFailureException if the backing service is unavailable or
+     * the table cannot be loaded
+     */
+    default Map<String, T> asMap() throws LookupFailureException {


Returning entire lookup table as a Map would cause issue with a large number of entries. I think this should be avoided.

I think/know there's a use case for this but I get the worry, so I'll drop it.

ijokarumawak · 2017-05-22T07:48:47Z

.../nifi-lookup-processors/src/main/java/org/apache/nifi/processors/lookup/LookupAttribute.java

+            if (dynamicProperties.isEmpty()) {
+                try {
+                    // If there aren't any dynamic properties, load the entire lookup table
+                    final Map<String, String> lookupTable = lookupService.asMap();


As I commented at the interface, loading entire lookup table and setting those into FlowFile attributes can be problematic if the number of entries increases. I'd prefer making at least one dynamicProperty required.

Will remove.

ijokarumawak · 2017-05-22T08:06:43Z

...-bundle/nifi-lookup-services/src/main/java/org/apache/nifi/lookup/DatabaseLookupService.java

+
+@Tags({"lookup", "cache", "enrich", "join", "jdbc", "database", "key", "value"})
+@CapabilityDescription("A reloadable properties file-based lookup service")
+public class DatabaseLookupService extends AbstractControllerService implements StringLookupService {


While this lookup single column service is powerful, I think it can be more powerful if we also provide lookup-record implementation like IPLookupService which lookups Record in the future.

ijokarumawak · 2017-05-22T08:10:19Z

...-bundle/nifi-lookup-services/src/main/java/org/apache/nifi/lookup/DatabaseLookupService.java

+    @OnEnabled
+    public void onEnabled(final ConfigurationContext context) throws InitializationException {
+        final DBCPService databaseService = context.getProperty(CONNECTION_POOL).asControllerService(DBCPService.class);
+        final DataSource dataSource = databaseService.getDataSource();


As discussed at #1450, I think we'd better to avoid exposing DataSource from DBCPService. Wrapping Connection to be a DataSource can be done here without requiring DBCPService change. Thought?

mattyb149 · 2017-05-24T17:40:18Z

...i-standard-processors/src/main/java/org/apache/nifi/processors/standard/LookupAttribute.java

+            .required(true)
+            .build();
+
+    public static final Relationship REL_SUCCESS = new Relationship.Builder()


Should this be "matched" instead of "success"? Seems like it better goes with the "unmatched" relationship in terms of behavior.

mattyb149 · 2017-05-24T17:42:17Z

...i-standard-processors/src/main/java/org/apache/nifi/processors/standard/LookupAttribute.java

+@Tags({"lookup", "cache", "enrich", "join", "mutable", "attributes", "Attribute Expression Language"})
+@CapabilityDescription("Lookup attributes from a lookup service")
+@DynamicProperty(name = "The name of the attribute to add to the FlowFile",
+    value = "The name of the key or property to retrieve from the lookup service",


The value of the dynamic property is the value to match as the key against the lookup service right? I wonder if saying "name of the key" is confusing vs "value for the key". Nothing required here, just thinking aloud :)

mattyb149 · 2017-05-24T17:43:29Z

...i-standard-processors/src/main/java/org/apache/nifi/processors/standard/LookupAttribute.java

+@SideEffectFree
+@SupportsBatching
+@InputRequirement(Requirement.INPUT_REQUIRED)
+@Tags({"lookup", "cache", "enrich", "join", "mutable", "attributes", "Attribute Expression Language"})


What does "mutable" refer to here?

Holdover from the interfaces this was originally written against. There was a LookupService and a MutableLookupService. Will remove.

jfrazee · 2017-05-25T01:47:21Z

@ijokarumawak The changes from NIFI-3339/#1450 have been removed from this PR along with the DatabaseLookupService. Will re-submit that stuff in another PR so we can move ahead with this and work out the details separately.

ijokarumawak · 2017-05-25T01:52:51Z

@jfrazee I understand, thank you! I am reviewing the updated commit now.

ijokarumawak · 2017-05-25T02:06:37Z

...le/nifi-lookup-services/src/main/java/org/apache/nifi/lookup/SimpleCsvFileLookupService.java

+import org.apache.nifi.util.file.monitor.SynchronousFileWatcher;
+
+@Tags({"lookup", "cache", "enrich", "join", "csv", "reloadable", "key", "value"})
+@CapabilityDescription("A reloadable properties file-based lookup service")


"CSV file-based" instead of "properties".

I can fix this when I merge, unless I find other issues.

ijokarumawak · 2017-05-25T02:42:49Z

...e/nifi-lookup-services/src/main/java/org/apache/nifi/lookup/SimpleKeyValueLookupService.java

@@ -47,6 +47,7 @@ protected PropertyDescriptor getSupportedDynamicPropertyDescriptor(final String
            .required(false)
            .dynamic(true)
            .addValidator(Validator.VALID)
+            .expressionLanguageSupported(true)


cacheConfiguredValues is still not evaluating expression. I can add EL evaluation when I merge.

I meant to remove the expressionLanguageSupported(true). I don't think I had an especially good reason to have added the EL support to the dynamic property for the CS in the first place.

I think supporting EL is useful, because user can refer variable registries or do some text processing with EL.

ijokarumawak · 2017-05-25T06:23:32Z

@jfrazee @mattyb149 I reviewed the updated commit and found few points to improve.

I was trying to comment all of those, but since it's easier to explain with code instead of comment, I created another PR based on this one.

@jfrazee Would you take a look on #1856 to see if those changes are reasonable? Please find the commit comments for what I've changed. If my additional commit looks ok, then I'd give +1 to this PR.

jfrazee · 2017-05-25T16:07:25Z

Superseded by #1856

jfrazee force-pushed the NIFI-3404 branch 3 times, most recently from 79f1752 to 66d0731 Compare May 20, 2017 23:19

ijokarumawak reviewed May 22, 2017

View reviewed changes

jfrazee force-pushed the NIFI-3404 branch 2 times, most recently from 335a137 to f9fb298 Compare May 24, 2017 09:42

mattyb149 reviewed May 24, 2017

View reviewed changes

jfrazee force-pushed the NIFI-3404 branch 3 times, most recently from d345267 to 56f1e58 Compare May 24, 2017 22:34

NIFI-3404 Added LookupAttribute processor and lookup controller services

b734398

jfrazee force-pushed the NIFI-3404 branch from 56f1e58 to b734398 Compare May 24, 2017 23:51

ijokarumawak reviewed May 25, 2017

View reviewed changes

jfrazee closed this May 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-3404 Add lookup processor for enrichments/joins to reference data #1830

NIFI-3404 Add lookup processor for enrichments/joins to reference data #1830

jfrazee commented May 19, 2017

ijokarumawak commented May 22, 2017

jfrazee commented May 22, 2017

ijokarumawak left a comment

ijokarumawak May 22, 2017

ijokarumawak May 22, 2017

jfrazee May 22, 2017

ijokarumawak May 22, 2017

jfrazee May 22, 2017

ijokarumawak May 22, 2017

jfrazee May 22, 2017

ijokarumawak May 22, 2017

ijokarumawak May 22, 2017

mattyb149 May 24, 2017

mattyb149 May 24, 2017

mattyb149 May 24, 2017

jfrazee May 24, 2017

jfrazee commented May 25, 2017

ijokarumawak commented May 25, 2017

ijokarumawak May 25, 2017 •

edited

Loading

ijokarumawak May 25, 2017

jfrazee May 25, 2017

ijokarumawak May 25, 2017

ijokarumawak commented May 25, 2017

jfrazee commented May 25, 2017

NIFI-3404 Add lookup processor for enrichments/joins to reference data #1830

NIFI-3404 Add lookup processor for enrichments/joins to reference data #1830

Conversation

jfrazee commented May 19, 2017

ijokarumawak commented May 22, 2017

jfrazee commented May 22, 2017

ijokarumawak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfrazee commented May 25, 2017

ijokarumawak commented May 25, 2017

ijokarumawak May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijokarumawak commented May 25, 2017

jfrazee commented May 25, 2017

ijokarumawak May 25, 2017 •

edited

Loading