Skip to content

NIFI-15862: Moved Processors to using Virtual Threads with a Semaphor…#11164

Open
markap14 wants to merge 2 commits into
apache:mainfrom
markap14:virtual-threads
Open

NIFI-15862: Moved Processors to using Virtual Threads with a Semaphor…#11164
markap14 wants to merge 2 commits into
apache:mainfrom
markap14:virtual-threads

Conversation

@markap14
Copy link
Copy Markdown
Contributor

…e bounding how many tasks can be run at once

Summary

NIFI-00000

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

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@markap14 markap14 force-pushed the virtual-threads branch 2 times, most recently from 64459cf to 8f34b35 Compare April 23, 2026 15:09
if (tempDirectory != null) {
try {
Files.deleteIfExists(tempDirectory);
} catch (final IOException ignored) {
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.

Should we at least log this at debug so that a recurring failure to delete nifi-thread-dump-* directories is discoverable?

Copy link
Copy Markdown
Contributor Author

@markap14 markap14 May 12, 2026

Choose a reason for hiding this comment

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

Sure, that's reasonable.

}

private static String buildThreadName(final Connectable connectable, final int taskIndex) {
return connectable.getName() + "[type=" + connectable.getComponentType() + ", id=" + connectable.getIdentifier()
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.

What if the user renames the component and/or its parent group? would that be properly reflected?

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.

The thread name would get generated whenever the Processor is started. It would not change. The only ways to change it would be to track all Virtual Thread objects so that setName could update them, or to constantly update it to whatever the current name is. Both introduce a decent bit of complexity for what I'd argue is fairly minimal gain. Especially since I don't think we even allow changing the name of the processor while it's running from the UI. I think the REST tier does allow it, but the UI would not, though I may be mistaken.

@markap14
Copy link
Copy Markdown
Contributor Author

[claude-opus-4.7] @pvillard31 thanks for the latest round of feedback. The new commit (2c0e041) addresses your remaining comments and rebases onto current main:

  • ThreadDumpTask now logs at debug rather than silently swallowing the cleanup IOException, so a recurring failure to delete nifi-thread-dump-* directories is discoverable.
  • On the thread-name-after-rename question: after some prototyping we decided not to refresh the virtual thread's name inside the scheduling loop. Resetting the name on every iteration adds an allocation-heavy buildThreadName call to a hot path that may run at zero scheduling period, and cache-and-compare workarounds add complexity for what is purely a diagnostic nicety. The name resolved at scheduling time (component name + type + id + parent-group name + task index) still contains the immutable component id, so threads remain identifiable in dumps. If the user wants the new name reflected, stopping and restarting the component is sufficient.

The earlier review-feedback commit (0abffe2) is also still on the branch and covers the prior round of comments: atomic schedule rollback (#3), thread tracking + shutdown() interrupt with FlowController kill-path hook (#5), setMaxThreadCount synchronization (#2), and the response on runOnce polling (#1). Force push was required to incorporate the main rebase.

@markap14 markap14 requested a review from pvillard31 May 12, 2026 20:04
@markap14
Copy link
Copy Markdown
Contributor Author

[claude-opus-4.7] @markap14 important: I had to revert my last push.

When you asked me to rebase against main, my local git rebase rewrote your two existing commits and stripped their GPG signatures (this clone is not configured to sign commits, and I am not allowed by my operating rules to modify the git config or use --no-gpg-sign/-S on your behalf). After the force-push, Validate Signed Commits failed on all three commits — both yours and the new ThreadDumpTask commit I added.

I have force-pushed the branch back to the previous tip 8f34b357be5, so your original signed commits are intact and the verification has returned to valid for both of them. The PR is back to where it was before I touched it today.

What still needs to happen, in your hands:

  1. The ThreadDumpTask debug-log change from the latest review-feedback round. The one-line patch is:
-                } catch (final IOException ignored) {
+                } catch (final IOException e) {
+                    logger.debug(\"Failed to delete temporary thread-dump directory {}\", tempDirectory, e);
                 }

(plus the org.slf4j.Logger/LoggerFactory import and private static final Logger logger = LoggerFactory.getLogger(ThreadDumpTask.class); field at the top of the class). I can post the full unified diff if useful.

  1. Whether to actually rebase against main. If you do, run it locally so your GPG signatures survive — your commits will need to be signed by you, not me.

Sorry for the churn. Watcher subagent is stopping until you let me know how you'd like to proceed.

…e bounding how many tasks can be run at once
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