joewitt commented on PR #11619: URL: https://github.com/apache/nifi/pull/11619#issuecomment-5649117060
+1 merging to main. Couple of comments to consider as a follow-up ## Comments **1. Yield when you cannot run** After unschedule, `lifecycle == null` → `onTrigger` returns. After primary revoke, generation is retired → `poll`/`canCreate` fail → `worker == null` → return. No `context.yield()`. NiFi still considers a PRIMARY-only processor scheduled across revoke (`onPrimaryNodeChange` is written for `isScheduled() && runOnPrimary`). Timer-driven will invoke `onTrigger` again as soon as it returns. Old code at least did JMS I/O (wrong, but it paced itself). New code can **spin**. Same for the `lifecycle == null` path if a trigger is dispatched after unschedule. Fix: `yield()` on those early returns (not on the “register lost the race” path, that is rare and the next trigger is fine). Small, in `AbstractJMSProcessor.onTrigger`. I would make that edit if you are already cherry-picking. **2. `destroy()` failure is at-most-once, including when it threw** CAS is set **before** `connectionFactory.destroy()`. If destroy throws, nobody retries. The unit test locks that in. Spring’s `destroy()` is usually safe to call once and forget; if a broker client can fail half-closed, that worker’s connection can leak. Acceptable tradeoff versus double-destroy. Do not “fix” it by moving CAS after destroy unless you also make destroy re-entrant. **3. Nits, not blockers** - `@OnUnscheduled` method is still named `shutdownConnectionFactoryProvider` and now closes workers. Rename while you are in there, or leave it. - `canCreateWorker` still does not cap `allWorkers` at `maxConcurrentTasks`. Two threads can both see an empty idle queue and both create. Pre-existing. Extra workers get `shutdown()` on release when the idle queue is full. - `AbstractJMSProcessorTest` duplicates `TestWorker` / `DestroyTrackingCachingConnectionFactory` with `JMSWorkerTest`. Fine. - Processor-level path (`onTrigger` → register/rebuild) is covered by ITs, not by the lifecycle unit tests. That is the right split. -- 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]
