tpalfy commented on pull request #5692: URL: https://github.com/apache/nifi/pull/5692#issuecomment-1022104731
The reason why we can't rely on the ```DBCPConnectionPool.getDriver()``` is that the subsequent ```Class.forName()``` call uses the classloader of the caller class. Which would be the nifi-dbcp-service NAR classloader which doesn't see the snowflake driver as that is included in the nifi-snowflake-services NAR. As for the ```onConfigured()``` I see no new argument. I understand that there is a level of duplication but I think it's fine for the reasons stated before. Here's a bit more detailed explanation. If I keep the new propertyDescriptors (to be able to have separate ```name``` and ```displayName``` for them) then the shared code would require a refactor that would introduce the same amount of complexity we try to prevent with the refactor in the first place. I could create a method something like ```setupDatasource``` but it would get a bunch of parameters or would get a DTO which I would need to call getters and setters on - just like on the ```datasource``` itself. Again, it's not business logic but a sequence of getter and setter calls. No need to overengineer it. The case would be different if I didn't change the ```propertyDescriptors``` at all. In that case the ```setupDatasource``` could receive the ```context``` and the refactor would be clean. I don't like this direction because we would give up a higher level cleanness for a lower level one. But I can be convinced. If there are others as well who would vote for this approach I'll do the change. -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
