NIFI-5561 - Add component name filtering to S2S Provenance Reporting … by pvillard31 · Pull Request #2973 · apache/nifi

pvillard31 · 2018-08-29T12:58:52Z

…Task

Thank you for submitting a contribution to Apache NiFi.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?

For code changes:

Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

alopresto · 2018-08-30T04:22:53Z

I created a flow which uses 3 GenerateFlowFile processors (named Component A, Component A, and Component A (B)) to evaluate the new filtering behavior. Each generates a flowfile with the content and source attribute indicating the source (for the 2nd processor, the message & attribute refer to Component A (2)). I exercised the SiteToSiteProvenanceReportingTask with no accept/deny list, then only an accept value, then both. The functionality worked as expected; explicit deny overrode allow (see screenshot).

The template is available here.

alopresto · 2018-08-30T05:09:03Z

...g-utils/src/main/java/org/apache/nifi/reporting/util/provenance/ProvenanceEventConsumer.java

        return componentTypeRegex != null || !eventTypes.isEmpty() || !componentIds.isEmpty()
-                || componentTypeRegexExclude != null || !eventTypesExclude.isEmpty() || !componentIdsExclude.isEmpty();
+                || componentTypeRegexExclude != null || !eventTypesExclude.isEmpty() || !componentIdsExclude.isEmpty()
+                || componentNameRegex != null || componentNameRegexExclude != null;


In general, I don't push changing "traditional" code blocks to "new" style just for the sake of it, but in this case, I think a Java 8-style .stream() construction will make this clearer (some of these fields are List and some are Pattern, and the Pattern can be empty/blank (which is not checked here), which would not (logically) enable filtering.

I made two commits (a regression test on the current logic and a new implementation) and it still works. Personally, I think this form is clearer and will scale more gracefully if new properties are added. Let me know your thoughts.

Yeah agree, I added your two commits on this pull request. Thanks!

alopresto · 2018-08-30T05:12:35Z

...eporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteProvenanceReportingTask.java

+        .description("Regular expression to filter the provenance events based on the component name. Only the events matching the regular "
+                + "expression will be sent. If no filter is set, all the events are sent. If multiple filters are set, the filters are cumulative.")
+        .required(false)
+        .addValidator(StandardValidators.REGULAR_EXPRESSION_VALIDATOR)


Should these properties support Expression Language (Variables Only)? I can imagine a variable being set with a "component name filter" specific to a process group. Thoughts?

Since it's a reporting task, it won't access the variable registries defined at UI-level. I believe this will only access the global file-based variable registry (defined in nifi.properties) and the environment/jvm variables. Not sure this provides much value but on the other hand it's easy to add. Thoughts?

Our CM story is still really developing at this point, so I would say whatever could be done to help facilitate making it easier for admins to configure NiFi in a standardized way is going to make the a little happier even if it's just "copy and paste this EL string."

Fair enough, I'll add it on the properties. On a related note, I hope to find time to start exploring NIFI-5367 to enable VR scope by default in NiFi.

That's an excellent point about the VR scoping that I forgot. I think this is nominally more "consistent" across the app, but you're right that it's not as valuable as I expected. Thanks.

…Task

…ngEnabled().

…as other attributes are added using Streams.

pvillard31 · 2018-08-30T11:29:40Z

Added VR scope on properties and fixed indentation to have the same everywhere. To see the changes and masking the indentation changes: https://github.com/apache/nifi/pull/2973/files?w=1

alopresto · 2018-08-30T17:02:12Z

Thanks Pierre. Running a final check and will merge.

alopresto reviewed Aug 30, 2018

View reviewed changes

pvillard31 and others added 4 commits August 30, 2018 13:25

NIFI-5561 - Add component name filtering to S2S Provenance Reporting …

ca9c19c

…Task

NIFI-5561 Added regression test for ProvenanceEventConsumer#isFilteri…

9d43fb6

…ngEnabled().

NIFI-5561 Changed isFilteringEnabled implementation to be expandable …

a3e14eb

…as other attributes are added using Streams.

EL + indentation

2d82162

pvillard31 force-pushed the NIFI-5561 branch from c0c29ae to 2d82162 Compare August 30, 2018 11:25

asfgit closed this in cbd942d Aug 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-5561 - Add component name filtering to S2S Provenance Reporting …#2973

NIFI-5561 - Add component name filtering to S2S Provenance Reporting …#2973
pvillard31 wants to merge 4 commits intoapache:masterfrom
pvillard31:NIFI-5561

pvillard31 commented Aug 29, 2018 •

edited

Loading

Uh oh!

alopresto commented Aug 30, 2018

Uh oh!

alopresto Aug 30, 2018

Uh oh!

pvillard31 Aug 30, 2018

Uh oh!

alopresto Aug 30, 2018

Uh oh!

pvillard31 Aug 30, 2018

Uh oh!

MikeThomsen Aug 30, 2018

Uh oh!

pvillard31 Aug 30, 2018

Uh oh!

alopresto Aug 30, 2018

Uh oh!

pvillard31 commented Aug 30, 2018

Uh oh!

alopresto commented Aug 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

pvillard31 commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

alopresto commented Aug 30, 2018

Uh oh!

alopresto Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

pvillard31 Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

alopresto Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

pvillard31 Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

MikeThomsen Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

pvillard31 Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

alopresto Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

pvillard31 commented Aug 30, 2018

Uh oh!

alopresto commented Aug 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pvillard31 commented Aug 29, 2018 •

edited

Loading