Skip to content

NIFI-16051 Propagate logging attributes down the process group tree#11372

Merged
pvillard31 merged 2 commits into
apache:mainfrom
boguszj:NIFI-16051
Jun 29, 2026
Merged

NIFI-16051 Propagate logging attributes down the process group tree#11372
pvillard31 merged 2 commits into
apache:mainfrom
boguszj:NIFI-16051

Conversation

@boguszj

@boguszj boguszj commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Summary

NIFI-16051

Propagating the logging attributes down solves 2 issues:

  • missing registeredFlowIdentifierPath when flow is first imported from the registry
  • lack of registeredFlowIdentifierPath propagation down to the children when VCI changes on the root PG

Technically, the 1st issue could probably be fixed in the initialization process itself, but propagation fixes both & makes sure they are always synchronized.

Additionally moving from a mutating the map, to overriding with an immutable instance would make sure there are no partial reads.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000
  • Pull request contains commits signed with a registered key indicating Verified status

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using ./mvnw clean install -P contrib-check
    • JDK 21
    • JDK 25

@pvillard31 pvillard31 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and happy to merge as-is but leaving two comments up for discussion


for (final ProcessGroup childGroup : processGroups.values()) {
if (childGroup instanceof StandardProcessGroup standardChildGroup) {
standardChildGroup.setLoggingAttributes();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since setConnectorLoggingAttributes already recurses into every child and each child now also calls setLoggingAttributes(), each descendant gets recomputed once per ancestor level. Did you consider skipping this subtree recursion when it is invoked from the connector cascade, to avoid the repeated recomputation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I was thinking that this happens rarely enough that added computation is not a problem, so I kept things simple, but on another thought maybe being explicit about it is a better idea. I know boolean arguments probably aren't ideal, but since similar pattern is also used for getControllerServices I added one here


this.loggingAttributes = Map.copyOf(attributes);

for (final ProcessGroup childGroup : processGroups.values()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sibling method setConnectorLoggingAttributes iterates getProcessGroups() (a read-locked copy) while this loop iterates processGroups.values() directly. Both are safe with a ConcurrentHashMap, but should we use getProcessGroups() here too for consistency?

@boguszj boguszj requested a review from pvillard31 June 29, 2026 13:30

@pvillard31 pvillard31 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and will merge assuming checks are green

@pvillard31 pvillard31 merged commit f831020 into apache:main Jun 29, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants