bobpaulin commented on PR #10688: URL: https://github.com/apache/nifi/pull/10688#issuecomment-3700510606
> Thanks for working on this improvement @bobpaulin. > > On a structural review, I'm not sure that `ReloadComponent` is the right place to add the new methods. It might make sense to implement some the loading logic in the `ProcessorNode` and `ControllerServiceNode` classes themselves. That should also avoid the need for duplicative implementations in the Stateless and Standard ReloadComponents. > I am also skeptical about ReloadComponent. However that is what the temporary component is doing. Its create a temporary instance to verify that will eventually be replace when the configuration is applied and the component is reloaded. It has all the object references required to swap in all the parts of the existing component. The issue with StandardProcessorNode and StandardControllerServiceNode is: 1) It does not have access to the StateManager so we'd need to wire that in only for this purpose. 2) It does not have access to the KeberosConfig. This can be built with the NiFiProperties class however that is also not currently wired in. 3) It does not have access to the FlowController (NodeTypeProvider). These would all need to be wired into both of the above classes which in my opinion might introduce unintended coupling in the future. So while I do agree ReloadComponent is a little awkward I think it is better than opening up access to all these objects directly in the ProcessorNode and ControllerServiceNode classes. > One fundamental note is that the `ExtensionManager` interface and corresponding implementation have methods for creating temporary components, including logic for handling Python Processors and working with the bundle ClassLoader. Decoupling the creation logic from the `initialize` call also seems important for clarity of behavior. > Yes I noticed the TempComponents there first. My original plan was to go in that direction. The ExtensionManager faces similar challenges as StandardProcessorNode and StandardControllerServiceNode where there are additional classes that would need to be wired in to get access to state, keberos and FlowController. The current "TempComponents" use a MockControllerServiceInitializationContext for these pieces which might produce inconsistent behavior in the verify method as all 3 can be accessed within the ControllerService or Processor verify methods. This seems like a more appropriate place for the behavior but I was concerned with confusing these TempComponents with the existing ones. Not sure if just better naming could do the trick here but this seemed like a more significant change than using the ReloadComponent. > I can take a closer look soon, but those are some initial elements to consider. Appreciate the feedback @exceptionfactory! Not sure if there is another path to consider here but I think there are trade offs with all 3 possibilities above: 1. ReloadComponent - a bit awkward since reload's purpose is generally for reloading the existing component 2.ExtensionManager - introduces additional object dependencies and confusion over existing TempComponents. 3. StandardProcessorNode and StandardControllerServiceNode - exposing additional object dependencies might encourage coupling in the future. The options above are in my current preferred order but I'm open to changing. -- 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]
